✨ Implement getClusterState for topology controller #5045

killianmuldoon · 2021-08-02T17:37:38Z

Implementation for reading the current state of a cluster managed
by a ClusterClass to the cluster topology controller. This will
allow the controller to see the observed state of a managed
cluster and use the information during reconcilliation.

Signed-off-by: killianmuldoon [email protected]

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

k8s-ci-robot · 2021-08-02T17:37:45Z

Hi @killianmuldoon. 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.

fabriziopandini · 2021-08-02T18:39:13Z

/ok-to-test

fabriziopandini · 2021-08-03T08:57:02Z

/retest

fabriziopandini

@killianmuldoon looks great!
I have a couple of nits, most of them on comments and error variables.
The only suggestion is to use getTemplate recently introduced by #5007 (in its current form or after renaming it to getReference) instead of repeating convertreference + get in every place.

I still have to take a deeper look on tests, I will do ASAP

controllers/clustertopology_controller_compute.go

controllers/clustertopology_controller_compute_test.go

fabriziopandini

Overall lgtm, waiting for the required const to merge or to be copied in this PR
@sbueringer PTAL

controllers/clustertopology_controller_compute_test.go

killianmuldoon · 2021-08-05T11:54:40Z

/retest

controllers/clustertopology_controller_compute.go

controllers/clustertopology_controller_compute_test.go

controllers/clustertopology_controller_compute.go

controllers/clustertopology_controller_compute_test.go

killianmuldoon · 2021-08-09T17:34:53Z

@sbueringer I think I've met most of your requested changes here - let me know if there's red lines on the couple I decided not to take on.

killianmuldoon · 2021-08-09T17:39:16Z

/retest

killianmuldoon · 2021-08-09T17:58:57Z

/retest

controllers/clustertopology_controller_compute.go

sbueringer · 2021-08-10T06:39:27Z

controllers/clustertopology_controller_compute.go

+			nil,
+		}, nil
+	}
+	cpInfraRef, err := getNestedRef(cp, "spec", "machineTemplate", "infrastructureRef")


@killianmuldoon Would be really good to have a new line here. Imho it just reads a lot easier with a ibt more structuring

controllers/clustertopology_controller_compute.go

sbueringer · 2021-08-10T06:41:32Z

controllers/clustertopology_controller_compute.go

+// whether they are managed by a ClusterClass using labels. A cluster may have zero or more MachineDeployments. Zero is
+// expected on first reconcile. If MachineDeployments are found for the cluster Infrastructure and Bootstrap references


@killianmuldoon fyi still open (I think you might have missed a few nits :))

controllers/clustertopology_controller_compute.go

killianmuldoon · 2021-08-10T10:12:31Z

@sbueringer I've pushed a new version with these issues addressed (or at least I think so 😄 - PTAL

sbueringer

I think you got all, just found a new one (so feel free to resolve the open conversations above). But apart from that lgtm (+/- rebase after Farizios package move PR is merged)

controllers/clustertopology_controller_compute.go

killianmuldoon · 2021-08-10T11:41:26Z

/retest

controllers/clustertopology_controller_compute.go

fabriziopandini

/lgtm
🥳

sbueringer · 2021-08-11T15:15:07Z

/lgtm 🎉

killianmuldoon · 2021-08-11T15:21:13Z

/retest

controllers/topology/current_state.go

sbueringer · 2021-08-12T09:30:34Z

/lgtm

Implementation for reading the current state of a cluster managed by a ClusterClass to the cluster topology controller. This will allow the controller to see the observed state of a managed cluster and use the information during reconcilliation. Signed-off-by: killianmuldoon [email protected]

fabriziopandini · 2021-08-12T10:27:36Z

/lgtm
/approve

k8s-ci-robot · 2021-08-12T10:27:50Z

[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

geetikabatra · 2021-08-12T11:27:33Z

controllers/topology/current_state.go

+	}
+
+	// A Cluster may have zero or more MachineDeployments and a Cluster is expected to have zero MachineDeployments on
+	// first reconcile.


@killianmuldoon What exactly do we mean by first reconcile? Why would the number of MachineDeployments change on nth reconcile?

On the first reconcile the Machine Deployments won't have been spun up as this happens when the objects are initially created. The first reconcile - if things are working right - should be able to add the correct number of machine deployments in the spec and then the current state will come into line with the desired state as these objects are created and come online.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 2, 2021

k8s-ci-robot requested review from enxebre and fabriziopandini August 2, 2021 17:37

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 Aug 2, 2021

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 Aug 2, 2021

fabriziopandini requested changes Aug 3, 2021

View reviewed changes

killianmuldoon force-pushed the pr-read-clusterclass branch from 206c7e6 to 56ddf06 Compare August 4, 2021 13:27

fabriziopandini reviewed Aug 5, 2021

View reviewed changes

controllers/clustertopology_controller_compute_test.go Outdated Show resolved Hide resolved

controllers/clustertopology_controller_compute_test.go Outdated Show resolved Hide resolved

