🌱 Implement e2e test for clusterctl upgrade by fabriziopandini · Pull Request #3708 · kubernetes-sigs/cluster-api

fabriziopandini · 2020-09-29T12:00:08Z

What this PR does / why we need it:
This PR implements a new E2E test providing a signal on clusterctl upgrades

init a management cluster with providers in version X
create a workload cluster
upgrade providers to version Y using clusterctl upgrade
check everything works fine (e.g. add a machine to the workload cluster)
cleanup

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

fabriziopandini · 2020-09-29T12:02:22Z

/area testing
/hold
for v0.3.10 to be released because it assumes docker components to be available in the release artifacts

/milestone v0.4.0
(this could go in v0.3.11 as well)

fabriziopandini · 2020-10-06T12:18:18Z

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

fabriziopandini · 2020-10-13T11:55:32Z

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

fabriziopandini · 2020-10-13T14:12:59Z

/retest

fabriziopandini · 2020-10-14T09:06:28Z

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

fabriziopandini · 2020-11-19T11:24:24Z

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

fabriziopandini · 2020-11-19T11:24:58Z

@wfernandes @srm09 if you have some spare time PTAL

fabriziopandini · 2020-11-19T12:49:14Z

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

wfernandes

I tried to run the test on my system and saw the following error:

  Should create a management cluster and then upgrade all the providers [It]
  /Users/wfernandes/workspace/cluster-api/test/e2e/clusterctl_upgrade.go:79

  Failed to run clusterctl config cluster
  Unexpected error:
      <*yamlprocessor.errMissingVariables | 0xc000ac6200>: {
          Missing: ["CNI_RESOURCES"],
      }
      value for variables [CNI_RESOURCES] is not set. Please set the value using os environment variables or the clusterctl config file
  occurred

Do I need to configure some variables for this E2E test?

fabriziopandini · 2020-11-20T13:59:36Z

@wfernandes the error was due to a conflicting merge with #3896 recently merged; now I have rebased, so everything should be fine

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

fabriziopandini · 2020-11-20T17:22:09Z

The test error seems unrelated to this PR...

wfernandes · 2020-11-20T19:44:35Z

For some reason when I run GINKGO_FOCUS="clusterctl upgrades" make test-e2e, it fails in the following step

STEP: Waiting for one control plane node to exist
When testing clusterctl upgrades
/Users/wfernandes/workspace/cluster-api/test/e2e/clusterctl_upgrade_test.go:27
  Should create a management cluster and then upgrade all the providers [It]
  /Users/wfernandes/workspace/cluster-api/test/e2e/clusterctl_upgrade.go:79

  Timed out after 600.000s.
  Expected
      <bool>: false
  to be true

  /Users/wfernandes/workspace/cluster-api/test/framework/controlplane_helpers.go:143

I'll try and investigate some more but it's not immediately clear as to what is happening. Any thoughts @fabriziopandini ?

fabriziopandini · 2021-04-27T12:39:38Z

/test pull-cluster-api-e2e-main

sbueringer

Overall the approach looks good to me.

sbueringer · 2021-04-28T19:33:01Z

@fabriziopandini Should I do another round of review or wait until the test is green or you ping again?

fabriziopandini · 2021-05-05T20:33:46Z

 func (p *providerComponents) DeleteWebhookNamespace() error {
 	log := logf.Log
-	log.V(5).Info("Deleting %s namespace", repository.WebhookNamespaceName)
+	log.V(5).Info("Deleting", "namespace", repository.WebhookNamespaceName)


This is an error in clusterctl detected by this test

fabriziopandini · 2021-05-05T20:34:39Z

-      targetPort: 9443
-  selector:
-    control-plane: controller-manager
+      targetPort: webhook-server


This is an inconsistency in CAPD vs other providers

Can you provide some more context what the inconsistency is, why we have it and why we are dropping the selector now?

This is mostly for consistency across all the providers in the CAPI codebase, eg.

All the provider are using the named port, CAPD was using the port number

All the provider are not using a selector (they rely on the labels added by kustomize), CAPD had the selector

Ah got it, so we're consistent now after this change. Wasn't sure ;).

fabriziopandini · 2021-05-05T20:37:59Z

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

@vincepri @sbueringer this test already helped in finding a controller runtime defect and a clusterctl error; the job passed when running alone, I'm now testing with e2e-full, but we should now be ready to go

sbueringer · 2021-05-05T21:45:26Z

@fabriziopandini Great! I put it on my review list

sbueringer

Mostly nits. I think relative to the complexity of the test it reads very nicely! :)

sbueringer · 2021-05-07T19:24:28Z

 }

