⚠️ add docker machine pool and e2e tests for machine pool #3506

devigned · 2020-08-19T16:32:22Z

What this PR does / why we need it:
Currently, the test framework is unaware of machine pools and will not wait for those nodes to be ready. This PR is to enhance the test framework to include machine pool functionality.

This PR adds a DockerMachinePool implementation. As part of adding that implementation, the PR also expands upon the e2e tests to validate the DockerMachinePool.

devigned · 2020-08-19T16:40:41Z

/retest

devigned · 2020-08-20T17:08:15Z

@fabriziopandini what would you think about me adding docker infra implementation of machine pool?

fabriziopandini · 2020-08-28T09:57:43Z

@fabriziopandini what would you think about me adding docker infra implementation of machine pool?

Sorry for lagging in the response.
I'm +1 to this, given that this will allow validation of machine pools in the CAPI e2e
I available to help if we agree on high-level design and there is a chance to break down the work

fabriziopandini

First pass. I'm really happy to see this happening!
/cc @srm09

test/infrastructure/docker/exp/api/v1alpha3/dockermachinepool_types.go

test/infrastructure/docker/exp/README.md

test/infrastructure/docker/examples/machine-pool.yaml

fabriziopandini · 2020-09-10T10:27:50Z

test/framework/clusterctl/clusterctl_helpers.go

 }

 // ApplyClusterTemplateAndWait gets a cluster template using clusterctl, and waits for the cluster to be ready.
 // Important! this method assumes the cluster uses a KubeadmControlPlane and MachineDeployments.
-func ApplyClusterTemplateAndWait(ctx context.Context, input ApplyClusterTemplateAndWaitInput) (*clusterv1.Cluster, *controlplanev1.KubeadmControlPlane, []*clusterv1.MachineDeployment) {
+func ApplyClusterTemplateAndWait(ctx context.Context, input ApplyClusterTemplateAndWaitInput) *ApplyClusterTemplateAndWaitResult {


This is a breaking change for all the consumers of the test framework, so we should document an action required in the release note or wait for v1alpha4
@vincepri ^^

test/e2e/kcp_upgrade.go

test/framework/machinedeployment_helpers.go

k8s-ci-robot · 2020-09-10T10:39:01Z

@fabriziopandini: GitHub didn't allow me to request PR reviews from the following users: srm09.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

First pass. I'm really happy to see this happening!
/cc @srm09

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.

devigned · 2020-09-10T15:41:06Z

Thank you for the feedback, @fabriziopandini.

Sorry for the slow response. I think I missed your first message while I was on vacation.

I'm pretty close on the machine creation / deletion within the DockerMachinePool. I'd love to work with you on design / implementation. Thought it might help the discussion to get a working sample and make changes from there.

fabriziopandini

@devigned this is a big jump forward!
I think that we can make the reconcile loop more robust if we start from a list of existing containers and then adapt the current real state to desired.
Also, it will be great if we can find a way to implement once the bits that are identical from MachinePool and Machine (e.g. getBootstrapToken, SetProviderID), but at the same time I don't want to expand too much the scope of this PR, so I leave this evaluation to you.

test/framework/machinepool_helpers.go

test/infrastructure/docker/exp/api/v1alpha3/dockermachinepool_types.go

test/infrastructure/docker/exp/controllers/dockermachinepool_controller.go

CecileRobertMichon · 2020-09-22T18:33:49Z

~~Given that this is adding a whole new feature set to the docker provider, it should probably be a ✨ commit~~

EDIT:

Incompatible changes:
  - ApplyClusterTemplateAndWait: changed from func(context.Context, ApplyClusterTemplateAndWaitInput) (*sigs.k8s.io/cluster-api/api/v1alpha3.Cluster, *sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1alpha3.KubeadmControlPlane, []*sigs.k8s.io/cluster-api/api/v1alpha3.MachineDeployment) to func(context.Context, ApplyClusterTemplateAndWaitInput) *ApplyClusterTemplateAndWaitResult

This is a breaking change, needs to be called out on release notes with ⚠️

fabriziopandini

@devigned
I finally gave a look at the controller (sorry for lagging behind a little bit in reviewing this PR)

test/infrastructure/docker/exp/controllers/dockermachinepool_controller.go

fabriziopandini · 2020-09-24T08:44:35Z

test/infrastructure/docker/exp/controllers/dockermachinepool_controller.go

+		}
+	}
+
+	return np.Refresh()


should this goes before the loop reconciling machines, so the new instance are reconciled immediately?

Refresh is called when initializing the node pool. If the node pool were to stick around for a while, this change would probably be needed. At this point, a node pool is created in the reconcile loop and is acting on only that one docker machine pool. I don't think that much will change between construction and use of the node pool, so this probably won't have much effect.

test/infrastructure/docker/exp/controllers/dockermachinepool_controller.go

devigned · 2020-09-24T23:07:59Z

/test pull-cluster-api-e2e-full

devigned · 2020-09-24T23:12:01Z

/test pull-cluster-api-e2e-full

k8s-ci-robot · 2020-09-24T23:13:51Z

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

Test name	Commit	Details	Rerun command
pull-cluster-api-verify-external-links	`0870ba2`	link	`/test pull-cluster-api-verify-external-links`
pull-cluster-api-apidiff	`f631989`	link	`/test pull-cluster-api-apidiff`

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.

fabriziopandini

@devigned great work!
/approve

k8s-ci-robot · 2020-09-25T12:07:08Z

