✨ clusterctl e2e tests #2236

Arvinderpal · 2020-01-31T16:13:39Z

What this PR does / why we need it:
This PR brings e2e tests to clusterctl. The work leverages the existing capi e2e test framework and the capd infra provider.

Which issue(s) this PR fixes
Rif #1729

/assign @fabriziopandini

k8s-ci-robot · 2020-01-31T16:13:47Z

Hi @Arvinderpal. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Arvinderpal · 2020-01-31T16:15:09Z

This is just a WIP at this point. I wanted to share it now to get early feedback.
/cc @ncdc @vincepri

fabriziopandini

@Arvinderpal many many thanks for tacking up this task!
I'm +100 to get E2E tests for clusterctl, but I defer to @chuckha the validation of the overall approach for ensuring consistency with the other E2E tests.
/assign @chuckha

In the meantime, I did first pass on the WIP from a clusterctl PoV

fabriziopandini · 2020-02-01T07:43:15Z

cmd/clusterctl/test/e2e/config_cluster_test.go

+			// Create clusterctl.yaml
+			tmpDir = createTempDir()
+			cfgFile = createLocalTestClusterCtlConfig(tmpDir, "clusterctl.yaml", "DOCKER_SERVICE_DOMAIN: \"docker.cluster.local\"")
+			// Let's setup some varibles for the workload cluster template


What about moving this UP to line 66 and changing into:
-> Defining variables for the workload cluster template; testing both variables from the clusterctl config file and variables from OS environment variables.

Not sure I see how moving this to line 66 changes anything. Are you saying I should define the same vars in both the config file as well as in the environment?

cmd/clusterctl/test/e2e/config_cluster_test.go

fabriziopandini · 2020-02-01T07:49:21Z

cmd/clusterctl/test/e2e/config_cluster_test.go

