Add sysctlInit to OpenSearch Chart #278
Conversation
|
Hi @amirgo1 thanks for contribution, |
|
@peterzhuamazon @amirgo1 Before we approve this as well, we really want to make sure there's a consensus from everyone that this is how we want to do this. It's a little worrying that we need to do this via privileged containers that are changing the host OS sysctl settings. @smlx @mprimeaux I'll just invite everyone to get a "Yes, lets do this". Maybe thumbs up / Thumbs down? |
|
Adding @TheAlgo @prudhvigodithi to the discussion please see above msg. |
charts/opensearch/Chart.yaml
Outdated
| @@ -15,7 +15,7 @@ type: application | |||
| # This is the chart version. This version number should be incremented each time you make changes | |||
| # to the chart and its templates, including the app version. | |||
| # Versions are expected to follow Semantic Versioning (https://semver.org/) | |||
| version: 2.1.0 | |||
| version: 2.1.1 | |||
There was a problem hiding this comment.
Because the API (values file) is changing this is at least a minor version bump, rather than a bugfix.
There was a problem hiding this comment.
Yes this should be a minor version bump. I agree with @smlx
charts/opensearch/Chart.yaml
Outdated
| @@ -15,7 +15,7 @@ type: application | |||
| # This is the chart version. This version number should be incremented each time you make changes | |||
| # to the chart and its templates, including the app version. | |||
| # Versions are expected to follow Semantic Versioning (https://semver.org/) | |||
| version: 2.1.0 | |||
| version: 2.1.1 | |||
There was a problem hiding this comment.
Yes this should be a minor version bump. I agree with @smlx
|
@TheAlgo @smlx @prudhvigodithi please approve this if you are ok with the change, I will resolve the conflicts later. |
amirgo1
changed the title
|
@peterzhuamazon @TheAlgo my apologies for the delay. I updated the PR |
|
@TheAlgo, could you please review the changes I made? |
|
@TheAlgo I've made the changes |
| - -c | ||
| - | | ||
| set -xe | ||
| DESIRED="{{ .Values.sysctl.VmMaxMapCount }}" |
There was a problem hiding this comment.
Can we add sample placeholders in values.yaml for vmMaxMapCount
|
@smlx @prudhvigodithi Can you guys please review it when free? Thanks |
|
@amirgo1 Any update on this PR? |
|
@TheAlgo Fixed comment and bumped version |
Signed-off-by: amirgo1 <[email protected]>
|
Resolved changelog conflict @prudhvigodithi @TheAlgo |
|
@amirgo1 Can you check the lint errors? |
|
We can take care of the lint error if there is still no responses from @amirgo1 next week. |
|
Any update? |
|
Seems like no update from @amirgo1 I will fix the link for him. |
|
Thanks @amirgo1 for the contribution and patience, we will merge this PR now. |
|
Backport 1.x: #349 |
Signed-off-by: amirgo1 [email protected]
Description
Add option to enable the use of
sysctlInitContainerto set sysctl vm.max_map_count through privilegedinitContainer.Issues Resolved
#187
Check List
For any changes to files within Helm chart directories:
CHANGELOG.mdupdated to reflect changeBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.