🏃 Wire up kubeadm control plane Ready status #2488

chuckha · 2020-02-28T23:32:34Z

Signed-off-by: Chuck Ha [email protected]

What this PR does / why we need it:
This PR aligns the KCP Initialized field with the comment on the field in the API type as well as sets the Ready status when it is appropriate to set.

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 #2480

I'm holding this because I might have to make changes to the capd e2es now that we're actually setting Ready based on NodeReady which requires CNI to exist on the cluster. CAPD may now how to install a CNI to pass which it really should do anyway in order to scale machine deployments.

In general, my experience with the tests for the reconciler are that we are testing many things throughout the whole Reconcile function. This is good in some ways and it's bad in others. There must be some way to make these tests more readable/understandable because it takes me forever to understand what it's doing and why an assertion is being made.

k8s-ci-robot · 2020-02-28T23:32:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

controlplane/kubeadm/controllers/kubeadm_control_plane_controller_test.go

CecileRobertMichon · 2020-02-28T23:36:12Z

nit: there's a typo in the commit message should be control plane

chuckha · 2020-02-28T23:36:52Z

nit: there's a typo in the commit message should be control plane

🦅 👀

sethp-nr

Agreed about the CNI piece: In my experience MachineDeployments also get stuck in weird ways if there's no CNI (so there's no chance of a Ready Node).

/lgtm

controlplane/kubeadm/controllers/kubeadm_control_plane_controller_test.go

sethp-nr · 2020-02-28T23:58:35Z

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go

-			}
-		}
+		r.Log.Info("unable to get workload cluster", "err", fmt.Sprintf("%s", err))
+		return nil


Should this be a capierrors.RequeueAfterError, do you think?

You are right that this change set would modify behavior in a subtle way. I think this should handle the error cases exactly as they were being handled before and so not change any behavior of this function.

Do you think a capierrors.RequeueAfterError is more appropriate than returning the errors.Wrap?

I think my worry with not using RequeueAfterError here is that we could hit the 10 retries during the normal error path prior to the remote APIServer being available, which would lead to needing to wait for the resync interval to continue updating the status (or really any other operations after the creation of the initial control plane instance), while waiting for that initial machine to finish booting/initializing.

The way the defer is structured there is no way to send up a requeue after error from updateStatus. If you think this should definitely use requeueAfter then we'll need a new issue / PR to refactor how the defer works

chuckha · 2020-03-02T15:12:17Z

Agreed about the CNI piece: In my experience MachineDeployments also get stuck in weird ways if there's no CNI (so there's no chance of a Ready Node).

yeah, machine deployments require CNI to get nodes into a ready state for scaling to work appropriately

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go

chuckha · 2020-03-03T00:07:39Z

/hold cancel

This is good for review

test/infrastructure/docker/e2e/docker_test.go

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go

controlplane/kubeadm/internal/workload_cluster.go

vincepri · 2020-03-03T14:52:37Z

test/framework/control_plane.go

