Elasticsearch: do not include heritage selector #437
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
I don't think helm adds any sort of labels by default- this should not be an issue with helm 2 > helm 3 upgrade? The labels do not match the recommendations as you pointed out. If they are going to be changed though, there are a bunch of places that should happen. Plus, it's not a matter of just removing them- this one should be changed to app.kubernetes.io/managed-by The best possible change would be to use the helpers file to build up a common labels block and replace all the instances with it. |
|
heritage is "Tiller" in helm 2 and "Helm" in helm 3, thus having the new service with new selector filters out the old nodes, causing an unnecessary downtime during upgrade when new nodes are not yet ready. Changing the labels to the new style labels is an incompatible change that requires downtime (removing statefulsets with cascading BEFORE upgrading), so it is much harder to do, although probably wanted in the (not so near) future, so maybe those are two different problems. (Note that this is what I've done with kubernetes-dashboard, see the templates for labels https://github.com/kubernetes/dashboard/pull/4502/files#diff-c24a48865b91946a2e67f077b67682cfR37 and matchlabels https://github.com/kubernetes/dashboard/pull/4502/files#diff-c24a48865b91946a2e67f077b67682cfR48 , the downside being having to uninstall the Helm Release...) I can update the PR to add two templates like this but keeping backward compatibility. |
|
jenkins test this please |
|
Gentle up! ;) |
According to https://github.com/helm/charts/blob/master/REVIEW_GUIDELINES.md, service selector should not contain heritage. It would break (future) migration from helm 2 to helm 3 (during the rolling update only, there could be a downtime when only old nodes are ready but new services is there).
|
Rebased. |
|
jenkins test this please |
There was a problem hiding this comment.
LGTM, thanks for this PR and sorry for the delay @desaintmartin
According to https://github.com/helm/charts/blob/master/REVIEW_GUIDELINES.md, service selector should not contain heritage. It would break (future) migration from helm 2 to helm 3 (during the rolling update only, there could be a downtime when only old nodes are ready but new services is there).
According to https://github.com/helm/charts/blob/master/REVIEW_GUIDELINES.md, service selector should not contain heritage. It would break (future) migration from helm 2 to helm 3 (during the rolling update only, there could be a downtime when only old nodes are ready but new services is there).
|
backported to |
According to https://github.com/helm/charts/blob/master/REVIEW_GUIDELINES.md, service selector should not contain heritage.
It would break (future) migration from helm 2 to helm 3 (during the rolling update only, there could be a downtime when only old nodes with old labels are ready but service has been updated to new selector).
${CHART}/tests/*.py${CHART}/examples/*/test/goss.yaml