diff --git a/cmd/clusterctl/client/cluster/mover.go b/cmd/clusterctl/client/cluster/mover.go index 72e087f6487f..afed66962abd 100644 --- a/cmd/clusterctl/client/cluster/mover.go +++ b/cmd/clusterctl/client/cluster/mover.go @@ -1174,30 +1174,27 @@ func (o *objectMover) checkTargetProviders(toInventory InventoryClient) error { return kerrors.NewAggregate(errList) } +// patchTopologyManagedFields patches the managed fields of obj if parts of it are owned by the topology controller. +// This is necessary to ensure the managed fields created by the topology controller are still present and thus to +// prevent unnecessary machine rollouts. Without patching the managed fields, clusterctl would be the owner of the fields +// which would lead to co-ownership and also additional machine rollouts. func patchTopologyManagedFields(ctx context.Context, oldManagedFields []metav1.ManagedFieldsEntry, obj *unstructured.Unstructured, cTo client.Client) error { var containsTopologyManagedFields bool - // managedFields will contain the target managed fields obj gets patched to. - wantManagedFields := []metav1.ManagedFieldsEntry{} - + // Check if the object was owned by the topology controller. for _, m := range oldManagedFields { - if m.Manager == "manager" && m.Operation == metav1.ManagedFieldsOperationUpdate && m.Subresource == "" { - continue - } if m.Operation == metav1.ManagedFieldsOperationApply && m.Manager == structuredmerge.TopologyManagerName && m.Subresource == "" { containsTopologyManagedFields = true + break } - wantManagedFields = append(wantManagedFields, m) } - - // Return early if the managed fields had no entry from topology manager + // Return early if the object was not owned by the topology controller. if !containsTopologyManagedFields { return nil } - base := obj.DeepCopy() - obj.SetManagedFields(wantManagedFields) + obj.SetManagedFields(oldManagedFields) if err := cTo.Patch(ctx, obj, client.MergeFrom(base)); err != nil { return errors.Wrapf(err, "error patching managed fields %q %s/%s", diff --git a/docs/book/src/clusterctl/commands/alpha-topology-plan.md b/docs/book/src/clusterctl/commands/alpha-topology-plan.md index cbf297d85939..563729c8b6c6 100644 --- a/docs/book/src/clusterctl/commands/alpha-topology-plan.md +++ b/docs/book/src/clusterctl/commands/alpha-topology-plan.md @@ -26,18 +26,18 @@ the input should have all the objects needed.

Limitations

-While running in a production the topology controllers uses [Server Side Apply](https://kubernetes.io/docs/reference/using-api/server-side-apply/) -to proper manage other controllers co-authoring the same objects, but this kind of interactions can't be recreated +The topology controllers uses [Server Side Apply](https://kubernetes.io/docs/reference/using-api/server-side-apply/) +to support use cases where other controllers are co-authoring the same objects, but this kind of interactions can't be recreated in a dry-run scenario. -As a consequence Dry-Run can give some false positive/false negative when trying to have a preview of +As a consequence Dry-Run can give some false positives/false negatives when trying to have a preview of changes to a set of existing topology owned objects. In other worlds this limitation impacts all the use cases described below except for "Designing a new ClusterClass". More specifically: -- DryRun doesn't consider OpenAPI schema extension like +ListMap this can lead to false positive when topology - dry run is simulating a change to an existing slice (DryRun always revert external changes, like server side apply when +ListMap=atomic). -- DryRun doesn't consider existing metadata.managedFields, and this can lead to false negative when topology dry run +- DryRun doesn't consider OpenAPI schema extension like +ListMap this can lead to false positives when topology + dry run is simulating a change to an existing slice (DryRun always reverts external changes, like server side apply when +ListMap=atomic). +- DryRun doesn't consider existing metadata.managedFields, and this can lead to false negatives when topology dry run is simulating a change where a field is dropped from a template (DryRun always preserve dropped fields, like server side apply when the field has more than one manager). diff --git a/docs/book/src/developer/providers/v1.1-to-v1.2.md b/docs/book/src/developer/providers/v1.1-to-v1.2.md index 7bed86739d7d..0cc7db8b56e5 100644 --- a/docs/book/src/developer/providers/v1.1-to-v1.2.md +++ b/docs/book/src/developer/providers/v1.1-to-v1.2.md @@ -46,8 +46,8 @@ in ClusterAPI are kept in sync with the versions used by `sigs.k8s.io/controller ### Required API Changes for providers - ClusterClass and managed topologies are now using [Server Side Apply](https://kubernetes.io/docs/reference/using-api/server-side-apply/) - to proper manager other controllers like CAPA/CAPZ coauthoring slices, see [#6320](https://github.com/kubernetes-sigs/cluster-api/issues/6320). - In order to proper leverage this feature providers are required to add marker to their API types as described in + to properly manage other controllers like CAPA/CAPZ coauthoring slices, see [#6320](https://github.com/kubernetes-sigs/cluster-api/issues/6320). + In order to take advantage of this feature providers are required to add marker to their API types as described in [merge-strategy](https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy). NOTE: the change will cause a rollout on existing clusters created with ClusterClass @@ -61,7 +61,7 @@ Must be modified into: ```go // +optional -// +listType=Map +// +listType=map // +listMapKey=id Subnets Subnets `json:"subnets,omitempty" ``` diff --git a/docs/book/src/tasks/experimental-features/cluster-class/change-clusterclass.md b/docs/book/src/tasks/experimental-features/cluster-class/change-clusterclass.md index 43ca84f9a571..1cbaedee4b3e 100644 --- a/docs/book/src/tasks/experimental-features/cluster-class/change-clusterclass.md +++ b/docs/book/src/tasks/experimental-features/cluster-class/change-clusterclass.md @@ -176,17 +176,17 @@ owned objects in a Cluster. More specifically, the topology controller uses [Server Side Apply](https://kubernetes.io/docs/reference/using-api/server-side-apply/) to write/patch topology owned objects; using SSA allows other controllers to co-author the generated objects, -like e.g. integration info for the subnet list in CAPA. +like e.g. adding info for subnets in CAPA. -A corollary of the behaviour described above is that it is technically possible to change non-authoritative -fields in the object derived from the template in a specific Cluster, but we advise against using the possibility +A corollary of the behaviour described above is that it is technically possible to change fields in the object +which are not derived from the templates and patches, but we advise against using the possibility or making ad-hoc changes in generated objects unless otherwise needed for a workaround. It is always preferable to improve ClusterClasses by supporting new Cluster variants in a reusable way. \ No newline at end of file diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go index 970b7616cff7..ed09aa500aa0 100644 --- a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go +++ b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go @@ -144,7 +144,7 @@ func (h *serverSidePatchHelper) Patch(ctx context.Context) error { return h.client.Patch(ctx, h.modified, client.Apply, options...) } -// cleanupLegacyManagedFields cleanups managed filed management in place before introducing SSA. +// cleanupLegacyManagedFields cleanups managed field management in place before introducing SSA. // NOTE: this operation can trigger a machine rollout, but this is considered acceptable given that ClusterClass is still alpha // and SSA adoption align the topology controller with K8s recommended solution for many controllers authoring the same object. func cleanupLegacyManagedFields(obj *unstructured.Unstructured, c client.Client) error {