cleanup helm chart by dungdm93 · Pull Request #16896 · cilium/cilium

dungdm93 · 2021-07-15T11:38:42Z

This is next part of refactoring helm chart #16792 with the following changes:

remove .Values.Capabilities in favor of --kube-version in helm template, as discussion in #16752
Unify image using cilium.image template.

cilium/install/kubernetes/cilium/templates/cilium-agent/daemonset.yaml

Line 226 in c95f6fd

image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}{{ if .Values.image.useDigest }}@{{ .Values.image.digest }}{{ end }}"

Currently, configuring image is quite long, and not consistent between components (some support digest, some are not). By using cilium.image template, it's make configuring image shorter, easier to read and consistent between components.
support priorityClassName.
Currently, some components (e.g agent) support define priorityClassName in values file, but not using in template :|, other components (e.g hubble-ui, hublle-relay) are not support priorityClassName. So, with this MR, priorityClassName is supported in all components.
move hubble-metrics service & hubble ServiceMonitor to seperated file (in hubble folder)
remove unnecessary parentheses
make name as the first attribute of object
using nindent instead of indent
using default and ternary instead of if-else
using with instead of if when it's possible

Signed-off-by: Đặng Minh Dũng dungdm93@live.com

install/kubernetes/cilium/templates/_helpers.tpl

errordeveloper

Thanks very much, overall this looks good to me!

gandro

remove redundant namespace: {{ .Release.Namespace }}

I mentioned it in the other PR, but this is unfortunately not redundant. This is needed for correct use of helm template --namespace, see helm/helm#3553 (comment)

We are relying on the fact that one can kubectl create the generated YAML without an explicit -n in the docs, so this will break existing workflows.

dungdm93 · 2021-07-20T04:17:15Z

Thank @gandro for clearify. namespace is back

gandro

From a first glance, this looks correct now, thank you! It will require some manual testing to ensure there are no significant differences in the output.

One thing we need to document (because it's a breaking change) is the removal of the Capabilities values. Please add an upgrade note here:
https://github.com/cilium/cilium/blob/master/Documentation/operations/upgrade.rst#111-upgrade-notes

It could be something like:

The Capabilities Helm value has been removed. When using helm template to generate the Kubernetes manifest for a specific Kubernetes version, please use the --kube-version flag (introduced in Helm 3.6.0) instead.

dungdm93 · 2021-07-21T03:41:23Z

v1.11 Upgrade Notes added

gandro

Starting to look good. A few minor things left that I noticed while comparing the helm template output

install/kubernetes/cilium/templates/cilium-agent/daemonset.yaml

install/kubernetes/cilium/templates/clustermesh-apiserver/deployment.yaml

install/kubernetes/cilium/templates/_helpers.tpl

qmonnet

Looks good overall, although my knowledge of the Helm chart is limited and I'm not able to provide an in-depth review of the changes.

Here are a few nitpicks below. In addition,

Please make sure to update the Helm reference in the Documentation (see comment below),
And please update your commit log message regarding the remove redundant namespace: {{ .Release.Namespace }}, given that, if I understand the discussion with Sebastian, this is in fact not the case.

install/kubernetes/cilium/templates/cilium-agent/clusterrole.yaml

install/kubernetes/cilium/templates/cilium-preflight/clusterrole.yaml

install/kubernetes/cilium/values.yaml

install/kubernetes/cilium/templates/cilium-agent/daemonset.yaml

gandro · 2021-07-22T12:01:40Z

Travis on ARM failed with a transient error, should not block this PR: https://app.travis-ci.com/github/cilium/cilium/jobs/526175793

# Pre-pull FROM docker images due to Buildkit sometimes failing to pull them.

grep "^FROM " Dockerfile | cut -d ' ' -f2 | xargs -n1 docker pull

Error response from daemon: Get https://registry-1.docker.io/v2/library/python/manifests/3.7.9-alpine3.11: Get https://auth.docker.io/token?scope=repository%3Alibrary%2Fpython%3Apull&service=registry.docker.io: EOF

gandro · 2021-07-22T12:01:52Z

test-me-please

christarazi

LGTM. Could you squash Update some typo into the first commit? It fits the description of the first commit anyway.

Looks like you'll also need to rebase on master.

christarazi

A few more git history related comments.

christarazi · 2021-07-23T04:32:21Z

install/kubernetes/cilium/templates/cilium-operator/deployment.yaml

This change is supposed to be part of the 1st commit but is part of the 3rd?

@christarazi The 1st commit also contains priorityClassName changes, so I decided to squash 3rd commit into 1st commit.
I also update 1st commit message to match w/ this PR description.

christarazi · 2021-07-23T04:32:30Z

install/kubernetes/cilium/templates/cilium-preflight/daemonset.yaml

This change is supposed to be part of the 1st commit but is part of the 3rd?

christarazi · 2021-07-23T04:32:41Z

install/kubernetes/cilium/templates/etcd-operator/cilium-etcd-operator-deployment.yaml

This change is supposed to be part of the 1st commit but is part of the 3rd?

aanm

Thank you for this amazing PR. Two comments:

Is there any linter that you used to catch all of these inconsistencies?
Just to confirm, all changes from #16933 seem to be included in this PR, correct?

dungdm93 · 2021-07-23T14:40:52Z

@aanm

Nope, I check all of this with my eye :) Honestly, it took me almost 2 weeks before I create this PR
Yeah, all changes from install/kubernetes: add priorityClasses #16933 are coverd in this PR

aanm · 2021-07-23T17:05:13Z

