Helm-operator: Changes to helm chart not being detected

[rally25rs]

Describe the bug

After a failed helm release, helm-operator seems to have difficulty identifying changes to the chart.

To Reproduce

I made a commit that had some helm chart changes, but also included an error that prevented the helm release from passing.

Specifically, inside an existing Deployment I

• Edited spec.template.spec.containers[0].volumeMounts
• Edited spec.template.spec.volumes
• Added spec.template.spec.containers[0].metadata

It looks like like spec.template.spec.containers[0].metadata was misplaced, that is not a valid attribute, which resulted in the helm release error

status:
  conditions:
  - lastTransitionTime: "2020-03-23T15:01:30Z"
    lastUpdateTime: "2020-03-23T15:09:58Z"
    message: 'failed to upgrade chart for release [qa]: error validating "": error
      validating data: ValidationError(Deployment.spec.template.spec.containers[0]):
      unknown field "metadata" in io.k8s.api.core.v1.Container'
    reason: HelmUpgradeFailed
    status: "False"
    type: Released

I then added a new commit that deleted the offending spec.template.spec.containers[0].metadata and moved it to spec.template.metadata

This resulted in helm-operator reporting:

ts=2020-03-23T15:42:58.393469753Z caller=release.go:404 component=release release=qa targetNamespace=qa resource=qa:helmrelease/qa helmVersion=v3 info="no changes" action=skip

The changes I had made to volumes and volumeMounts was never deployed, nor was the updated metadata.

Expected behavior

I expect helm-operator to detect that there are several changes to the chart compared to wat was last successful deployed, and make a deployment.

Logs

(log snippets above)

Additional context

• helm-operator version: fluxcd/helm-operator:1.0.0-rc9
• Kubernetes version: AWS EKS 🤷‍♂
• Git provider: github
• Container registry provider: github

[hiddeco]

Not able to replicate this following the exact steps using this dummy chart.

Following the described steps the Helm operator synchronized on commit https://github.com/hiddeco/test-charts/commit/89fa405a10ef1c26daa5b51b6ddb59d7eb4ce204, https://github.com/hiddeco/test-charts/commit/be3859262d20c7c949b1e55774ee053f8778e73e, https://github.com/hiddeco/test-charts/commit/c6e16acafd456392be26efc8f1c111e76b0be743, and finally the revert commit https://github.com/hiddeco/test-charts/commit/a95e8537495efd8682460bafbd4919ec6baf8293.

The HelmRelease successfully recovered once the revert commit was pushed:

ts=2020-03-25T00:15:46.977480083Z caller=helm.go:69 component=helm version=v3 info="preparing upgrade for default-undetected-changes" targetNamespace=default release=default-undetected-changes
ts=2020-03-25T00:15:47.279102919Z caller=release.go:188 component=release release=default-undetected-changes targetNamespace=default resource=default:helmrelease/undetected-changes helmVersion=v3 error="failed to determine if the release should be synced" err="failed to upgrade chart for release [default-undetected-changes]: error validating \"\": error validating data: ValidationError(Deployment.spec.template.spec.containers[0]): unknown field \"metadata\" in io.k8s.api.core.v1.Container"
...
resource=default:helmrelease/undetected-changes helmVersion=v3 info="dry-run differed" action=upgrade
ts=2020-03-25T00:16:36.458352613Z caller=helm.go:69 component=helm version=v3 info="preparing upgrade for default-undetected-changes" targetNamespace=default release=default-undetected-changes
ts=2020-03-25T00:16:36.697703631Z caller=helm.go:69 component=helm version=v3 info="performing update for default-undetected-changes" targetNamespace=default release=default-undetected-changes
ts=2020-03-25T00:16:36.704372657Z caller=helm.go:69 component=helm version=v3 info="creating upgraded release for default-undetected-changes" targetNamespace=default release=default-undetected-changes
ts=2020-03-25T00:16:36.724250575Z caller=helm.go:69 component=helm version=v3 info="checking 1 resources for changes" targetNamespace=default release=default-undetected-changes
ts=2020-03-25T00:16:36.74969771Z caller=helm.go:69 component=helm version=v3 info="updating status for upgraded release for default-undetected-changes" targetNamespace=default release=default-undetected-changes
ts=2020-03-25T00:16:36.827943359Z caller=release.go:265 component=release release=default-undetected-changes targetNamespace=default resource=default:helmrelease/undetected-changes helmVersion=v3 info="Helm release sync succeeded" revision=a95e8537495efd8682460bafbd4919ec6baf8293

[rally25rs]

