Allow statefulSet updates

[stepanstipl]

This fixes CouchDB updates by removing Version part from StatefulSet label, as Kubernetes don't allow updates to any other fields but 'replicas', 'template', and 'updateStrategy', the chart and heritage labels from thevolumeClaimTemplates which is immutable and therefore updating CouchDB service would be impossible.

Update: To update from current state use --force flag (see https://docs.helm.sh/helm/helm_upgrade/ for details). This will cause StatefulSet recreation, therefore downtime, but as long as you use persistent storage, the data will be preserved. delete the CouchDB StatefulSet before upgrading:

$ kubectl delete statefulsets --cascade=false my-release-couchdb

[desaintmartin]

As this is a breaking change (even if it broke before this PR), could you bump major version and explain how to upgrade? (see https://github.com/helm/charts/pull/7686/files or other PRs from #7680 for reference).
Edit: the problem is NOT here. IMHO you need to revert your changes and follow only the next paragraph. It is OK (and wanted) to have a field containing version in metadata.labels.

There is another problem with the volumeClaimTemplate described in #7803, could you update this as well and bump minor version instead of patch version?

cc @kocolosk

Thanks!

[stepanstipl]

@desaintmartin Thanks for reviewing this. I believe that removing the version from metadata.labels is required, as that field IS immutable in case of statefulSet (see #7680). Removing the version part of the label solved the issue for us.

Surprisingly we haven't experienced the #7803 in our case, but IMHO it makes sense to remove the version there as well and I have done so in this PR.

I have bumped the minor version and added not about update to the description of the PR.

[paulczar]

/ok-to-test

[desaintmartin]

I've quite tested and documented the three different upgrading problems we have at:
#7726
#7680
#7803
And if I'm not wrong (tell me if it is the case!), couchdb chart has the third problem, and you are trying to solve the second (but second is about spec.selector.matchLabels which is immutable, which is already the case here).

Can you sign your commits?

[stepanstipl]

@desaintmartin thanks, so the problem we've experienced and that I'm trying to solve is the first one - version in metadata.labels preventing the statefulSet upgrade (#7726)

We haven't experienced the 3rd one - version in spec.VolumeClaimTemplate.metadata.labels (#7803), but I have added fix for that as the version was in the label and I understood you asked for it in the previous comment and it seems reasonable.

I believe the 2nd one - version in spec.selector.matchLabels (#7680) does not apply here, as there's no version in the matchLabels as you pointed out. We have not experienced that issue, nor am I trying to fix it. (Referencing it in the previous comment was a mistake, I actually meant the 1st one - #7726).

Sorry for confusion :) and thanks for your time reviewing this.

Commits signed.

[desaintmartin]

The first one is about v1beta1 without explicit selector, so it doesnt match.

I'm interested, could you give me the commands used and errors encountered? It may be yet another issue.

[stepanstipl]

It's a bit more complicated - were using our fork of this chart with some added bits, you can see it at https://github.com/gpii-ops/gpii-infra/blob/master/shared/charts/couchdb. Also we're not installing the chart directly, but rather via exekube (https://github.com/exekube/exekube), which uses Terraform provider (https://github.com/terraform-providers/terraform-provider-helm/).

With the version in we would receive update error, and after removing it we don't have that problem - see commit gpii-ops/gpii-infra@28efcc1.

I'll try to find out the exact error and also reproduce directly with this chart and helm.

[desaintmartin]

I am really interested into a failing use case with the existing configuration and error log to be sure that we do not miss anything. But @unguiculus seems to agree with me. :)

[stepanstipl]

@desaintmartin @unguiculus thanks for review and comments. I've updated the labels as suggested -> the result is only removing the mutable label chart: and heritage: one from the volumeClaimTemplates of StatefulSet.

I've tested the upgrade process by changing the chart version and it seems to do the job.

[desaintmartin]

After some testing, this changes is in itself a breaking change (impossible to update from the existing broken state to your new version fixing this) and I did not realize it before with the other fixes of #7803.

You should bump a MAJOR version and document it like in https://github.com/helm/charts/pull/7686/files#diff-b290a06961a11374d72f584e5925762cR48 (with only the update about volumeClaimTemplate and expliaing how to delete without cascade the statefulset).

This will allow to finally close #7803.

[stepanstipl]

@desaintmartin thanks for the review, I have bumped the major version to 1.0.0 and added README section about upgrading.

I have tested upgrading via the proposed strategy (non-cascading delete of StatefulSet) and it worked well.

[desaintmartin]

Thanks! This lgtm now, ping @unguiculus

[scottrigby]

Thanks for doing this. @unguiculus requests were already changed. lgtm too 👍

[scottrigby]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: scottrigby, stepanstipl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [scottrigby]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Change requests were addressed