Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test/k8s: keep configmap across upgrade test #12051

Merged
merged 2 commits into from Jun 15, 2020
Merged

test/k8s: keep configmap across upgrade test #12051

merged 2 commits into from Jun 15, 2020

Conversation

aanm
Copy link
Member

@aanm aanm commented Jun 12, 2020 •
edited

test/k8s: keep configmap across upgrade test

Since we have new options set for new Cilium installations those
might break the connectivity when doing upgrades.

To ideally test an upgrade, we should keep the existing ConfigMap
from the previous the version.

This also fixes the upgrade guide using helm as using --set config.enabled=false deploys Cilium without its configuration map,
deleting the existing one from the cluster.

Also fixes the Cilium DaemonSet since its upgrade does not work using
kubectl apply given that we have introduced a new liveness and readiness
probes in the DaemonSet. Added upgrade notes to keep the old probes
while users perform the upgrades.

Fixes: d613dea ("install/kubernetes: use HTTP for agent {liveness,readiness}Probe")
Signed-off-by: André Martins andre@cilium.io

@aanm aanm added sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/ci This PR makes changes to the CI. needs-backport/1.8 labels Jun 12, 2020
@aanm aanm requested a review from a team as a code owner June 12, 2020 14:32
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Jun 12, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.0 Jun 12, 2020
@aanm
Copy link
Member Author

aanm commented Jun 12, 2020

test-me-please

@aanm aanm mentioned this pull request Jun 12, 2020
Merged
@coveralls
Copy link

coveralls commented Jun 12, 2020 •
edited

Coverage Status

Coverage increased (+0.005%) to 37.032% when pulling 07ea918 on pr/fix-upgrade-config-map into d0a0898 on master.

joestringer
joestringer approved these changes Jun 12, 2020
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's another option which is --reuse-values which I think we should start encouraging more widely including in the upgrade docs.

But this actually matches our upgrade docs so 👌

@aanm
Copy link
Member Author

aanm commented Jun 12, 2020

test-missed-k8s

@aanm aanm force-pushed the pr/fix-upgrade-config-map branch from 0a6cd59 to 11ca347 Compare June 12, 2020 17:43
@aanm aanm requested review from a team as code owners June 12, 2020 17:43
@aanm aanm requested a review from a team June 12, 2020 17:43
@aanm
Copy link
Member Author

aanm commented Jun 12, 2020

test-focus K8sUpdates

@aanm aanm force-pushed the pr/fix-upgrade-config-map branch from 11ca347 to e9ca3b7 Compare June 12, 2020 17:44
@aanm
Copy link
Member Author

aanm commented Jun 12, 2020

test-focus K8sUpdates

@aanm aanm added needs-backport/1.7 priority/high This is considered vital to an upcoming release. labels Jun 12, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.6 Jun 12, 2020
joestringer
joestringer approved these changes Jun 12, 2020
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good enough to fix the upgrade for v1.7 and v1.8 branches, then during v1.9 cycle we can take a long hard look at our Helm charts & upgrade instructions to figure out the long-term solution here.

@aanm aanm force-pushed the pr/fix-upgrade-config-map branch from e9ca3b7 to ef3a921 Compare June 12, 2020 19:34
@aanm
Copy link
Member Author

aanm commented Jun 12, 2020

test-focus K8sUpdates

1 similar comment
@aanm
Copy link
Member Author

aanm commented Jun 12, 2020

test-focus K8sUpdates

joestringer
joestringer requested changes Jun 12, 2020
View reviewed changes
Documentation/install/upgrade.rst
Comment on lines 221 to +223
Deployments using the `ConfigMap` which is not generated if
``config.enabled=false`` is set. The above command *only* generates the
DaemonSet, Deployment and RBAC definitions.
``config.enabled=false`` or ``config.keepCurrent=true`` are set. The above
command *only* generates the DaemonSet, Deployment and RBAC definitions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we need to drop this hunk.

@aanm aanm added the kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. label Jun 12, 2020
@aanm aanm force-pushed the pr/fix-upgrade-config-map branch from ef3a921 to 5ef27cc Compare June 12, 2020 21:18
@aanm aanm requested a review from a team as a code owner June 13, 2020 00:35
@joestringer
Copy link
Member

test-focus K8sUpdates

@joestringer
Copy link
Member

test-focus K8sUpdates

@joestringer
Copy link
Member

