Remote helm charts should be upgraded by default

[yolkov]

We use Skaffold, not only for local development, but also for CI/CD.

skaffold build --profile build --file-output build.json
skaffold deploy --profile "${ENV}-infra" --images=internal/app1:faketag #we have to use a workaround [https://github.com/GoogleContainerTools/skaffold/issues/3559#issuecomment-632330117]
skaffold deploy --profile ${ENV} --build-artifacts build.json

After this issue Skaffold changed the behavior for remote Charts without the configuration option "upgradeOnChange: true" it will not deploy anything even if you change only values, and the Chart itself remains the same version.

Expected behavior

1. Default behavior for remote Charts should be changed to "upgradeOnChange: true" at least.
   Otherwise it breaks backward compatibility. For CI/CD we use Skaffold with remote Helm Charts only for dozens of services in production. Skaffold updating with this change force us to update all these product and infrastructure services.
   These changes are hard to test, because the problem occurs only when you change either a Chart version or values. I make changes to values of the Chart and expect them to apply, but I still need to enable some options.
   I don't understand the purpose of this option. For example, I have several services that depend on a single remote chart. I need to update the chart and deploy all dependent services from this chart. With the current option, I will have to do the following:

• Change the chart
• Enable the true flag for the service we want to deploy
• Deploy
• And turn off the flag again, because it is intended not to update the helm chart entities during the next deployment.

Changing values should always start deployment and execute the intended Chart logic, regardless of whether option "upgradeOnChange" is enabled or not. We make changes of Chart values advisedly and there is no need to confirm this with an additional option.
If you decide to keep this option, please add ENV for it.

2. The right decision in my opinion, there is no need in this option "upgradeOnChange".
   Helm supports version control https://helm.sh/docs/chart_best_practices/conventions/#version-numbers, https://helm.sh/docs/topics/charts/#charts-and-versioning (similar to python with requirements.txt, ruby with gemfile, php with composer.json, go modules with go. mod, etc). Correspondingly, if you don't change the version of the Chart, the Helm will do only what is inherent in the logic of the Chart.
   If there is no special logic for changes each time you deploy these changes, then Helm no deploy anything. Not need to intervene in the logic of the Charts, if it doesn't approach you then fork and modify Chart as you require or go to use another chart. Skaffold should not interfere in the logic of the Charts, except setting values that it is doing well. As a result, the fundamental error was made in this issue, they did not fix the Chart versions, and so the Chart version was updated when deployed, and Helm did what it was supposed to do. This is just the wrong use of the Helm.
   You may need to add a check. if "remote: true", the Chart version field should be mandatory.

Actual behavior

For remote Charts, and for local ones, too, if values change, the Chart must deployed and perform the logic laid down in the Chart(change configmaps, redeploy deployments, statefulsets, change ingress resources, etc.), but now Charts not deployed.

Information

• Skaffold version: v1.11.0
• Helm version: v2.16.1
• Operating system: linux/macos

P.S. Please do not break backward compatibility if possible. For companies that use Skaffold in production for CI/CD for dozens, hundreds of services, broken backward compatibility leads to a lot of work, either blocking the transition to the new version, or incidents in production.
(sorry for not good English)

[nkubala]

@yolkov thanks for the issue, and sorry we made your life harder with this change. this is a bit tough because for skaffold dev, I think we made the right choice here - upgrading remote charts by default seems a bit cumbersome, especially if you want to iterate on one piece of your application while keeping your remote dependencies like databases, etc. untouched. helm itself doesn't actually do any change detection when upgrading a release, and skaffold doesn't either, so our hands were tied here. this is what #3274 addressed.

however, for using skaffold deploy for CI/CD, the use case is different, and most likely we'd actually want to redeploy remote charts, especially if the values file is local. so I think we missed this here when we made our original decision.

if you don't change the version of the Chart, the Helm will do only what is inherent in the logic of the Chart.
can you explain this a bit more? my understanding is that on helm upgrade, helm will always create a new release (with a new revision version number), regardless of whether the chart changed. maybe i'm wrong though?

if this is true, then the main question here for me becomes:

• do we want to simply change the default value of upgradeOnChange to true (and force people doing skaffold dev to change their skaffold.yaml to reflect this new default); or,
• do we want to change the default behavior between skaffold dev and skaffold deploy, such that dev doesn't upgrade remote charts on redeploy but deploy does?