+			CheckAndWaitDeploymentExists(kindClient, "capi-kubeadm-bootstrap-system", "capi-kubeadm-bootstrap-controller-manager")
+			CheckAndWaitDeploymentExists(kindClient, "capd-system", "capd-controller-manager")
+
+			options := clusterctlclient.GetClusterTemplateOptions{


This requires a template available in the docker repository or as a local override, which is something that is not handled by the local override script; also it creates an external dependency which I would like to avoid to make the test more predictable
I guess we should wait for #2133 to be fixed and use a template self-contained in the test / stored in the test folder

Yes, for the time being I'm just creating cluster-template.yaml in the docker overrides folder.
#2133 would be useful here, though I think just the filesystem repository should also work. I'll look into this more.

cmd/clusterctl/test/e2e/config_cluster_test.go

fabriziopandini · 2020-02-01T08:06:18Z

cmd/clusterctl/test/run-e2e.sh

+set -o nounset
+set -o pipefail
+
+REPO_ROOT=$(dirname "${BASH_SOURCE[0]}")/../../..


what about using git rev-parse --show-toplevel instead?

I used the cluster-api/scripts/ci-capd-e2e.sh approach.

fabriziopandini · 2020-02-01T08:10:02Z

cmd/clusterctl/test/e2e/e2e_suite_test.go

+var _ = BeforeSuite(func() {
+	ctx = context.Background()
+	// Docker image to load into the kind cluster for testing
+	managerImage = os.Getenv("MANAGER_IMAGE")


It might be a silly question, but how the locally built image gets injected into the kind image?

Here:
kindCluster, err := kind.NewCluster(ctx, kindClusterName, clientgoscheme.Scheme, managerImage)

I build a local image and ensure the local overrides folder points to that image. For example:

docker/v0.3.0/infrastructure-components.yaml:502: image: gcr.io/arvinders-1st-project/docker-provider-manager-amd64:dev

cmd/clusterctl/test/run-e2e.sh

fabriziopandini · 2020-02-01T08:24:02Z

/ok-to-test

cmd/clusterctl/test/e2e/config_cluster_test.go

wfernandes

I believe clusterctl is going through some more changes. Thanks for all this work! I'm sure we can build upon this once clusterctl changes slow down 😄

wfernandes · 2020-02-03T20:23:18Z

cmd/clusterctl/test/e2e/config_cluster_test.go

+			_, _, err = c.Init(initOpt)
+			Expect(err).ToNot(HaveOccurred())
+			// Confirm controllers exists
+			CheckAndWaitDeploymentExists(kindClient, "capi-system", "capi-controller-manager")


We have a e2e test framework with some convenience methods. We could use this method to verify if cert-manager is available.

cluster-api/test/framework/convenience.go

Line 50 in 5cfd0f1

func WaitForAPIServiceAvailable(ctx context.Context, mgmt Waiter, serviceName string) {

cmd/clusterctl/test/e2e/helpers.go

wfernandes · 2020-02-03T20:31:31Z

cmd/clusterctl/test/e2e/helpers.go

+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+func CreateKindClusterAndClient() (*kind.Cluster, client.Client, error) {


This may help in creating a kind cluster.

cluster-api/test/framework/management/kind/mgmt.go

Line 57 in 5cfd0f1

func NewCluster(ctx context.Context, name string, scheme *runtime.Scheme, images ...string) (*Cluster, error) {

Arvinderpal

Please take another look. I have incorporated all the feedback. It should be possible for you to run the tests locally as well. For the workload cluster create test, you will need to specify a cluster template in the local-overrides folder for docker. Let me know if you need a copy of that.

Arvinderpal · 2020-02-04T14:41:47Z

cmd/clusterctl/test/e2e/config_cluster_test.go

+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+var _ = Describe("clusterctl config cluster", func() {


Sure. However, currently I have these two tests in separate files. If you don't mind, I would like to keep it that way. At least to me, it makes for better organization to have _init, _config, _upgrade, etc.. tests in their respective files.

cmd/clusterctl/test/e2e/config_cluster_test.go

Arvinderpal · 2020-02-04T14:46:37Z

cmd/clusterctl/test/e2e/config_cluster_test.go

+			// Create clusterctl.yaml
+			tmpDir = createTempDir()
+			cfgFile = createLocalTestClusterCtlConfig(tmpDir, "clusterctl.yaml", "DOCKER_SERVICE_DOMAIN: \"docker.cluster.local\"")
+			// Let's setup some varibles for the workload cluster template


Not sure I see how moving this to line 66 changes anything. Are you saying I should define the same vars in both the config file as well as in the environment?

cmd/clusterctl/test/e2e/config_cluster_test.go

cmd/clusterctl/test/run-e2e.sh

cmd/clusterctl/test/e2e/config_cluster_test.go

Arvinderpal · 2020-02-05T02:24:07Z

cmd/clusterctl/test/e2e/config_cluster_test.go

+			os.RemoveAll(tmpDir)
+		})
+
+		Context("using default infra and bootstrap provider", func() {


Arvinderpal · 2020-02-05T02:27:20Z

cmd/clusterctl/test/e2e/config_cluster_test.go

+			CheckAndWaitDeploymentExists(kindClient, "capi-kubeadm-bootstrap-system", "capi-kubeadm-bootstrap-controller-manager")
+			CheckAndWaitDeploymentExists(kindClient, "capd-system", "capd-controller-manager")
+
+			options := clusterctlclient.GetClusterTemplateOptions{


Yes, for the time being I'm just creating cluster-template.yaml in the docker overrides folder.
#2133 would be useful here, though I think just the filesystem repository should also work. I'll look into this more.

wfernandes · 2020-02-05T17:46:39Z

@Arvinderpal I'll try and take a look at these as soon as I can. I would definitely appreciate a copy of the cluster-template for the workload cluster create test 🙂. Thanks.

Arvinderpal · 2020-02-08T18:33:58Z

@fabriziopandini Added a simple move test -- move objects of a single node capd workload cluster from one mgmt node to another. It's passing for me.

fabriziopandini

@Arvinderpal thanks for adding the move test! Let's now consolidate this PR and get this merged by

making the test self contained using a local respository instead of local overrides
conflate init and cluster config test into a single test (to shorten the overall execution time)
creating some utility func to make the code cleaner/have some building blocks for creating new tests

cmd/clusterctl/test/e2e/config_cluster_test.go

cmd/clusterctl/test/run-e2e.sh

cmd/clusterctl/test/e2e/move_test.go

fabriziopandini · 2020-02-09T13:58:43Z

cmd/clusterctl/test/e2e/move_test.go

+	})
+
+	Context("single node workerload cluster", func() {
+		It("should move all Cluster API objects to the new mgmt cluster, unpause the Cluster and delete all objects from previous mgmt cluster", func() {


This set of tests are strictly related to the cluster template. Does it make sense to move this in a separate function into the same file that is defining the cluster template?

What about expecting the same test in the from cluster/any time we create the cluster template

Arvinderpal

@fabriziopandini As per your feedback:

e2e tests are self-contained. They make use of the local repository in the artifacts folder.
I have also refactored to crate util funcs that init a mgmt cluster and create a workload cluster.
The init test has been removed.
I added a couple of delete tests as well.

cmd/clusterctl/test/e2e/config_cluster_test.go

Arvinderpal · 2020-02-10T22:18:56Z

@fabriziopandini FYI, the deletes everything spec is currently failing. I'm trying to debug the issue. The remaining tests should pass and you should be able to run them locally.

fabriziopandini · 2020-02-11T12:16:42Z

In the next iteration we should make this consistent with #2294

chuckha · 2020-02-11T14:22:51Z

Are there any docs to go along with this? I cloned the repo and ran ./cmd/clusterctl/test/run-e2e.sh but that gave me a number of errors

chuckha · 2020-02-11T14:25:51Z

cmd/clusterctl/test/e2e/config_cluster_test.go

+		createTestWorkloadCluster(ctx, mgmtInfo, workloadInfo)
+	})
+
+	AfterEach(func() {


I noticed this tears down the kind cluster and spins the whole thing back up. Do you think it would be possible to create the kind cluster once and then install and remove components without completely tearing down and standing up the cluster?

So, I debated this for a bit. Initially I went with the reusing mgmt cluster for all tests; however, for that you need to be certain that clusterctl delete -all does work 100% or that you manually delete all state (and wait for the delete to complete). Given that spinning up a new kind cluster takes a small portion of the overall execution time, and gives you a clean mgmt cluster, it seemed like the right way to go.

cmd/clusterctl/test/e2e/config_cluster_test.go

cmd/clusterctl/test/e2e/delete_test.go

cmd/clusterctl/test/e2e/helpers.go

chuckha · 2020-02-11T14:41:57Z

cmd/clusterctl/test/e2e/move_test.go

+					}
+					return nil
+				}, 3*time.Minute, 5*time.Second,
+			).ShouldNot(HaveOccurred())


our convention for this would be .Should(BeNil()) or maybe .Should(Succeed()) but in situations like this we tend to avoid the .ShouldNot(HaveOccurred()) pattern

cmd/clusterctl/test/e2e/move_test.go

Arvinderpal

@chuckha Please see my comments. I'll message you on slack to see why it's not running on your machine.

cmd/clusterctl/test/e2e/config_cluster_test.go

Arvinderpal · 2020-02-11T14:56:29Z

cmd/clusterctl/test/e2e/config_cluster_test.go

+		createTestWorkloadCluster(ctx, mgmtInfo, workloadInfo)
+	})
+
+	AfterEach(func() {


So, I debated this for a bit. Initially I went with the reusing mgmt cluster for all tests; however, for that you need to be certain that clusterctl delete -all does work 100% or that you manually delete all state (and wait for the delete to complete). Given that spinning up a new kind cluster takes a small portion of the overall execution time, and gives you a clean mgmt cluster, it seemed like the right way to go.

cmd/clusterctl/test/e2e/delete_test.go

cmd/clusterctl/test/e2e/helpers.go

cmd/clusterctl/test/e2e/move_test.go

fabriziopandini · 2020-02-11T17:14:49Z

finally got a good slot to test it locally, some points that should be addressed in follow up PRs.

repository setup:
- not override user's clusterctl-settings.json (we are not calling the clusterctl hack)
- generate the docker infrastructure-components.yaml (we are using a copy ATM)
- not rely on local overrides at all (we are still relying on providers for everything except the docker provider); we should mimic docker approach building manifests and store them in the local repository
to align the test suite to docker's one (use reporter)
to reuse framework approach/framework helpers introduced by 🏃 Refactor of the e2e framework #2294

Arvinderpal · 2020-02-11T22:31:07Z

@fabriziopandini @chuckha Please see my latest commit. I added a README for anyone who wants to run the tests locally. I also generate the docker infrastructure-components.yaml instead of copying.

fabriziopandini

only two nits from my side, everything else is for next iterations (as per the previous comment)

cmd/clusterctl/test/run-e2e.sh

fabriziopandini · 2020-02-12T08:39:47Z

cmd/clusterctl/test/README.md

@@ -0,0 +1,17 @@
+# Running the tests
+
+	./run-e2e.sh


I assume the hack should be called before run-e2e test, or I'm wrong?

Yes, you're right. I have updated the docs. The docker override folder will have to be deleted since we generated it's yamls in the test script.
We can add auto-generation of all other component yamls is in a follow up PR.

chuckha · 2020-02-12T16:18:50Z

I can't get this running locally. From a clean check out this is what happens:

$ ./cmd/clusterctl/test/run-e2e.sh 
# some time later...
./cmd/clusterctl/test/run-e2e.sh: line 53: /Users/cha/dev/capi-dev/cluster-api/_artifacts/testdata/docker/v0.3.0/infrastructure-components.yaml: No such file or directory

fabriziopandini · 2020-02-12T16:43:13Z

@chuckha the test is not self-contained now and it depends on running the clusterctl hack before (see comment #2236 (comment)).

I'm prototyping on top of this PR so we can get the test use the config file defined in the framework + other things in the framework line NewClusterForCAPD, so we can clean-up some dependency and make the experience more consistent across projects

chuckha · 2020-02-12T16:57:22Z

This has the problem of assuming host networking when trying to contact the workload cluster on CAPD. This assumption makes the e2es fail on OS X where host networking is not available.

Arvinderpal · 2020-02-12T17:00:51Z

This has the problem of assuming host networking when trying to contact the workload cluster on CAPD. This assumption makes the e2es fail on OS X where host networking is not available.

I updated README.md to reflect the lack of OS X support at the moment. Perhaps we can address this in a follow up PR.

chuckha · 2020-02-12T17:04:37Z

sounds great, looks good to me :D! Thanks for helping me fix it locally

fabriziopandini · 2020-02-13T10:33:51Z

/lgtm

@chuckha do you think we can merge this PR?
I have already started to address some of the follow-up work in #2321, introducing a config file similar to the one included in the framework, so we can greatly simplify the run-e2e script introduced by this PR and make the experience of configuring the test much more consistent and flexible

* Makes use of the capi e2e framework * Added a script to run clusterctl e2e tests. The script will issue a `make docker-build` and setup env var to load image into kind. Setup various cluserctl files and env vars. * Added a test that creates a workload cluster. * Added a clusterctl move test -- it moves a Cluster API objects associated with a single node capd cluster from one mgmt cluster to another mgmt cluster. * Added a delete test. * Use a local respository instead of local overrides for docker * run-e2e.sh script updated to setup local repo in _artifacts dir and also create a custom clusterctl.yaml. * Created util funcs to init a mgmt cluster and create a workload cluster. * Added clusterctl delete tests. * Added a README.md with instructions to run tests.

Arvinderpal · 2020-02-13T15:45:55Z

@fabriziopandini @chuckha I squashed all the commits into a single commit. Please let me know if want any additional changes; otherwise, I think we can merge this.

chuckha · 2020-02-13T17:23:39Z

/approve
looks great! It doesn't work on os x but that's documented so assigning to fabrizio for final lgtm

/assign @fabriziopandini
for lgtm

k8s-ci-robot · 2020-02-13T17:24:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Arvinderpal, chuckha

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:

~~OWNERS~~ [chuckha]

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

fabriziopandini · 2020-02-18T07:38:42Z

/lgtm

k8s-ci-robot assigned fabriziopandini Jan 31, 2020

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 31, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 31, 2020

k8s-ci-robot requested review from justinsb and ncdc January 31, 2020 16:13

k8s-ci-robot requested a review from vincepri January 31, 2020 16:15

fabriziopandini mentioned this pull request Feb 1, 2020

Tracking issue for clusterctl v2 implementation #1729

Closed

62 tasks

fabriziopandini reviewed Feb 1, 2020

View reviewed changes

k8s-ci-robot assigned chuckha Feb 1, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 1, 2020

ncdc added this to the v0.3.0 milestone Feb 3, 2020

wfernandes reviewed Feb 3, 2020

View reviewed changes

cmd/clusterctl/test/e2e/config_cluster_test.go Outdated Show resolved Hide resolved

wfernandes reviewed Feb 3, 2020

View reviewed changes

cmd/clusterctl/test/e2e/config_cluster_test.go Outdated Show resolved Hide resolved

wfernandes reviewed Feb 3, 2020

View reviewed changes

Arvinderpal force-pushed the clusterctl-e2e branch from d1a197b to 9673250 Compare February 5, 2020 02:34

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 5, 2020

Arvinderpal commented Feb 5, 2020

View reviewed changes

Arvinderpal force-pushed the clusterctl-e2e branch from 9673250 to 9472b35 Compare February 8, 2020 18:31

Arvinderpal force-pushed the clusterctl-e2e branch from 9472b35 to 421c839 Compare February 9, 2020 00:35

fabriziopandini reviewed Feb 9, 2020

View reviewed changes

Arvinderpal commented Feb 10, 2020

View reviewed changes

cmd/clusterctl/test/e2e/config_cluster_test.go Outdated Show resolved Hide resolved

cmd/clusterctl/test/e2e/config_cluster_test.go Outdated Show resolved Hide resolved

Arvinderpal force-pushed the clusterctl-e2e branch from e8ec819 to 66f0bcc Compare February 11, 2020 13:54

Arvinderpal changed the title ~~✨ [wip] clusterctl e2e tests~~ ✨ clusterctl e2e tests Feb 11, 2020

chuckha reviewed Feb 11, 2020

View reviewed changes

Arvinderpal commented Feb 11, 2020

View reviewed changes

Arvinderpal force-pushed the clusterctl-e2e branch from 66f0bcc to 5df11f0 Compare February 11, 2020 15:09

fabriziopandini reviewed Feb 12, 2020

View reviewed changes

Arvinderpal force-pushed the clusterctl-e2e branch from 43ae2a1 to 5531675 Compare February 12, 2020 13:53

Arvinderpal force-pushed the clusterctl-e2e branch from 5531675 to 6a69ce8 Compare February 12, 2020 16:59

fabriziopandini mentioned this pull request Feb 13, 2020

✨clusterctl: e2e test framework #2321

Closed

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2020

Arvinderpal force-pushed the clusterctl-e2e branch from 6a69ce8 to 57ae35d Compare February 13, 2020 15:43

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2020

k8s-ci-robot merged commit ebced80 into kubernetes-sigs:master Feb 18, 2020

✨ clusterctl e2e tests #2236

✨ clusterctl e2e tests #2236

Conversation

Arvinderpal commented Jan 31, 2020

k8s-ci-robot commented Jan 31, 2020

Arvinderpal commented Jan 31, 2020

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini Feb 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini commented Feb 1, 2020

wfernandes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arvinderpal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfernandes commented Feb 5, 2020

Arvinderpal commented Feb 8, 2020

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arvinderpal left a comment

Choose a reason for hiding this comment

Arvinderpal commented Feb 10, 2020

fabriziopandini commented Feb 11, 2020

chuckha commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arvinderpal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini commented Feb 11, 2020

Arvinderpal commented Feb 11, 2020

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckha commented Feb 12, 2020

fabriziopandini commented Feb 12, 2020

chuckha commented Feb 12, 2020

Arvinderpal commented Feb 12, 2020

chuckha commented Feb 12, 2020

fabriziopandini commented Feb 13, 2020

Arvinderpal commented Feb 13, 2020

chuckha commented Feb 13, 2020

k8s-ci-robot commented Feb 13, 2020

fabriziopandini commented Feb 18, 2020

fabriziopandini Feb 1, 2020 •

edited

Loading