test-focus K8sUpdates

@aanm
Copy link
Member Author

aanm commented Jun 13, 2020

test-me-please

joestringer
joestringer approved these changes Jun 13, 2020
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment that doesn't need to block merging / backporting / -rc3.

Documentation/install/upgrade.rst Outdated
Comment on lines 171 to 173
Deploy Cilium release via Helm (Running this command will overwrite the
existing cluster's `ConfigMap` which might not be ideal if you want to keep
your existing `ConfigMap`.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For micro releases, we should just use --reuse-values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, actually AFAIU since we don't specify any --set, --reuse-values is the default.

tgraf
tgraf approved these changes Jun 14, 2020
View reviewed changes
Copy link
Member

@tgraf tgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix for probe looks great. I have a question remaining for option A in the upgrade path.

Documentation/install/upgrade.rst Outdated
Deploy Cilium release via Helm:
Deploy Cilium release via Helm (Running this command will overwrite the
existing cluster's `ConfigMap` which might not be ideal if you want to keep
your existing `ConfigMap`.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need an overall warning that this will possibly break compatibility as we will use the new helm defaults for upgrades? So far we have been assuming that the ConfigMap values would remain unchanged. The above helm template will overwrite it as far as I understand. Our backwards compatibility guarantee has been around not changing the defaults in the binary but default breaking changes to the new value in the ConfigMap so new deployments get the good new values without breaking existing clusters?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need an overall warning that this will possibly break compatibility as we will use the new helm defaults for upgrades?

yes. I think we need to take an overall look at this section since we use helm upgrade in a couple places.

So far we have been assuming that the ConfigMap values would remain unchanged. The above helm template will overwrite it as far as I understand

good point, I've added the warning in the notes section.

Our backwards compatibility guarantee has been around not changing the defaults in the binary but default breaking changes to the new value in the ConfigMap so new deployments get the good new values without breaking existing clusters?

If I understand you correctly, yes that is supposed to be our guarantee.

aanm added 2 commits June 14, 2020 10:49
@aanm
test/k8s: keep configmap across upgrade test
0da557f 
Since we have new options set for new Cilium installations those
might break the connectivity when doing upgrades.

To ideally test an upgrade, we should keep the existing ConfigMap
from the previous the version.

This also fixes the upgrade guide using helm as using `--set
config.enabled=false` deploys Cilium without its configuration map,
deleting the existing one from the cluster.

Also fixes the Cilium DaemonSet since its upgrade does not work using
kubectl apply given that we have introduced a new liveness and readiness
probes in the DaemonSet. Added upgrade notes to keep the old probes
while users perform the upgrades.

Fixes: d613dea ("install/kubernetes: use HTTP for agent {liveness,readiness}Probe")
Signed-off-by: André Martins <andre@cilium.io>
@aanm
operator: Fix IPAM default flag
07ea918 
Unless the user explicitly specifies the IPAM in use, fall back to the
default cilium-operator image and use the legacy host-scope IPAM to
ensure that during upgrade from a previous release that Cilium is not
automatically migrated to the new IPAM model; that should be an explicit
opt-in step that the user performs subsequently.

Signed-off-by: André Martins <andre@cilium.io>
Signed-off-by: Joe Stringer <joe@cilium.io>
@aanm aanm force-pushed the pr/fix-upgrade-config-map branch from be76441 to 07ea918 Compare June 14, 2020 08:50
@aanm
Copy link
Member Author

aanm commented Jun 14, 2020

no need to re-rerun the CI since all tests have passed before pushing again to fix some typos.

@aanm aanm merged commit 773363a into master Jun 15, 2020
1.8.0 automation moved this from In progress to Merged Jun 15, 2020
@aanm aanm deleted the pr/fix-upgrade-config-map branch June 15, 2020 07:48
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.0 Jun 15, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.0 Jun 16, 2020
@christarazi christarazi mentioned this pull request Jun 30, 2020
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.6 Jun 30, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Backport pending to v1.7 in 1.7.6 Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@tgraf tgraf tgraf approved these changes

@christarazi christarazi christarazi approved these changes

Assignees
No one assigned
Labels
kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. priority/high This is considered vital to an upcoming release. release-note/ci This PR makes changes to the CI. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
No open projects
1.8.0
  
Merged
1.8.0
Backport done to v1.8
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@aanm @coveralls @joestringer @tgraf @christarazi