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

🌱 E2E test to upgrade workload cluster #4130

Merged
merged 1 commit into from
Mar 24, 2021
Merged

🌱 E2E test to upgrade workload cluster #4130

merged 1 commit into from
Mar 24, 2021

Conversation

srm09
Copy link
Contributor

@srm09 srm09 commented Jan 28, 2021 •
edited
Loading

What this PR does / why we need it:
Adds a new E2E test to upgrade a workload cluster and run conformance tests to verify the upgrade

Which issue(s) this PR fixes:
Fixes #4043

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 28, 2021
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 28, 2021
@srm09
Copy link
Contributor Author

srm09 commented Jan 28, 2021

/hold WIP

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2021
neolit123
neolit123 reviewed Jan 28, 2021
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

thanks for the PR, added some quick comments on styling / wording.

test/e2e/cluster_upgrade.go Outdated Show resolved Hide resolved
test/e2e/cluster_upgrade.go Outdated Show resolved Hide resolved
test/e2e/cluster_upgrade.go Outdated Show resolved Hide resolved
test/e2e/cluster_upgrade_test.go Outdated Show resolved Hide resolved
test/e2e/cluster_upgrade.go Outdated Show resolved Hide resolved
test/e2e/cluster_upgrade.go Outdated Show resolved Hide resolved
test/e2e/cluster_upgrade.go Outdated Show resolved Hide resolved
test/e2e/cluster_upgrade.go Outdated Show resolved Hide resolved
@srm09
Copy link
Contributor Author

srm09 commented Feb 1, 2021

/test pull-cluster-api-e2e-full-main

@srm09
Copy link
Contributor Author

srm09 commented Feb 1, 2021

/test pull-cluster-api-e2e-full-main

@srm09
Copy link
Contributor Author

srm09 commented Feb 1, 2021

cc @fabriziopandini PTAL, working on a prow job to run this with different combinations of versions.

@vincepri
Copy link
Member

vincepri commented Feb 3, 2021

/assign @fabriziopandini

@fabriziopandini fabriziopandini mentioned this pull request Feb 4, 2021
Merged
fabriziopandini
fabriziopandini reviewed Feb 4, 2021
View reviewed changes
test/e2e/Makefile Outdated Show resolved Hide resolved
test/e2e/cluster_upgrade_test.go Outdated
. "github.com/onsi/ginkgo"
)

