🌱 Test and document controller ownerReferences #9153

killianmuldoon · 2023-08-09T12:47:31Z

Signed-off-by: killianmuldoon kmuldoon@vmware.com

Add tests and documentation for which ownerReferences should have controller: true for Cluster API objects.

Part of #4014

killianmuldoon

IMO we should use this PR to close any gaps between what we see here and the intention expressed in #4014. I'll note them on the tables.

killianmuldoon

One other note - the state of BlockOwnerDeletion is not consistent across all of our types where Controller is true I'd suggest setting this to true in all cases where controller is true.

killianmuldoon · 2023-08-09T12:50:35Z

docs/book/src/reference/owner_references.md

+
+## Owner reference relationships in Cluster API
+
+The below tables map out the a reference for ownership relationships for the objects in a Cluster API cluster. The tables are identical for classy and non-classy clusters.


classy and non-classy are the same as far as testing goes. I'm happy to have this as a principle to reduce complexity, but there's certain points where it may not make sense. e.g. for classy clusters KCP is created by the cluster controller, but for non-classy clusters it may be created by the user.

killianmuldoon · 2023-08-09T12:52:16Z

docs/book/src/reference/owner_references.md

+## KubeadmControlPlane types
+| type                        | Owner        | Controller | Note |
+|-----------------------------|--------------|------------|------|
+| KubeadmControlPlane         | Cluster      | true       |      |


Should not be contolled by Cluster as the Cluster doesn't necessarily create it.

killianmuldoon · 2023-08-09T12:52:53Z

docs/book/src/reference/owner_references.md

+## Kubeadm bootstrap types
+| type                  | Owner        | Controller | Note                                            |
+|-----------------------|--------------|------------|-------------------------------------------------|
+| KubeadmConfig         | Machine      | true       | When created for Machine                        |


This is created by the MachineSet controller and then the ownerReference is passed over. I think it's still fine to keep this as a controller though.

killianmuldoon · 2023-08-09T12:53:25Z

docs/book/src/reference/owner_references.md

+## Infrastructure provider types
+| type                          | Owner        | Controller | Note                                        |
+|-------------------------------|--------------|------------|---------------------------------------------|
+| InfrastructureMachine         | Machine      | yes        |                                             |


Similar to KubeadmConfig - this is passed over from the MachineSet / KCP controller.

killianmuldoon · 2023-08-09T12:57:33Z

docs/book/src/reference/owner_references.md

+| InfrastructureMachine         | Machine      | yes        |                                             |
+| InfrastructureMachineTemplate | Cluster      | no         | When created by cluster topology controller |
+| InfrastructureMachineTemplate | ClusterClass | no         | When referenced in a ClusterClass           |
+| InfrastructureCluster         | Cluster      | yes        |                                             |


Not true for classy clusters. Should probably be reverted to no.

killianmuldoon · 2023-08-09T12:57:50Z

docs/book/src/reference/owner_references.md

+| type                       | Owner              | Controller | Note              |
+|----------------------------|--------------------|------------|-------------------|
+| ClusterResourcesSet        | None               |            |                   |
+| ClusterResourcesSetBinding | ClusterResourceSet | no         |                   |


Should be yes.

killianmuldoon · 2023-08-09T12:58:49Z

docs/book/src/reference/owner_references.md

+| type                  | Owner        | Controller | Note                                            |
+|-----------------------|--------------|------------|-------------------------------------------------|
+| KubeadmConfig         | Machine      | true       | When created for Machine                        |
+| KubeadmConfig         | MachinePool  | unknown    | When created for MachinePool. Not tested in e2e |


We no longer test MachinePools in the quick start so this isn't covered. I can open an issue for this and tag folks that work regularly on MachinePools.

killianmuldoon · 2023-08-09T12:59:25Z

test/framework/ownerreference_helpers.go