the first option is less confusing, but requires dev users to change their defaults which is not great, though arguably better than deploy users having to change their defaults across potentially dozens of skaffold.yamls. the second option, while maybe giving both sets of users what they want, could lead to confusing behavior, and could also be making false assumptions about the "expected" behavior.

cc @dahovey if you have any opinions on this :) and cc @dgageot if you have any thoughts

[dahovey]

Hi @yolkov, I definitely agree with your P.S..

@nkubala I don't have any hard opinions on this. I didn't use skaffold for CI/CD, so unfortunately, didn't work thru these scenarios. I guess I lean toward your second option. If upgradeOnChange is nil, and using skaffold dev then remote is used determine value of upgradeOnChange (as it is now), but if using skaffold deploy then when upgradeOnChange is nil the default value is true.

Since I only used part of the power of skaffold, your last statement is concerning:

could lead to confusing behavior, and could also be making false assumptions about the "expected" behavior.

[yolkov]

@nkubala

my understanding is that on helm upgrade, helm will always create a new release (with a new revision version number), regardless of whether the chart changed. maybe i'm wrong though?

Yes, helm creates new releases every time, but this is only a change to the metadata (which is stored in helm's configmap). If the helm chart version does not change and the values do not change, the kubernetes entities created by the chart(deployment,statefullsets, services, ingress, configmap, etc) will not deploy again and will not be changed.
Helm will compare the " new " release with the current metadata, and if there are no changes, nothing changes.

[yolkov]

For example:

apiVersion: skaffold/v2beta4
kind: Config
build:
deploy:
profiles:
- name: dev-local
  deploy:
    helm:
      releases:
      - name: example-app-mysql
        chartPath: stable/mysql
        valuesFiles:
        # - ./dev/values.mysql.yaml
        setValues:
          persistence.enabled: false
        namespace: example-app
        version: 1.2.1
        remote: true
      - name: example-app-redis
        chartPath: stable/redis
        version: 8.0.17
        namespace: example-app
        valuesFiles:
        # - ./dev/values.redis.yaml
        setValues:
          image.pullPolicy: IfNotPresent
          cluster.enabled: false
          master.persistence.enabled: false
          usePassword: false
        remote: true

run:
skaffold dev -f skaffold-example.yaml

in another console run:
kubectl -n example-app get po -o wide -w

NAME                                             READY   STATUS    RESTARTS   AGE     IP             NODE                 NOMINATED NODE   READINESS GATES
example-app-mysql-9d69c788-jcc7k                 1/1     Running   0          2m30s   10.244.0.239   kind-control-plane   <none>           <none>
example-app-redis-master-0                       1/1     Running   0          2m11s   10.244.0.240   kind-control-plane   <none>           <none>

then change skaffold-example.yaml
echo >> skaffold-example.yaml

skaffold run this commands:

helm --kube-context kind-kind install --name example-app-mysql --version 1.2.1 stable/mysql --namespace example-app --set persistence.enabled=false

helm --kube-context kind-kind install --name example-app-redis --version 8.0.17 stable/redis --namespace example-app --set cluster.enabled=false --set image.pullPolicy=IfNotPresent --set master.persistence.enabled=false --set usePassword=false

Skaffold start re-deploy and for every release run helm upgrade ... , but all pods do not restart.

If we change the Values of the chart, the embedded chart logic will be executed and most likely the pod of this chart will restart.

---

You can install helm-diff plugin https://github.com/databus23/helm-diff
helm plugin install https://github.com/databus23/helm-diff --version=v3.1.1

and see what changes will occur during deployment:
helm diff upgrade example-app-redis --version 8.0.17 stable/redis --namespace example-app --set cluster.enabled=false --set image.pullPolicy=IfNotPresent --set master.persistence.enabled=false --set usePassword=false

stop skaffold, then if you remove the value "usePassword=false" and restart skaffold again, then we will have changes every time we deploy it

helm diff upgrade example-app-redis --version 8.0.17 stable/redis --namespace example-app --set cluster.enabled=false --set image.pullPolicy=IfNotPresent --set master.persistence.enabled=false
example-app, example-app-redis, Secret (v1) has changed:
  # Source: redis/templates/secret.yaml
  apiVersion: v1
  kind: Secret
  metadata:
    labels:
      app: redis
      chart: redis-8.0.17
      heritage: Tiller
      release: example-app-redis
    name: example-app-redis
  data:
-   redis-password: '-------- # (10 bytes)'
+   redis-password: '++++++++ # (10 bytes)'
  type: Opaque

example-app, example-app-redis-master, StatefulSet (apps) has changed:
  # Source: redis/templates/redis-master-statefulset.yaml
  apiVersion: apps/v1beta2
  kind: StatefulSet
  metadata:
    name: example-app-redis-master
...
        annotations:
          checksum/health: 24e3d787210ece7dea4c0235d0dd3894941547c819c444593763b76063b2c434
          checksum/configmap: cb1a29cb7a3b7d11030cfe6f9831fcc9136f92e8f030b0f2bb2d4ed811dadb45
-         checksum/secret: 4539a5ff46ad90d2eaa4e1eed7ed03ae66cd110439f88a7c6adfd05bed2ab9c3
+         checksum/secret: 6db6270fa9418b363d4b113e4dc4914b5ce36c70c17b789ee4b334b08bdc9853
...

According to the logic of the redis chart, without the value "usePassword=false", the pod will be deployed every time.

For example, with the current mysql chart values. Each deployment also has changes:

helm diff upgrade example-app-mysql --version 1.2.1 stable/mysql --namespace example-app --set persistence.enabled=false
example-app, example-app-mysql, Secret (v1) has changed:
  # Source: mysql/templates/secrets.yaml
  apiVersion: v1
  kind: Secret
  metadata:
    labels:
      app: example-app-mysql
      chart: mysql-1.2.1
      heritage: Tiller
      release: example-app-mysql
    name: example-app-mysql
    namespace: example-app
  data:
-   mysql-password: '-------- # (10 bytes)'
-   mysql-root-password: '-------- # (10 bytes)'
+   mysql-password: '++++++++ # (10 bytes)'
+   mysql-root-password: '++++++++ # (10 bytes)'
  type: Opaque

But by the embedded mysql chart logic, these changes will not make the pod to restart.

As I mentioned before, you do not need to interfere with the built-in logic of the chart, you need to use them correctly or fork the chart to suit your needs.

[theEndBeta]

I ran into this when trying to upgrade a remote chart using skaffold. I updated the version field, and nothing happened, which is not what I was expecting. I had expected that I could just change the skaffold configuration, and have that change reflected.

[yolkov]

@nkubala any update?
Due to this error, we are not updating the skaffold. Although it already has the features we need and fixed bugs in new releases.

[aboyett]

Another user perspective here:

My team provides (company internal) charts with opinionated defaults for a given language framework. Developers who don't want to take on the maintenance overhead of maintaining their chart(s) can reference our remote charts from skaffold.yaml instead. Upgrading from skaffold 1.9.1 to 1.16.0 caused a breaking change as services deployed via skaffold (using remote charts) were no longer updating. As noted by others, adding upgradeOnChange: true to each impacted deployment definition restored expected behavior. We will have to roll this change out to tens of repos as we update each of them to use newer skaffold versions for CI/CD.

The change in default behavior for existing skaffold.yaml files was surprising given the care and discipline used to maintain skaffold.yaml schema versions. Our CI/CD systems lock the skaffold version used but I suspect this has been a source of frustration for several of our developers who installed skaffold using homebrew or linux package managers that track the latest version. It was certainly frustrating for me to track down once I upgraded to a newer version of skaffold.

If the current default behavior is going to remain, would it be reasonable to expect that skaffold fix would inject upgradeOnChange: true into releases using remote charts when upgrading from schema versions prior to v2beta5 (when this option was introduced and the default behavior was changed) to reduce the disruption of existing workflows?

[tejal29]

@yolkov Currently [upgradeOnChange](

skaffold/pkg/skaffold/schema/v2beta5/config.go

Line 554 in 3c7ce0a

// Default is `false` if `remote: true`.

) is set to false for remote helm charts. It would be an easy config change to change it to true in your skaffold.yaml file.
skaffold fix updating to true, might break other use cases like dev. Its always a tough decsion to provide sane defaults for all use cases.

Closing this due to inactivity and no further action needed.

[yolkov]

/reopen

This is not convenient, to prescribe a senseless option in all our services, in all environments for each release we need to add
upgradeOnChange: true, because all ours chart is remote

[yolkov]

@tejal29
For each service, we have from one to 8 helm releases of various charts, this is multiplied by different environments (preprod, staging, testing, prod). Accordingly, we multiply this by the entire number of services. Where for each release we have to add this option, just because someone did not understand how the charts work and committed incorrect default values.