var _ = Describe("When upgrading a workload cluster and testing K8S conformance [K8SVersion-Upgrade]", func() {
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to document labels ([K8SVersion-Upgrade]) in https://cluster-api.sigs.k8s.io/developer/testing.html#running-the-end-to-end-tests and cross link the list of labels in https://cluster-api.sigs.k8s.io/developer/e2e.html

Also, are you planning to sunset the [Periodic-K8SVersion] label?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL

wfernandes
wfernandes reviewed Feb 4, 2021
View reviewed changes
Copy link
Contributor

@wfernandes wfernandes left a comment

Choose a reason for hiding this comment

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

I'm a +1 on @fabriziopandini's suggestions regarding documenting the test labels and perhaps reusing [Periodic-K8SVersion] if it follows the intent.

@srm09 Is there a reason we still have the /hold on this?

@srm09
Copy link
Contributor Author

srm09 commented Feb 5, 2021

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 5, 2021
wfernandes
wfernandes reviewed Feb 5, 2021
View reviewed changes
Copy link
Contributor

@wfernandes wfernandes left a comment

Choose a reason for hiding this comment

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

I'm assuming there's a corresponding test-infra PR that updates our CI config so that tests are run without disruption. If there is one, can we link it here 🙂

test/e2e/machine_pool_test.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2021
@srm09 srm09 force-pushed the e2e-test-for-upgrades branch 3 times, most recently from cde84d2 to 6a2854c Compare February 5, 2021 23:29
@fabriziopandini
Copy link
Member

/lgtm
@srm09 thanks for driving this effort!
NB. let's make sure to merge this and kubernetes/test-infra#20720 ~at the same time to not block PRs or periodic jobs

@srm09
Copy link
Contributor Author

srm09 commented Feb 24, 2021

/test pull-cluster-api-e2e-full-main

@srm09
Copy link
Contributor Author

srm09 commented Feb 24, 2021

/test pull-cluster-api-test-main
/test pull-cluster-api-e2e-full-main

@fabriziopandini
Copy link
Member

/retest

@srm09
Copy link
Contributor Author

srm09 commented Mar 1, 2021

/test pull-cluster-api-e2e-full-main

@srm09
Copy link
Contributor Author

srm09 commented Mar 2, 2021

@fabriziopandini Am unsure as to why this is failing. I have had a clean re-run for this locally multiple times.

@vincepri
Copy link
Member

vincepri commented Mar 2, 2021

/retest

@fabriziopandini
Copy link
Member

/test pull-cluster-api-verify

@fabriziopandini
Copy link
Member

@srm09 given the type of error in make verify my assumption is that you are using an older version of go locally.
Please update to go 1.16, run make modules, then you should be ok

@fabriziopandini
Copy link
Member

/milestone v0.4.0

@k8s-ci-robot k8s-ci-robot added this to the v0.4.0 milestone Mar 5, 2021
@srm09
Copy link
Contributor Author

srm09 commented Mar 8, 2021

/test pull-cluster-api-e2e-full-main

@fabriziopandini
Copy link
Member

/lgtm
@srm09 I'm ok to move forward with this. Could you kindly squash commits in kubernetes/test-infra#20720 so we can get the two PR merge ~at the same time?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2021
@srm09
Copy link
Contributor Author

srm09 commented Mar 8, 2021

@fabriziopandini Done

@srm09 @neolit123 @fabriziopandini
E2E test to upgrade workload cluster
3f7430d 
This patch adds an E2E test to create a workload cluster containing of
machinedeployment and machinepool. This cluster is then upgraded to
another k8s version, including the CP, machine pool and machine
deployment.
To verify the status of the upgrade, conformance tests are run on the
cluster post upgrade.

This also updates the e2e run target to include an additional parameter
named GINKGO_SKIP which can be used to skip ginkgo tests matching the
regexp.

Signed-off-by: Sagar Muchhal <muchhals@vmware.com>
Co-authored-by: Cecile Robert-Michon <cerobert@microsoft.com>
Co-authored-by: Lubomir I. Ivanov <neolit123@gmail.com>
Co-authored-by: Fabrizio Pandini <fabrizio.pandini@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 24, 2021
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Mar 24, 2021 •
edited
Loading

@srm09: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-cluster-api-e2e-full-main 08b3b8b link /test pull-cluster-api-e2e-full-main

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@srm09
Copy link
Contributor Author

srm09 commented Mar 24, 2021

/test pull-cluster-api-test-main

@fabriziopandini
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 24, 2021
@fabriziopandini
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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 Mar 24, 2021
@k8s-ci-robot k8s-ci-robot merged commit 1671cbf into kubernetes-sigs:master Mar 24, 2021
@srm09 srm09 deleted the e2e-test-for-upgrades branch March 24, 2021 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 left review comments

@wfernandes wfernandes wfernandes left review comments

@fabriziopandini fabriziopandini fabriziopandini left review comments

@CecileRobertMichon CecileRobertMichon CecileRobertMichon left review comments

@detiber detiber Awaiting requested review from detiber

@vincepri vincepri Awaiting requested review from vincepri

Assignees

@fabriziopandini fabriziopandini

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 "Looks good to me", 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
v0.4
Development

Successfully merging this pull request may close these issues.

Add E2E test to verify workload cluster upgrades
7 participants
@srm09 @vincepri @fabriziopandini @CecileRobertMichon @k8s-ci-robot @neolit123 @wfernandes