-func hasExactOwnersByGVK(refList []metav1.OwnerReference, wantGVKs []schema.GroupVersionKind) error {
-	refGVKs := []schema.GroupVersionKind{}
+// simpleOwnerRef is a simple representation of an ownerReference.
+type simpleOwnerRef struct {


A bunch of glue code changes to make the controller references comparable.

killianmuldoon · 2023-08-09T13:01:36Z

/test

k8s-ci-robot · 2023-08-09T13:01:39Z

@killianmuldoon: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-cluster-api-build-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-test-main
/test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-apidiff-main
/test pull-cluster-api-e2e-full-dualstack-and-ipv6-main
/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-informing-main
/test pull-cluster-api-e2e-mink8s-main
/test pull-cluster-api-e2e-scale-main-experimental
/test pull-cluster-api-e2e-workload-upgrade-1-27-latest-main
/test pull-cluster-api-test-mink8s-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-apidiff-main
pull-cluster-api-build-main
pull-cluster-api-e2e-informing-main
pull-cluster-api-e2e-main
pull-cluster-api-test-main
pull-cluster-api-verify-main

In response to this:

/test

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.

killianmuldoon · 2023-08-09T13:02:07Z

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

killianmuldoon · 2023-08-09T13:06:28Z

/area api

killianmuldoon · 2023-08-24T09:04:23Z

/hold

I want to make sure the utils exposed here are right before merging.

killianmuldoon · 2023-08-24T13:25:59Z

/hold cancel

I think we should get the initial state of this merged and I'll create an issue, or update the existing issue, with elements of our ownerShip graph that we may want to change based on this documentation and test.

killianmuldoon · 2023-08-24T13:27:12Z

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

docs/book/src/reference/owner_references.md

sbueringer · 2023-08-24T16:13:25Z

test/framework/ownerreference_helpers.go

 	},
 	// ClusterResourcesSetBinding has ClusterResourceSet set as owners on creation.
 	clusterResourceSetBindingKind: func(owners []metav1.OwnerReference) error {
-		return hasExactOwnersByGVK(owners, []schema.GroupVersionKind{clusterResourceSetGVK})
+		return HasOneOfExactOwners(owners, []simpleOwnerRef{clusterResourceSetOwner}, []simpleOwnerRef{clusterResourceSetOwner, clusterResourceSetOwner})


Mostly out of curiosity. Do you know why 1 or 2? Just the way our tests are written?

ClusterResourceSetBinding will have as many owners as ClusterResourceSets it managed for a single cluster. I added the option of 2 here just to save me a rewrite in CAPV - can revert if you'd prefer.

I think this is one of the ownerReference that we should reconsider.

Ah I see because doesn't matter how many CRS we have we will only have one ClusterResourceSetBinding per cluster, right?

(not really familiar with this stuff)

Yeah - each Cluster will have one CRS binding, but each binding may represent multiple ClusterResourceSets

WDYT about opening a follow-up issue to support something like ">=1 Owners of the same" in the assertion?

Definitely just a nice to have and we don't have to immediately follow-up. I think the current change is just a simple improvement to support more cases. So fine to merge as is

Signed-off-by: killianmuldoon <kmuldoon@vmware.com>

sbueringer · 2023-08-24T16:29:55Z

/lgtm
/approve

k8s-ci-robot · 2023-08-24T16:30:01Z

LGTM label has been added.

Git tree hash: 387f9c8cc25f49e5cb4f8df4ca9c8867af0ce568

k8s-ci-robot · 2023-08-24T16:30:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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~~ [sbueringer]

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

killianmuldoon · 2023-08-24T17:03:10Z

/cherry-pick release-1.5

k8s-infra-cherrypick-robot · 2023-08-24T17:03:49Z

@killianmuldoon: new pull request created: #9303

In response to this:

/cherry-pick release-1.5

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.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 9, 2023

k8s-ci-robot requested review from jackfrancis and stmcginnis August 9, 2023 12:47

k8s-ci-robot added 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 Aug 9, 2023

killianmuldoon commented Aug 9, 2023

View reviewed changes

killianmuldoon force-pushed the pr-test-controller-refs branch from 7e70e12 to 0fd7f7c Compare August 9, 2023 13:02

killianmuldoon changed the title ~~[WIP] 🌱 Add a guide describing ownerReference usage in CAPI~~ [WIP] 🌱 Test and document controller ownerReferences Aug 9, 2023

k8s-ci-robot added the area/api Issues or PRs related to the APIs label Aug 9, 2023

killianmuldoon mentioned this pull request Aug 9, 2023

📖 Add a guide describing ownerReference usage in CAPI #9150

Merged

killianmuldoon changed the title ~~[WIP] 🌱 Test and document controller ownerReferences~~ 🌱 Test and document controller ownerReferences Aug 23, 2023

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 Aug 23, 2023

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

killianmuldoon force-pushed the pr-test-controller-refs branch 3 times, most recently from d3bb8dc to f4e80d7 Compare August 24, 2023 13:05

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

killianmuldoon force-pushed the pr-test-controller-refs branch from f4e80d7 to 4a89ffb Compare August 24, 2023 13:26

sbueringer reviewed Aug 24, 2023

View reviewed changes

Ensure controller references are stable

cedb82e

Signed-off-by: killianmuldoon <kmuldoon@vmware.com>

killianmuldoon force-pushed the pr-test-controller-refs branch from 4a89ffb to cedb82e Compare August 24, 2023 16:21

k8s-ci-robot assigned sbueringer Aug 24, 2023

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

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

k8s-ci-robot merged commit 62a574c into kubernetes-sigs:main Aug 24, 2023

k8s-ci-robot added this to the v1.6 milestone Aug 24, 2023

k8s-infra-cherrypick-robot mentioned this pull request Aug 24, 2023

[release-1.5] 🌱 Test and document controller ownerReferences #9303

Merged

killianmuldoon mentioned this pull request Aug 29, 2023

Cluster API should not use controller refs for resources that it does not create #4014

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Test and document controller ownerReferences #9153

🌱 Test and document controller ownerReferences #9153

killianmuldoon commented Aug 9, 2023 •

edited

Loading

killianmuldoon left a comment

killianmuldoon left a comment

killianmuldoon Aug 9, 2023

killianmuldoon Aug 9, 2023

killianmuldoon Aug 9, 2023

killianmuldoon Aug 9, 2023

killianmuldoon Aug 9, 2023

killianmuldoon Aug 9, 2023

killianmuldoon Aug 9, 2023

killianmuldoon Aug 9, 2023

killianmuldoon commented Aug 9, 2023

k8s-ci-robot commented Aug 9, 2023

killianmuldoon commented Aug 9, 2023

killianmuldoon commented Aug 9, 2023

killianmuldoon commented Aug 24, 2023

killianmuldoon commented Aug 24, 2023

killianmuldoon commented Aug 24, 2023

sbueringer Aug 24, 2023

killianmuldoon Aug 24, 2023

sbueringer Aug 24, 2023

killianmuldoon Aug 24, 2023

sbueringer Aug 24, 2023 •

edited

Loading

sbueringer commented Aug 24, 2023

k8s-ci-robot commented Aug 24, 2023

k8s-ci-robot commented Aug 24, 2023

killianmuldoon commented Aug 24, 2023

k8s-infra-cherrypick-robot commented Aug 24, 2023


		## Owner reference relationships in Cluster API

		The below tables map out the a reference for ownership relationships for the objects in a Cluster API cluster. The tables are identical for classy and non-classy clusters.

🌱 Test and document controller ownerReferences #9153

🌱 Test and document controller ownerReferences #9153

Conversation

killianmuldoon commented Aug 9, 2023 • edited Loading

killianmuldoon left a comment

Choose a reason for hiding this comment

killianmuldoon 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killianmuldoon commented Aug 9, 2023

k8s-ci-robot commented Aug 9, 2023

killianmuldoon commented Aug 9, 2023

killianmuldoon commented Aug 9, 2023

killianmuldoon commented Aug 24, 2023

killianmuldoon commented Aug 24, 2023

killianmuldoon commented Aug 24, 2023

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

sbueringer Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

sbueringer commented Aug 24, 2023

k8s-ci-robot commented Aug 24, 2023

k8s-ci-robot commented Aug 24, 2023

killianmuldoon commented Aug 24, 2023

k8s-infra-cherrypick-robot commented Aug 24, 2023

killianmuldoon commented Aug 9, 2023 •

edited

Loading

sbueringer Aug 24, 2023 •

edited

Loading