+// InitWithBinary uses clusterctl binary to run init with the list of providers defined in the local repository.
+func InitWithBinary(_ context.Context, binary string, input InitInput) {


I'm not sure what the idea behind the context param here (and the other funcs in this file is). Maybe just hand it over in case we need it later on?

I imagine at least in the funcs with binary we will probably never use it?

The idea is to have some context with timeout percolating down the code, and potentially down to the exec command as well...

sbueringer · 2021-05-07T19:39:30Z

-      targetPort: 9443
-  selector:
-    control-plane: controller-manager
+      targetPort: webhook-server


Can you provide some more context what the inconsistency is, why we have it and why we are dropping the selector now?

k8s-ci-robot · 2021-05-10T13:27:03Z

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

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff-main	`2a6e55d`	link	`/test pull-cluster-api-apidiff-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.

Details

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.

sbueringer · 2021-05-10T18:29:28Z

/lgtm

Just to confirm. No changes in test-infra required as this test will just run with our other tests in pull-cluster-api-e2e-full-main?

fabriziopandini · 2021-05-10T18:57:36Z

Just to confirm. No changes in test-infra required as this test will just run with our other tests in pull-cluster-api-e2e-full-main?

Yes!
Eventually, if the test proves to be stable we will consider if to move it into the [PR blocking] group, but not for now.

vincepri

/approve

k8s-ci-robot · 2021-05-12T18:34:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [vincepri]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 29, 2020

k8s-ci-robot requested review from JoelSpeed and vincepri September 29, 2020 12:00

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 29, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2020

k8s-ci-robot added this to the v0.4.0 milestone Sep 29, 2020

k8s-ci-robot added the area/testing Issues or PRs related to testing label Sep 29, 2020

fabriziopandini force-pushed the clusterctl-upgrade-e2e-test branch from 4fd507a to 134b7af Compare October 6, 2020 12:17

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2020

fabriziopandini force-pushed the clusterctl-upgrade-e2e-test branch from 134b7af to 9c288cb Compare October 6, 2020 12:28

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2020

fabriziopandini force-pushed the clusterctl-upgrade-e2e-test branch from 9c288cb to 1dbc0aa Compare October 13, 2020 11:55

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2020

fabriziopandini force-pushed the clusterctl-upgrade-e2e-test branch from 1dbc0aa to 6100c5a Compare November 19, 2020 11:24

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2020

fabriziopandini force-pushed the clusterctl-upgrade-e2e-test branch from 6100c5a to 66d342e Compare November 19, 2020 12:48

wfernandes reviewed Nov 19, 2020

View reviewed changes

Comment thread test/framework/clusterctl/e2e_config.go Outdated

fabriziopandini force-pushed the clusterctl-upgrade-e2e-test branch from 66d342e to 6fc8b94 Compare November 20, 2020 13:45

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 26, 2021

fabriziopandini force-pushed the clusterctl-upgrade-e2e-test branch from d12ba1e to e1a622f Compare April 27, 2021 12:20

fabriziopandini force-pushed the clusterctl-upgrade-e2e-test branch from e1a622f to 706caea Compare April 27, 2021 13:35

sbueringer reviewed Apr 27, 2021

View reviewed changes

fabriziopandini force-pushed the clusterctl-upgrade-e2e-test branch from 8b19078 to 4bf0c08 Compare May 5, 2021 20:11

add clusterctl upgrades E2E tests

bb2fbfe

fabriziopandini force-pushed the clusterctl-upgrade-e2e-test branch from 4bf0c08 to bb2fbfe Compare May 5, 2021 20:31

fabriziopandini commented May 5, 2021

View reviewed changes

fabriziopandini changed the title ~~[WIP] 🌱 Implement e2e test for clusterctl upgrade~~ 🌱 Implement e2e test for clusterctl upgrade May 5, 2021

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 May 5, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 5, 2021

sedefsavas mentioned this pull request May 6, 2021

Add v1alpha3 to v1alpha4 upgrade e2e test kubernetes-sigs/cluster-api-provider-aws#2388

Closed

sbueringer reviewed May 7, 2021

View reviewed changes

address comments

2a6e55d

k8s-ci-robot assigned sbueringer May 10, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2021

vincepri approved these changes May 12, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 12, 2021

k8s-ci-robot merged commit 93ee29c into kubernetes-sigs:master May 12, 2021

fabriziopandini deleted the clusterctl-upgrade-e2e-test branch May 17, 2021 10:04

Conversation

fabriziopandini commented Sep 29, 2020

Uh oh!

fabriziopandini commented Sep 29, 2020

Uh oh!

fabriziopandini commented Oct 6, 2020

Uh oh!

fabriziopandini commented Oct 13, 2020

Uh oh!

fabriziopandini commented Oct 13, 2020

Uh oh!

fabriziopandini commented Oct 14, 2020

Uh oh!

fabriziopandini commented Nov 19, 2020

Uh oh!

fabriziopandini commented Nov 19, 2020

Uh oh!

fabriziopandini commented Nov 19, 2020

Uh oh!

wfernandes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fabriziopandini commented Nov 20, 2020

Uh oh!

fabriziopandini commented Nov 20, 2020

Uh oh!

wfernandes commented Nov 20, 2020

Uh oh!

fabriziopandini commented Apr 27, 2021

Uh oh!

sbueringer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbueringer commented Apr 28, 2021

Uh oh!

fabriziopandini May 5, 2021

Choose a reason for hiding this comment

Uh oh!

fabriziopandini May 5, 2021

Choose a reason for hiding this comment

Uh oh!

sbueringer May 7, 2021

Choose a reason for hiding this comment

Uh oh!

fabriziopandini May 10, 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!

sbueringer May 10, 2021

Choose a reason for hiding this comment

Uh oh!

fabriziopandini commented May 5, 2021

Uh oh!

sbueringer commented May 5, 2021

Uh oh!

sbueringer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbueringer May 7, 2021

Choose a reason for hiding this comment

Uh oh!

fabriziopandini May 10, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbueringer May 7, 2021

Choose a reason for hiding this comment

Uh oh!

fabriziopandini May 10, 2021 •

edited

Loading