✨ clusterctl move support for a cross namespace ClusterClass reference #11649

Danil-Grigorev · 2025-01-07T21:26:21Z

What this PR does / why we need it:

A follow-up on #11395 functionality, which integrates classNamespace field with clusterctl move.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Related to #5673

k8s-ci-robot · 2025-01-07T21:26:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign neolit123 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

elmiko

i think this generally makes sense, but i have a couple questions.

also, happy to see the docs update 🙏

elmiko · 2025-01-08T22:39:15Z

api/v1beta1/cluster_types.go

+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength=253
+	// +kubebuilder:validation:Pattern="^[a-z0-9](?:[-a-z0-9]*[a-z0-9])?(?:\\.[a-z0-9](?:[-a-z0-9]*[a-z0-9])?)*$"
+	ClassNamespace string `json:"classNamespace,omitempty"`


do we have consensus about adding this field to the API?

i mean, it seems necessary for this change, but i don't recall what we might have agreed upon in the office hours.

It’s actually a superset of #11352, I believe we have a consensus on the actual PR in the review #11352 (comment) (correct me if I’m wrong)

elmiko · 2025-01-08T22:41:09Z

api/v1beta1/index/cluster.go

@@ -55,3 +69,12 @@ func ClusterByClusterClassClassName(o client.Object) []string {
 	}
 	return nil
 }