sbueringer reviewed Aug 9, 2021

View reviewed changes

killianmuldoon mentioned this pull request Aug 9, 2021

✨ Compute ControlPlane version for a managed topology #5059

Closed

fabriziopandini reviewed Aug 9, 2021

View reviewed changes

controllers/clustertopology_controller_compute.go Outdated Show resolved Hide resolved

k8s-ci-robot requested a review from vincepri August 9, 2021 14:16

killianmuldoon mentioned this pull request Aug 9, 2021

Make error messages consistent across ClusterClass and Topology Controller #5067

Closed

sbueringer reviewed Aug 9, 2021

View reviewed changes

sbueringer reviewed Aug 10, 2021

View reviewed changes

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

sbueringer reviewed Aug 10, 2021

View reviewed changes

controllers/clustertopology_controller_compute.go Outdated Show resolved Hide resolved

killianmuldoon force-pushed the pr-read-clusterclass branch from e6663d1 to bc58ea2 Compare August 10, 2021 11:37

sbueringer reviewed Aug 10, 2021

View reviewed changes

controllers/clustertopology_controller_compute.go Outdated Show resolved Hide resolved

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 11, 2021

killianmuldoon force-pushed the pr-read-clusterclass branch from e4382c4 to 8805a5b Compare August 11, 2021 14:18

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed 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. labels Aug 11, 2021

fabriziopandini reviewed Aug 11, 2021

View reviewed changes

k8s-ci-robot assigned fabriziopandini Aug 11, 2021

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

k8s-ci-robot assigned sbueringer Aug 11, 2021

vincepri reviewed Aug 11, 2021

View reviewed changes

controllers/topology/current_state.go Outdated Show resolved Hide resolved

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

killianmuldoon force-pushed the pr-read-clusterclass branch from 8805a5b to 278c6cc Compare August 12, 2021 09:27

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 12, 2021

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

killianmuldoon force-pushed the pr-read-clusterclass branch from 278c6cc to 383b9d0 Compare August 12, 2021 09:51

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 12, 2021

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

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

k8s-ci-robot merged commit 99def25 into kubernetes-sigs:master Aug 12, 2021

k8s-ci-robot added this to the v0.4 milestone Aug 12, 2021

geetikabatra reviewed Aug 12, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Implement getClusterState for topology controller #5045

✨ Implement getClusterState for topology controller #5045

killianmuldoon commented Aug 2, 2021

k8s-ci-robot commented Aug 2, 2021

fabriziopandini commented Aug 2, 2021

fabriziopandini commented Aug 3, 2021

fabriziopandini left a comment

fabriziopandini left a comment

killianmuldoon commented Aug 5, 2021

killianmuldoon commented Aug 9, 2021

killianmuldoon commented Aug 9, 2021

killianmuldoon commented Aug 9, 2021

sbueringer Aug 10, 2021

sbueringer Aug 10, 2021

killianmuldoon commented Aug 10, 2021

sbueringer left a comment •

edited

Loading

killianmuldoon commented Aug 10, 2021

fabriziopandini left a comment

sbueringer commented Aug 11, 2021 •

edited

Loading

killianmuldoon commented Aug 11, 2021

sbueringer commented Aug 12, 2021

fabriziopandini commented Aug 12, 2021

k8s-ci-robot commented Aug 12, 2021

geetikabatra Aug 12, 2021 •

edited

Loading

killianmuldoon Aug 12, 2021

		// whether they are managed by a ClusterClass using labels. A cluster may have zero or more MachineDeployments. Zero is
		// expected on first reconcile. If MachineDeployments are found for the cluster Infrastructure and Bootstrap references

✨ Implement getClusterState for topology controller #5045

✨ Implement getClusterState for topology controller #5045

Conversation

killianmuldoon commented Aug 2, 2021

k8s-ci-robot commented Aug 2, 2021

fabriziopandini commented Aug 2, 2021

fabriziopandini commented Aug 3, 2021

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

killianmuldoon commented Aug 5, 2021

killianmuldoon commented Aug 9, 2021

killianmuldoon commented Aug 9, 2021

killianmuldoon commented Aug 9, 2021

sbueringer Aug 10, 2021

Choose a reason for hiding this comment

sbueringer Aug 10, 2021

Choose a reason for hiding this comment

killianmuldoon commented Aug 10, 2021

sbueringer left a comment • edited Loading

Choose a reason for hiding this comment

killianmuldoon commented Aug 10, 2021

fabriziopandini left a comment

Choose a reason for hiding this comment

sbueringer commented Aug 11, 2021 • edited Loading

killianmuldoon commented Aug 11, 2021

sbueringer commented Aug 12, 2021

fabriziopandini commented Aug 12, 2021

k8s-ci-robot commented Aug 12, 2021

geetikabatra Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

killianmuldoon Aug 12, 2021

Choose a reason for hiding this comment

sbueringer left a comment •

edited

Loading

sbueringer commented Aug 11, 2021 •

edited

Loading

geetikabatra Aug 12, 2021 •

edited

Loading