Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

fix: broken upgrade path from previous versions #1346

Merged
merged 5 commits into from
Jan 25, 2021
Merged

fix: broken upgrade path from previous versions #1346

merged 5 commits into from
Jan 25, 2021

Conversation

hectorj2f
Copy link
Contributor

Signed-off-by: Hector Fernandez [email protected]

What this PR does / why we need it:

The upgrade path from v0.5.0 to v0.6.0 was failing due to the following error:

Error: UPGRADE FAILED: cannot patch "kubefed" with kind KubeFedConfig: admission webhook "kubefedconfigs.core.kubefed.io" denied the request: spec.featureGates.name: Unsupported value: "RawResourceStatusCollection":

The running admissionWebhook was not upgraded by the time the new kubefedconfig was updated. Therefore we needed to add certain hooks to the created resources. Once I added those, I managed to upgrade from v0.3.0 --> v0.4.0 --> v0.5.0 --> v0.6.0 without any issue.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1333

Special notes for your reviewer:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 12, 2021
@hectorj2f hectorj2f self-assigned this Jan 12, 2021
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 12, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hectorj2f

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2021
@hectorj2f hectorj2f changed the title fix: broken upgrade path from previous versions [WIP] fix: broken upgrade path from previous versions Jan 12, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 12, 2021
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 12, 2021
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 13, 2021
@hectorj2f hectorj2f changed the title [WIP] fix: broken upgrade path from previous versions fix: broken upgrade path from previous versions Jan 13, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 13, 2021
@hectorj2f
Copy link
Contributor Author

Ping @jimmidyson

@jimmidyson
Copy link
Contributor

@hectorj2f I'm really not confident with this approach. We need an upgrade test to confirm - can you add one please?

@hectorj2f
test: add upgrade test
8a435bc 
Signed-off-by: Hector Fernandez <[email protected]>
@hectorj2f
Copy link
Contributor Author

@jimmidyson I add an initial test to validate the upgrade. Is that enough for you or you are looking for a specific piece of code ?

jimmidyson
jimmidyson reviewed Jan 19, 2021
View reviewed changes
scripts/pre-commit.sh Outdated
@@ -38,6 +38,9 @@ E2E_TEST_CMD="${TEMP_DIR}/e2e-${PLATFORM} ${COMMON_TEST_ARGS}"
# given control plane scope.
IN_MEMORY_E2E_TEST_CMD="go test -v -timeout 900s -race ./test/e2e -args ${COMMON_TEST_ARGS} -in-memory-controllers=true -limited-scope-in-memory-controllers=false"

KUBEFED_UPGRADE_TEST_NS="upgrade-test"
KUBEFED_UPGRADE_TEST_VERSION="v0.5.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

This will have to be bumped for every release, right? Can we update documentation for this or better still automate this, either in the test (retrieve latest release here) or as part of release script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I automated to get the previous version to the latest stable in the remote chart.

@hectorj2f
chore: extract the previous stable version for upgrade
14c62f2 
Signed-off-by: Hector Fernandez <[email protected]>
Signed-off-by: Hector Fernandez <[email protected]>
@hectorj2f
Copy link
Contributor Author

The build failed due to a long job that was stopped during the cleanup step. I simplified the upgrade test to take less time.

@hectorj2f
fix: repo variable name
7fb0814 
Signed-off-by: Hector Fernandez <[email protected]>
@hectorj2f
Copy link
Contributor Author

@makkes @jimmidyson please, take a look. I think I addressed all your comments.

makkes
makkes reviewed Jan 25, 2021
View reviewed changes
scripts/pre-commit.sh
helm repo update

# Get the previous version prior to our latest stable version
KUBEFED_UPGRADE_TEST_VERSION=$(helm search repo kubefed-charts/kubefed --versions | awk '{print $2}' | head -3 | tail -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

For PRs this would currently test an upgrade from 0.5.1 to what's in the PR. However, I believe that for PRs we would like to test upgrades from the latest version (i.e. 0.6.0 atm). wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was testing from the old version 0.5.1 that showed the issue that this PR fixes. Generally, we should run upgrades from the most recent release, but the upgrade from 0.5.1 -> 0.6.0 is broken right now, hence this test.

I guess we should actually remove 0.6.0 once 0.6.1 is released as it's a broken release (at least for upgrades)? Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we should delete v0.6.0 once v0.6.1 is released.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@jimmidyson
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2021
@k8s-ci-robot k8s-ci-robot merged commit 5934e63 into kubernetes-retired:master Jan 25, 2021
@makkes
Copy link
Contributor

makkes commented Apr 13, 2021

closes #1313

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@makkes makkes makkes left review comments

@font font Awaiting requested review from font

@xunpan xunpan Awaiting requested review from xunpan

@jimmidyson jimmidyson Awaiting requested review from jimmidyson

Assignees

@jimmidyson jimmidyson

@hectorj2f hectorj2f

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

How to upgrade kubefed v0.3.0 to v0.6.0
4 participants
@hectorj2f @k8s-ci-robot @jimmidyson @makkes