I continue to fairly regularly have issues. Yesterday afternoon I pushed a commit that changed the version of nginx-ingress chart that I was depending on (so charts/nginx-ingress-x.x.x.tgz changed, as well as the chart lockfile) and the change was not deployed. More frustrating, I actually changed the nginx-chart dependency version in 2 releases/namespaces both being monitored by the same flux and helm-op instance, and one of those releases was properly updated. The other one wasn't.

This morning in an effort to fix it, I manually deleted the out of date nginx services

kubectl delete service infrastructure-nginx-ingress-controller -n infrastructure
kubectl delete service infrastructure-nginx-ingress-default-backend -n infrastructure

and help-operator continues to report "no changes"

ts=2020-03-25T12:21:35.970686079Z caller=release.go:360 component=release release=infrastructure targetNamespace=infrastructure resource=infrastructure:helmrelease/infrastructure helmVersion=v3 info="performing dry-run upgrade to see if release has diverged"
ts=2020-03-25T12:21:35.998293454Z caller=helm.go:69 component=helm version=v3 info="preparing upgrade for infrastructure" targetNamespace=infrastructure release=infrastructure
ts=2020-03-25T12:21:37.082273072Z caller=helm.go:69 component=helm version=v3 info="performing update for infrastructure" targetNamespace=infrastructure release=infrastructure
ts=2020-03-25T12:21:37.351970595Z caller=helm.go:69 component=helm version=v3 info="dry run for infrastructure" targetNamespace=infrastructure release=infrastructure
ts=2020-03-25T12:21:37.4322061Z caller=release.go:404 component=release release=infrastructure targetNamespace=infrastructure resource=infrastructure:helmrelease/infrastructure helmVersion=v3 info="no changes" action=skip

I get that these kinds of things are really hard to reproduce in a different environment.
Some things to consider as feature requests;

• maybe a better process for debugging? It'd be nice if helm-op saved off the result of the helm upgrade --dry-run somewhere, or had a way to get the output of a helm template so the result could be viewed for correctness.
• is there really a great reason to have the check at all? when helm goes to apply it to k8s and there is no change, doesn't k8s already detect that?
• Or maybe a way to skip the check once and force a helm update, even if the way to do that is to kubectl exec -- bash into the helm-op pod and run a command manually to kick it off, it would be better than having to guess at it or delete the entire namespace and start over.

---

Additional details:

$ kubectl get service infrastructure-nginx-ingress-controller -n infrastructure -o yaml

contains

  labels:
    app: nginx-ingress
    chart: nginx-ingress-1.34.0

  selector:
    app: nginx-ingress
    component: controller
    release: infrastructure

then doing

kubectl delete helmrelease infrastructure -n infrastructure

then helm-op reports

ts=2020-03-25T12:58:27.869568721Z caller=helm.go:69 component=helm version=v3 info="purge requested for infrastructure" targetNamespace=infrastructure release=infrastructure

eventually helm replaces the helmrelease and helm-op redeploys everything.

The result is

kubectl get service infrastructure-nginx-ingress-controller -n infrastructure -o yaml

contains

  labels:
    app: nginx-ingress
    chart: nginx-ingress-1.34.2

  selector:
    app: nginx-ingress
    app.kubernetes.io/component: controller
    release: infrastructure

which is now correct (note that the chart version changed and the selectors are different)

---

This could totally just be a bug in helm3 where the dry-run isn't detecting changes, but without the output from that command, it's hard to tell...

[hiddeco]

Yesterday afternoon I pushed a commit that changed the version of nginx-ingress chart that I was depending on (so charts/nginx-ingress-x.x.x.tgz changed, as well as the chart lockfile) [...]

This may be a regression bug and was also reported in #338. I am going to take a look at this tonight.

This morning in an effort to fix it, I manually deleted the out of date nginx services [...] and help-operator continues to report "no changes"

The Helm Operator does not look at live cluster-state (yet). See #317.

is there really a great reason to have the check at all? when helm goes to apply it to k8s and there is no change, doesn't k8s already detect that?

The revision of the Helm chart will drift, making historical rollbacks impossible given the advised (and default) history limit of 10 revisions.

Or maybe a way to skip the check once and force a helm update.

A feature like this based on an annotation is in the pipeline.

[rally25rs]

Another example.

This change:

results in

release=qa targetNamespace=qa resource=qa:helmrelease/qa helmVersion=v3 info="no changes" action=skip

[hiddeco]

What is the folder / file structure of the above chart?

[rally25rs]

It's a chart with subcharts in it's charts/ folder, like

+- charts
|   +- cloud-services
|       +- Chart.yaml
|       +- charts
|           +- platform-ui
|               +- Chart.yaml
|               +- templates
|                   +- service.yaml <-- this is the file that changed
+- releases
    +- qa
        +- cloud-services.yaml <-- HelmRelease that specifies charts/cloud-services of this repo

[hiddeco]

Can you give a pseudo-example of the requirements.yaml file of the above chart?

