Add a switch to control writing to /proc/sys

[alexjh]

Not all environments allow writing to /proc/sys, allow these to be
controlled via config setting added in cloudfoundry/capi-release#3.

[cfdreddbot]

Hey alexjh!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/115951553.

[sax]

Hi @alexjh. Thanks for the contribution! @simonleung8 and I were looking at these two pull requests, and there were a few suggestions that we had before accepting them.

tweak_proc_sys is a bit ambiguous, particularly since the one place it is used is to control whether or not to configure the path and file pattern for writing core files. Ideally, this would configure cc.core_file_pattern, with the current default. You might then need to use the if_p helper (http://bosh.io/docs/reference/jobs.html). An alternative would be to retain the boolean setting, but rename it to something more specific to its usage, for instance cc.persist_core_files.

Also, in our tests this variable was not actually passed through to the template, and raised an error for us when we tried to load this in our bosh-lite instance. The scripts to generate manifests pass properties through https://github.com/cloudfoundry/cf-release/blob/master/templates/cf-properties.yml, and does not seem to automatically assign defaults from the job spec files. Are you merging in another properties file to generate your bosh manifest?

It seems like you would need to add a default in cf-properties.yml, for instance:

properties:
  ......
  cc:
    persist_core_files: (( merge || true ))
    ......or
    core_file_pattern: (( merge || "/var/vcap/sys/cores/core-%e-%s-%p-%t" ))

Then the spec file would serve more as documentation of the properties.

[sax]

@alexjh It turns out that the errors we saw were caused by a different issue, so please ignore the part about cf-properties.yml above. It still would be nice for this property be named closer to what it actually does, though.

Thanks!

[alexjh]

Agreed, I think that cc.core_file_pattern would be more flexible, I'll update soon.

[emalm]

Changes proposed in cloudfoundry/diego-release#146 may also be relevant for this PR.

[SocalNick]

@alexjh - would you like to close or update this per the work in cloudfoundry/diego-release#146?

[alexjh]

@SocalNick I think 4946fce reflects the suggestions made on the other PRs, ie make the setting a value and wrap it in an if_p call. (running_in_container isn't needed here)

[SocalNick]

@alexjh - I was assuming you would just guard this with the running_in_container function. Do you have a use case for writing a pattern to /proc/sys/kernel/core_pattern when running in a container? Does that even work?

[alexjh]

Hi @SocalNick, I was assuming that if this wasn't wrapped already, it wouldn't be a problem for bosh-lite. I'm not sure how I can test this in the bosh-lite vagrant box, I've been looking for documentation on how to do this, but I haven't found anything yet. Do you have any pointers to where I can learn how to do this? I think that a garden equivalent of docker exec is what I'm looking for.

[alexjh]

Just realized I made this PR against master. I'm going to close and open a new PR done properly.

[simonleung8]

@alexjh Master will be the right branch for pull request. Go ahead and reopen this PR if you like.

[alexjh]

😓 I was reading the cf-release README. Reopening.

[cfdreddbot]

Hey alexjh!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

[ameowlia]

Hi @alexjh,

@rizwanreza and I were looking at your trio of PRs, but I see that you are still editing the code. This PR is currently not valid Ruby, as the if_p is a custom method to check property's presence you can use in manifest files, but if p does not take a block and requires the attribute to be present.

Feel free to make the change and let us know when we can test it again.

Thanks,
Amelia @adowns01 and Riz @rizwanreza, CAPI Team members

[alexjh]

@adowns01 @rizwanreza I've pushed my update with the fix for if p.

[ameowlia]

This has been manually merged into master. I don't know why it didn't update here.

Thanks,
Amelia @adowns01 and Riz @rizwanreza, CAPI Team Members