+
+// ClusterByClusterClassClassNamespace contains the logic to index Clusters by ClusterClass namespace.
+func ClusterByClusterClassClassNamespace(o client.Object) []string {


does this need the stutter, could we just name it ClusterByClusterClassNamespace ?

or if we need the extra context could we name it something different like ClusterByClusterClassTopologyNamespace ?

Ah, good catch, I’ll update it. I think ClusterByClusterClassNamespace follows the naming pattern here.

elmiko

this is generally making sense to me, do we need to merge #11352 first?

Danil-Grigorev · 2025-01-22T11:20:25Z

/test pull-cluster-api-e2e-main

chrischdi · 2025-01-22T12:27:22Z

cmd/clusterctl/internal/test/fake_objects.go

@@ -109,6 +110,11 @@ func (f *FakeCluster) WithTopologyClass(class string) *FakeCluster {
 	return f
 }

+func (f *FakeCluster) WithTopologyClassWithin(namespace string) *FakeCluster {


Suggested change

func (f *FakeCluster) WithTopologyClassWithin(namespace string) *FakeCluster {

func (f *FakeCluster) WithTopologyClassNamespace(namespace string) *FakeCluster {

- Prevent duplicate CC creation in template processing - Test runtimextension integration - Add changes to CC rebase e2e test - Add a note CLUSTER_CLASS_NAMESPACE to the clusterctl contract Signed-off-by: Danil-Grigorev <[email protected]>

Signed-off-by: Danil-Grigorev <[email protected]>

k8s-ci-robot · 2025-01-23T17:24:53Z

@Danil-Grigorev: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-test-main	`36b7d8a`	link	true	`/test pull-cluster-api-test-main`

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-sigs/prow repository. I understand the commands that are listed here.

k8s-ci-robot · 2025-01-23T17:25:05Z

PR needs rebase.

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-sigs/prow repository.

fabriziopandini · 2025-01-23T17:28:53Z

@Danil-Grigorev I took a look at the PR, and I think we should improve it to ensure that we delete a ClusterClass a CC from the source cluster on if:

it is in the same namespace as the one we move &
if it is only referenced by Clusters in that namespace

In order to do so, I suggest to:

add two new attribute to node in the object graph, ShouldNotDelete, ShouldNotDeleteHierarchy
in objectGraph.Discovery, right after the call to o.setSoftOwnership, call a new func o.setShouldNotDelete
implemente objectGraph.setShouldNotDelete
- loop through all cc
  - if cc.ns != ns we are moving, set cc.ShouldNotDelete = true
  - if cc.ns == ns, chechk if there are clusters outside ns referencind this CCC. If yes, set cc.ShouldNotDelete = true
in objectGraph.setTenants(), set ShouldNotDeleteHierarchy (~same as we do for isGlobalHierarchy)
In objectMover.deleteSourceObject return early if ShouldNotDelete, ShouldNotDeleteHierarchy

fabriziopandini · 2025-01-23T17:39:28Z

cmd/clusterctl/client/cluster/objectgraph.go

+
+					// Set isGlobalHierarchy on clusterClass if is referenced from a different namespace
+					if classNamespace != cluster.identity.Namespace {
+						clusterClass.isGlobalHierarchy = true


While this probably achieve the goal, it seems a little bit hacky:

isGlobal and isGlobalHierarchy are meant to track if "object is a global resource (no namespace).", which this is not the case for cc

you are using isGlobalHierarchy without having an isGlobal object on top

Please take a look at the alternative solution I proposed above, happy to chat about it if you want

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

k8s-ci-robot requested review from elmiko and enxebre January 7, 2025 21:26

k8s-ci-robot added do-not-merge/needs-area PR is missing an area label size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 7, 2025

elmiko reviewed Jan 8, 2025

View reviewed changes

Danil-Grigorev force-pushed the class-namespace-move-support branch 2 times, most recently from bc1b8fc to 8f8f47b Compare January 9, 2025 21:08

k8s-ci-robot added area/clusterclass Issues or PRs related to clusterclass and removed do-not-merge/needs-area PR is missing an area label labels Jan 9, 2025

elmiko reviewed Jan 9, 2025

View reviewed changes

Danil-Grigorev force-pushed the class-namespace-move-support branch from 8f8f47b to 2a98610 Compare January 22, 2025 11:02

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 22, 2025

chrischdi reviewed Jan 22, 2025

View reviewed changes

Danil-Grigorev force-pushed the class-namespace-move-support branch from 2a98610 to ba93019 Compare January 22, 2025 12:29

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 22, 2025

Danil-Grigorev force-pushed the class-namespace-move-support branch 2 times, most recently from 866abc0 to 4e5295b Compare January 22, 2025 14:03

Danil-Grigorev force-pushed the class-namespace-move-support branch from 4e5295b to f14a2f0 Compare January 23, 2025 16:46

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 23, 2025

sbueringer mentioned this pull request Jan 23, 2025

✨ Extend scale test and make ExtensionConfig name in RuntimeSDK test configurable #11667

Merged

Implement clusterctl move support for cross-ns CC

36b7d8a

Signed-off-by: Danil-Grigorev <[email protected]>

Danil-Grigorev force-pushed the class-namespace-move-support branch from f14a2f0 to 36b7d8a Compare January 23, 2025 17:14

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

fabriziopandini reviewed Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ clusterctl move support for a cross namespace ClusterClass reference #11649

✨ clusterctl move support for a cross namespace ClusterClass reference #11649

Danil-Grigorev commented Jan 7, 2025 •

edited

Loading

k8s-ci-robot commented Jan 7, 2025

elmiko left a comment

elmiko Jan 8, 2025

Danil-Grigorev Jan 9, 2025

elmiko Jan 8, 2025

Danil-Grigorev Jan 9, 2025

elmiko left a comment

Danil-Grigorev commented Jan 22, 2025

chrischdi Jan 22, 2025

k8s-ci-robot commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

fabriziopandini commented Jan 23, 2025

fabriziopandini Jan 23, 2025 •

edited

Loading

	func (f FakeCluster) WithTopologyClassWithin(namespace string) FakeCluster {
	func (f FakeCluster) WithTopologyClassNamespace(namespace string) FakeCluster {

✨ clusterctl move support for a cross namespace ClusterClass reference #11649

Are you sure you want to change the base?

✨ clusterctl move support for a cross namespace ClusterClass reference #11649

Conversation

Danil-Grigorev commented Jan 7, 2025 • edited Loading

k8s-ci-robot commented Jan 7, 2025

elmiko left a comment

Choose a reason for hiding this comment

elmiko Jan 8, 2025

Choose a reason for hiding this comment

Danil-Grigorev Jan 9, 2025

Choose a reason for hiding this comment

elmiko Jan 8, 2025

Choose a reason for hiding this comment

Danil-Grigorev Jan 9, 2025

Choose a reason for hiding this comment

elmiko left a comment

Choose a reason for hiding this comment

Danil-Grigorev commented Jan 22, 2025

chrischdi Jan 22, 2025

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

fabriziopandini commented Jan 23, 2025

fabriziopandini Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Danil-Grigorev commented Jan 7, 2025 •

edited

Loading

fabriziopandini Jan 23, 2025 •

edited

Loading