[rally25rs]

Helm 3.

Chart.yaml would be something like

apiVersion: v2
name: cloud-services
version: 0.0.0
description: "Cloud Services"
dependencies:
- name: "nginx-ingress"
  repository: "https://kubernetes-charts.storage.googleapis.com/"
  version: "^1.26.0"
- name: "rabbitmq"
  condition: rabbitmq.enabled
  repository: "https://kubernetes-charts.storage.googleapis.com/"
  version: "~6.10.1"
- name: "platform-ui"
  condition: platform-ui.enabled
  version: "^0.0.0"

(also edited my dir layout above. I had forgot the templates/ directory)

[sa-spag]

I have another case where a change in a chart would not lead to an upgrade.

1. Create a chart such as:

.
├── Chart.yaml
├── files
│   └── config.yaml
├── templates
│   └── configmap.yaml
└── values.yaml

... with templates/confimap.yaml...

apiVersion: v1
kind: ConfigMap
metadata:
  name: {{ .Release.Name }}
data:
  config.yaml: |+
    {{- tpl (.Files.Get "files/config.yaml") . | nindent 4 }}

1. Do some changes in files/config.yaml
2. Notice the Helm Operator not upgrading the release as it only compares values and templates

[hiddeco]

Now that I have a clear picture of the changes made, I have to conclude that both reported issues are rather 'limitations of the feature' than an actual bug.

1. Stored release data does not contain metadata about dependencies, so we are unable to diff on metadata from that as it will result in spurious upgrades (the dry-run _has_ metadata, but the release received from storage _does not_).
2. We can not diff on the generated manifest, as it is not uncommon for charts to have generated metadata in e.g. annotations or what-not that differs on every dry-run. Again resulting in spurious upgrades.
3. People have a habit of templating in common configuration using an include. The contents of this include are not visible in the chart metadata, only in the generated manifest. Which poses the same problem as laid out in 2.

While I look for alternative solutions to the above (one thing that comes to mind is taking a shortcut to the upgrade action on a revision change that touches the chart path), it is already solvable by not relying on just file updates, but also making sure the metadata of your chart is changed. Bumping versions in the Chart.yaml file (and updating your dependency lock file, to mark the change in case of a dependency) should be sufficient for this.

[rally25rs]

@hiddeco would you mind better explaining what the "feature" is? To me it is unclear why helm-operator would need to diff the data.
If there was a github commit then there is a change. Just send it to helm. If helm outputs the same template as last time, then the k8s API will already not modify things.
I'm sure the diff is there to solve some problem, but I'm not clear on what that problem is.

[hiddeco]

This would corrupt the Helm storage with bogus revisions not representing an actual change, as Helm will still increase the revision number of the release (and prune older revisions) while no changes have been applied to resources.

It would also result in the operator upgrading all releases every --chart-sync-interval, as this would simply run the upgrade every time instead of making a comparison.

In addition to all of this, it is also used to detect when an upgrade for a rolled back release should be reattempted, etc.

We can definitely be smarter with the information Git is giving us but this needs to be thought out well given the above.

[rally25rs]

Bumping versions in the Chart.yaml file (and updating your dependency lock file, to mark the change in case of a dependency) should be sufficient for this.

Unfortunately changing the sub-chart version also does not result in a detected change, if the top-level chart has a range dependency (^1.0.0) or *.

[hiddeco]

@rally25rs please try hiddeco/helm-operator:optimize-6c1087e3 build from optimize. This takes latest attempted Git state into account.

[hiddeco]

Provided image is currently tested by multiple users. Results of this will determine how to move forward.

[sa-spag]

Just tested your Docker image with my aforementioned case and the operator successfully detect the change and ultimately applies it. Well done @hiddeco! 👏

[rally25rs]

After a few days of using it, the hiddeco/helm-operator:optimize-6c1087e3 seems to be doing well. It's a bit hard to keep track of how many updates I've made that only update a subchart, but I haven't noticed any instance of my deployment not being updated.

I appreciate the effort @hiddeco I think this build looks pretty good.

[hiddeco]

That makes the total amount of confirmations 5 which I'd consider sufficient evidence to close this issue for good. Thank you all for testing, it will be included in the next release :star2:

[carlpett]

@hiddeco We're running helm-operator 1.1.0, and still having upgrades not happening when we expect them. The new lastAttemptedRevision field is incremented, but the dry-run reports no changes. From your comment above, it's not clear to me when there will be an upgrade? As I read it, it does not trigger on changes to manifests, and not on metadata? What does trigger it?
Just now I commited a change to the values.yaml file in our repo (we use the git-chart approach), which does not cause helm-operator to upgrade.

If I can get a clear understanding of when upgrades are expected, I can contribute some docs.