test-me-please

christarazi

🚀

* remove .Values.Capabilities in favor of --kube-version in helm template * using `cilium.image` template to unify image render * support `priorityClassName` in all components * move `hubble-metrics` service & `hubble` ServiceMonitor to seperated file (in hubble folder) * make `name` as the first attribute of object * remove unnecessary parentheses * using `nindent` instead of `indent` * using `default` and `ternary` instead of `if-else` * using `with` instead of `if` when it's possible Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>

Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>

dungdm93 · 2021-07-25T12:57:02Z

@christarazi I rebased from master. But seem needs-rebase tag is not auto remove

qmonnet

Not sure why you need another review from me, but it still looks good.

dungdm93 · 2021-07-27T05:21:07Z

@qmonnet sorry, I click to request review from @errordeveloper but accident click into your name :)

qmonnet · 2021-07-27T08:05:45Z

test-me-please

qmonnet · 2021-07-27T08:15:26Z

Ilya is currently out of office, given that his feedback has been addressed I think we can take his earlier comment as an approval. I'm unassigning him.

qmonnet · 2021-07-27T13:52:46Z

Considering Ilya's implicit approval (see above) and that all tests have passed, I'm labelling as ready-to-merge.

dungdm93 requested a review from a team as a code owner July 15, 2021 11:38

dungdm93 requested review from a team and errordeveloper July 15, 2021 11:38

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 15, 2021

maintainer-s-little-helper bot assigned errordeveloper Jul 15, 2021

sayboras added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jul 15, 2021

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 15, 2021

sayboras added area/helm Impacts helm charts and user deployment experience dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jul 15, 2021

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 15, 2021

sayboras added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. kind/cleanup This includes no functional changes. labels Jul 15, 2021

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 15, 2021

errordeveloper reviewed Jul 19, 2021

View reviewed changes

install/kubernetes/cilium/templates/_helpers.tpl Outdated Show resolved Hide resolved

errordeveloper reviewed Jul 19, 2021

View reviewed changes

gandro requested changes Jul 19, 2021

View reviewed changes

dungdm93 requested a review from gandro July 20, 2021 04:17

maintainer-s-little-helper bot assigned gandro Jul 20, 2021

gandro reviewed Jul 20, 2021

View reviewed changes

dungdm93 requested a review from a team as a code owner July 21, 2021 03:40

dungdm93 requested a review from qmonnet July 21, 2021 03:40

maintainer-s-little-helper bot assigned qmonnet Jul 21, 2021

dungdm93 requested a review from gandro July 21, 2021 03:41

gandro reviewed Jul 21, 2021

View reviewed changes

qmonnet added the upgrade-impact This PR has potential upgrade or downgrade impact. label Jul 21, 2021

qmonnet requested changes Jul 21, 2021

View reviewed changes

gandro mentioned this pull request Jul 21, 2021

install/kubernetes: add priorityClasses #16933

Closed

christarazi reviewed Jul 22, 2021

View reviewed changes

christarazi added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jul 22, 2021

christarazi reviewed Jul 23, 2021

View reviewed changes

dungdm93 requested a review from christarazi July 23, 2021 11:54

aanm approved these changes Jul 23, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned aanm Jul 23, 2021

christarazi approved these changes Jul 23, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned christarazi Jul 23, 2021

dungdm93 added 2 commits July 25, 2021 19:34

update v1.11 Upgrade Notes

ce3e76d

Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>

christarazi removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jul 26, 2021

dungdm93 requested review from errordeveloper and qmonnet July 26, 2021 07:56

maintainer-s-little-helper bot assigned qmonnet Jul 26, 2021

qmonnet approved these changes Jul 26, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned qmonnet Jul 26, 2021

qmonnet unassigned errordeveloper Jul 27, 2021

qmonnet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 27, 2021

brb merged commit b22ba5a into cilium:master Jul 28, 2021

gandro mentioned this pull request Jul 28, 2021

refactor cert-gen logic #16900

Merged

andy176631 mentioned this pull request Jan 17, 2026

cilium-dbg: Add preflight checker for cilium-configmap #43025

Merged

Conversation

dungdm93 commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

errordeveloper left a comment

Choose a reason for hiding this comment

Uh oh!

gandro left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dungdm93 commented Jul 20, 2021

Uh oh!

gandro left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dungdm93 commented Jul 21, 2021

Uh oh!

gandro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qmonnet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gandro commented Jul 22, 2021

Uh oh!

gandro commented Jul 22, 2021

Uh oh!

christarazi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christarazi left a comment

Choose a reason for hiding this comment

Uh oh!

christarazi Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

dungdm93 Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christarazi Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

christarazi Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

aanm left a comment

Choose a reason for hiding this comment

Uh oh!

dungdm93 commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aanm commented Jul 23, 2021

Uh oh!

christarazi left a comment

Choose a reason for hiding this comment

Uh oh!

dungdm93 commented Jul 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qmonnet left a comment

Choose a reason for hiding this comment

Uh oh!

dungdm93 commented Jul 27, 2021

Uh oh!

qmonnet commented Jul 27, 2021

Uh oh!

qmonnet commented Jul 27, 2021

dungdm93 commented Jul 15, 2021 •

edited

Loading

gandro left a comment •

edited

Loading

gandro left a comment •

edited

Loading

christarazi left a comment •

edited

Loading

dungdm93 Jul 23, 2021 •

edited

Loading

dungdm93 commented Jul 23, 2021 •

edited

Loading

dungdm93 commented Jul 25, 2021 •

edited

Loading