[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:

~~OWNERS~~ [fabriziopandini]

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

devigned · 2020-09-25T12:16:05Z

Really appreciate your insightful reviews, @fabriziopandini and @CecileRobertMichon! Thank you.

CecileRobertMichon

/lgtm

🙌

vincepri · 2020-09-25T15:54:22Z

/milestone v0.3.10

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 19, 2020

k8s-ci-robot requested review from detiber and ncdc August 19, 2020 16:32

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

devigned force-pushed the e2e-mp branch from efc6aa1 to f3a7a4d Compare August 19, 2020 16:46

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 20, 2020

devigned force-pushed the e2e-mp branch from 4255d3b to 0472453 Compare August 27, 2020 15:52

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

devigned force-pushed the e2e-mp branch 3 times, most recently from 43aff40 to 2993308 Compare August 27, 2020 19:36

devigned force-pushed the e2e-mp branch from 2993308 to 752aef6 Compare September 9, 2020 17:58

fabriziopandini reviewed Sep 10, 2020

View reviewed changes

devigned force-pushed the e2e-mp branch 2 times, most recently from ad67bd5 to b4bcdeb Compare September 11, 2020 01:57

This comment has been minimized.

Sign in to view

fabriziopandini reviewed Sep 13, 2020

View reviewed changes

devigned force-pushed the e2e-mp branch 3 times, most recently from 4238b16 to 46c6328 Compare September 16, 2020 03:10

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

CecileRobertMichon mentioned this pull request Sep 17, 2020

e2e tests for AWSMachinePools kubernetes-sigs/cluster-api-provider-aws#1940

Closed

fabriziopandini reviewed Sep 24, 2020

View reviewed changes

devigned force-pushed the e2e-mp branch from 7e68281 to 0870ba2 Compare September 24, 2020 15:56

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

devigned changed the title ~~🌱 add docker machine pool implementation~~ ✨ add docker machine pool implementation Sep 24, 2020

devigned changed the title ~~✨ add docker machine pool implementation~~ ✨ add docker machine pool implementation and e2e tests for machine pool Sep 24, 2020

devigned changed the title ~~✨ add docker machine pool implementation and e2e tests for machine pool~~ ✨ add docker machine pool and e2e tests for machine pool Sep 24, 2020

devigned mentioned this pull request Sep 24, 2020

Move DockerMachineSpec.Bootstrapped to status #3686

Closed

devigned force-pushed the e2e-mp branch 2 times, most recently from 1a862a7 to 59e42ec Compare September 24, 2020 22:18

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

devigned changed the title ~~✨ add docker machine pool and e2e tests for machine pool~~ ⚠️ add docker machine pool and e2e tests for machine pool Sep 24, 2020

devigned force-pushed the e2e-mp branch from 59e42ec to 8b1ba69 Compare September 24, 2020 22:23

devigned requested a review from fabriziopandini September 24, 2020 22:49

✨ add docker machine pool and e2e tests for capi machine pool

f631989

devigned force-pushed the e2e-mp branch from 8b1ba69 to f631989 Compare September 24, 2020 23:10

fabriziopandini reviewed Sep 25, 2020

View reviewed changes

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

CecileRobertMichon approved these changes Sep 25, 2020

View reviewed changes

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

k8s-ci-robot merged commit ccb4ad5 into kubernetes-sigs:master Sep 25, 2020

devigned deleted the e2e-mp branch September 25, 2020 15:51

k8s-ci-robot added this to the v0.3.10 milestone Sep 25, 2020

devigned mentioned this pull request Oct 1, 2020

Add MachinePool tests from CAPI kubernetes-sigs/cluster-api-provider-azure#970

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ add docker machine pool and e2e tests for machine pool #3506

⚠️ add docker machine pool and e2e tests for machine pool #3506

devigned commented Aug 19, 2020 •

edited

Loading

devigned commented Aug 19, 2020

devigned commented Aug 20, 2020

fabriziopandini commented Aug 28, 2020

fabriziopandini left a comment

fabriziopandini Sep 10, 2020

k8s-ci-robot commented Sep 10, 2020

devigned commented Sep 10, 2020

This comment has been minimized.

fabriziopandini left a comment

CecileRobertMichon commented Sep 22, 2020 •

edited

Loading

fabriziopandini left a comment

fabriziopandini Sep 24, 2020

devigned Sep 24, 2020

devigned commented Sep 24, 2020

devigned commented Sep 24, 2020

k8s-ci-robot commented Sep 24, 2020

fabriziopandini left a comment

k8s-ci-robot commented Sep 25, 2020

devigned commented Sep 25, 2020

CecileRobertMichon left a comment

vincepri commented Sep 25, 2020

⚠️ add docker machine pool and e2e tests for machine pool #3506

⚠️ add docker machine pool and e2e tests for machine pool #3506

Conversation

devigned commented Aug 19, 2020 • edited Loading

devigned commented Aug 19, 2020

devigned commented Aug 20, 2020

fabriziopandini commented Aug 28, 2020

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini Sep 10, 2020

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 10, 2020

devigned commented Sep 10, 2020

This comment has been minimized.

fabriziopandini left a comment

Choose a reason for hiding this comment

CecileRobertMichon commented Sep 22, 2020 • edited Loading

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini Sep 24, 2020

Choose a reason for hiding this comment

devigned Sep 24, 2020

Choose a reason for hiding this comment

devigned commented Sep 24, 2020

devigned commented Sep 24, 2020

k8s-ci-robot commented Sep 24, 2020

fabriziopandini left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 25, 2020

devigned commented Sep 25, 2020

CecileRobertMichon left a comment

Choose a reason for hiding this comment

vincepri commented Sep 25, 2020

devigned commented Aug 19, 2020 •

edited

Loading

CecileRobertMichon commented Sep 22, 2020 •

edited

Loading