Page MenuHomePhabricator

Fix calico, cfssl-issuer and knative-serving Helm dependencies
Closed, ResolvedPublic

Description

We currently have a nice typo, dependecies (missing n) in various Helm charts: knative-serving, calico and cfssl-issuer

I tried to fix them via https://gerrit.wikimedia.org/r/c/operations/deployment-charts/+/768800, but the CI and helmfile diff look like all the CRDs of the related -crds charts are going to be re-added. We are currently relying on the dependency between the charts brought by hemfile, so we may not need to have an explicit Helm dependency as well.

The above change was reverted in https://gerrit.wikimedia.org/r/c/operations/deployment-charts/+/768800 to figure out what's best to do.

Event Timeline

Change 769062 had a related patch set uploaded (by Elukey; author: Elukey):

[operations/deployment-charts@master] calico,cfssl-issuer,knative-serving: bump chart's version

https://gerrit.wikimedia.org/r/769062

Change 769062 merged by Elukey:

[operations/deployment-charts@master] calico,cfssl-issuer,knative-serving: bump chart's version

https://gerrit.wikimedia.org/r/769062

Change 826810 had a related patch set uploaded (by JMeybohm; author: JMeybohm):

[operations/deployment-charts@master] Update calico to v3.23.3

https://gerrit.wikimedia.org/r/826810

Fixed for calico with v3.23.3

Change 826810 merged by jenkins-bot:

[operations/deployment-charts@master] Update calico to v3.23.3

https://gerrit.wikimedia.org/r/826810

Fixed for cfss-issuer in chart version 0.3.0

Change 861399 had a related patch set uploaded (by Elukey; author: Elukey):

[operations/deployment-charts@master] knative-serving: improve chart's dependencies

https://gerrit.wikimedia.org/r/861399

When deploying the updated calico chart to my test cluster I realized that the fixed dependency to the CRD chart does not mean that chart is installed (e.g. CRDs available) before the main chart is installed (obviously the deployment failed). In helmfile.d we deploy the CRD chart as a release before deploying the main chart. This unfortunately does no longer work as helm detects the CRDs already being present (from a different release) and fails to deploy.

This is very in line with https://helm.sh/docs/chart_best_practices/custom_resource_definitions/ and it seems there is no proper way out. Including the CRDs as non-templated files inin the crd/ folder of the main chart is not a real option as there is no support for updating those CRDs.

Speaking with @elukey about this we decided that the easiest way forward is probably to stick to not declaring the dependency to CRD charts in the main charts (which is basically what we did with the typo) and maintaining their relationship via helmfile (see https://gerrit.wikimedia.org/r/c/operations/deployment-charts/+/861399).

I'll undo/unfix the cfssl and calico charts.

CC @BTullis & @bking: This might be relevant for operators as well.

Thanks @JMeybohm - I'll definitely bear that in mind.

From my work so far with the spark-operator it seems that the operator itself deploys the CRDs required for subsequent creation of SparkApplication and ScheduledSparkApplication resources.
As in, none of these objects are created at the time of the helm chart being deployed.

I'll keep my eyes open to any problems of this type though and let you know.

Change 861399 merged by Elukey:

[operations/deployment-charts@master] knative-serving: improve chart's dependencies

https://gerrit.wikimedia.org/r/861399

Change 867529 had a related patch set uploaded (by JMeybohm; author: JMeybohm):

[operations/deployment-charts@master] calico, cfssl-issuer: Remove chart defined dependencies

https://gerrit.wikimedia.org/r/867529

JMeybohm claimed this task.

Change 867529 merged by jenkins-bot:

[operations/deployment-charts@master] calico, cfssl-issuer: Remove chart defined dependencies

https://gerrit.wikimedia.org/r/867529

Change 867600 had a related patch set uploaded (by Elukey; author: Elukey):

[operations/deployment-charts@master] kserve-inference: fix dependencies in Chart.yaml

https://gerrit.wikimedia.org/r/867600

Change 867600 merged by Elukey:

[operations/deployment-charts@master] kserve-inference: fix dependencies in Chart.yaml

https://gerrit.wikimedia.org/r/867600