+	Eventually(func() (bool, error) {
+		machineList := &clusterv1.MachineList{}
+		if err := input.Lister.List(ctx, machineList, inClustersNamespaceListOption, matchClusterListOption); err != nil {
+			fmt.Println(err)


Actually no, it's not leftover, this is the pattern used throughout. These fmt.Printlns should be replaced with a writer to ginkgo's log. I'm going to keep this and file an issue for improvement.

chuckha · 2020-03-03T19:47:28Z

with this issue, i believe this is good to merge #2514

@vincepri got time for one more 👁 👃 👁?

rudoi · 2020-03-04T16:29:53Z

/lgtm

just a lovely piece of code

Signed-off-by: Chuck Ha <[email protected]>

vincepri

/lgtm

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 28, 2020

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

k8s-ci-robot requested review from CecileRobertMichon and detiber February 28, 2020 23:33

chuckha commented Feb 28, 2020

View reviewed changes

controlplane/kubeadm/controllers/kubeadm_control_plane_controller_test.go Show resolved Hide resolved

chuckha force-pushed the kcp-read branch from cc70175 to 3d5d479 Compare February 28, 2020 23:37

chuckha changed the title ~~🏃 Wire up kubeadm control plnae Ready status~~ 🏃 Wire up kubeadm control plane Ready status Feb 28, 2020

sethp-nr reviewed Feb 29, 2020

View reviewed changes

k8s-ci-robot assigned sethp-nr Feb 29, 2020

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

vincepri added this to the v0.3.0-rc.3 milestone Mar 2, 2020

chuckha force-pushed the kcp-read branch from 3d5d479 to c3bf7e2 Compare March 2, 2020 16:21

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

chuckha commented Mar 2, 2020

View reviewed changes

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go Show resolved Hide resolved

chuckha mentioned this pull request Mar 2, 2020

✨ Adds kubernetes networking to capd e2es #2506

Merged

chuckha force-pushed the kcp-read branch from c3bf7e2 to be3b7f1 Compare March 3, 2020 00:07

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

chuckha force-pushed the kcp-read branch from be3b7f1 to 3a4d560 Compare March 3, 2020 14:36

sethp-nr reviewed Mar 3, 2020

View reviewed changes

test/infrastructure/docker/e2e/docker_test.go Show resolved Hide resolved

sethp-nr reviewed Mar 3, 2020

View reviewed changes

controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go Outdated Show resolved Hide resolved

chuckha force-pushed the kcp-read branch 2 times, most recently from 7aef971 to 4f0d8d7 Compare March 3, 2020 14:47

vincepri reviewed Mar 3, 2020

View reviewed changes

controlplane/kubeadm/internal/workload_cluster.go Show resolved Hide resolved

vincepri reviewed Mar 3, 2020

View reviewed changes

chuckha force-pushed the kcp-read branch from 4f0d8d7 to 4cb085e Compare March 3, 2020 18:24

chuckha force-pushed the kcp-read branch from 4cb085e to fe81df9 Compare March 4, 2020 16:22

k8s-ci-robot assigned rudoi Mar 4, 2020

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

Wire up kubeadm control plane Ready status

40ea63e

Signed-off-by: Chuck Ha <[email protected]>

chuckha force-pushed the kcp-read branch from fe81df9 to 40ea63e Compare March 4, 2020 16:34

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

vincepri approved these changes Mar 4, 2020

View reviewed changes

k8s-ci-robot assigned vincepri Mar 4, 2020

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

k8s-ci-robot merged commit 88506f8 into kubernetes-sigs:master Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🏃 Wire up kubeadm control plane Ready status #2488

🏃 Wire up kubeadm control plane Ready status #2488

chuckha commented Feb 28, 2020 •

edited

Loading

k8s-ci-robot commented Feb 28, 2020

CecileRobertMichon commented Feb 28, 2020

chuckha commented Feb 28, 2020

sethp-nr left a comment

sethp-nr Feb 28, 2020

chuckha Mar 2, 2020

detiber Mar 2, 2020

chuckha Mar 3, 2020

chuckha commented Mar 2, 2020

chuckha commented Mar 3, 2020

vincepri Mar 3, 2020

chuckha Mar 3, 2020

chuckha Mar 3, 2020

chuckha commented Mar 3, 2020

rudoi commented Mar 4, 2020

vincepri left a comment

🏃 Wire up kubeadm control plane Ready status #2488

🏃 Wire up kubeadm control plane Ready status #2488

Conversation

chuckha commented Feb 28, 2020 • edited Loading

k8s-ci-robot commented Feb 28, 2020

CecileRobertMichon commented Feb 28, 2020

chuckha commented Feb 28, 2020

sethp-nr left a comment

Choose a reason for hiding this comment

sethp-nr Feb 28, 2020

Choose a reason for hiding this comment

chuckha Mar 2, 2020

Choose a reason for hiding this comment

detiber Mar 2, 2020

Choose a reason for hiding this comment

chuckha Mar 3, 2020

Choose a reason for hiding this comment

chuckha commented Mar 2, 2020

chuckha commented Mar 3, 2020

vincepri Mar 3, 2020

Choose a reason for hiding this comment

chuckha Mar 3, 2020

Choose a reason for hiding this comment

chuckha Mar 3, 2020

Choose a reason for hiding this comment

chuckha commented Mar 3, 2020

rudoi commented Mar 4, 2020

vincepri left a comment

Choose a reason for hiding this comment

chuckha commented Feb 28, 2020 •

edited

Loading