Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🌱 Log diffs for Cluster topology rollouts/patches #10690

Merged
merged 1 commit into from
May 31, 2024
Merged

🌱 Log diffs for Cluster topology rollouts/patches #10690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 36 additions & 6 deletions internal/controllers/topology/cluster/reconcile_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,12 @@ func (r *Reconciler) reconcileCluster(ctx context.Context, s *scope.Scope) error
return nil
}

log.Infof("Patching %s", tlog.KObj{Obj: s.Current.Cluster})
changes := patchHelper.Changes()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has there been consideration to let this changes log be part of the patchHelper.Patch func?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I think it would work for almost all cases. Except the rotation. For the rotation case we create a new patchHelper because we eventually decide (after diffing) that we have to create an entirely new object.

So that patchHelper is not aware of the old object / the diff.

if len(changes) == 0 {
log.Infof("Patching %s", tlog.KObj{Obj: s.Current.Cluster})
} else {
log.Infof("Patching %s, diff: %s", tlog.KObj{Obj: s.Current.Cluster}, changes)
}
if err := patchHelper.Patch(ctx); err != nil {
return errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: s.Current.Cluster})
}
Expand Down Expand Up @@ -751,7 +756,12 @@ func (r *Reconciler) updateMachineDeployment(ctx context.Context, s *scope.Scope
return nil
}

log.Infof("Patching %s", tlog.KObj{Obj: currentMD.Object})
changes := patchHelper.Changes()
if len(changes) == 0 {
log.Infof("Patching %s", tlog.KObj{Obj: currentMD.Object})
} else {
log.Infof("Patching %s, diff: %s", tlog.KObj{Obj: currentMD.Object}, changes)
}
fabriziopandini marked this conversation as resolved.
Show resolved Hide resolved
if err := patchHelper.Patch(ctx); err != nil {
// Best effort cleanup of the InfrastructureMachineTemplate & BootstrapTemplate (only on template rotation).
infrastructureMachineCleanupFunc()
Expand Down Expand Up @@ -1029,7 +1039,12 @@ func (r *Reconciler) updateMachinePool(ctx context.Context, s *scope.Scope, curr
return nil
}

log.Infof("Patching %s", tlog.KObj{Obj: currentMP.Object})
changes := patchHelper.Changes()
if len(changes) == 0 {
log.Infof("Patching %s", tlog.KObj{Obj: currentMP.Object})
} else {
log.Infof("Patching %s, diff: %s", tlog.KObj{Obj: currentMP.Object}, changes)
}
if err := patchHelper.Patch(ctx); err != nil {
return errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: currentMP.Object})
}
Expand Down Expand Up @@ -1173,7 +1188,12 @@ func (r *Reconciler) reconcileReferencedObject(ctx context.Context, in reconcile
return false, nil
}

log.Infof("Patching %s", tlog.KObj{Obj: in.desired})
changes := patchHelper.Changes()
if len(changes) == 0 {
log.Infof("Patching %s", tlog.KObj{Obj: in.desired})
} else {
log.Infof("Patching %s, diff: %s", tlog.KObj{Obj: in.desired}, changes)
}
if err := patchHelper.Patch(ctx); err != nil {
return false, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: in.current})
}
Expand Down Expand Up @@ -1260,7 +1280,12 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci
// If there are no changes in the spec, and thus only changes in metadata, instead of doing a full template
// rotation we patch the object in place. This avoids recreating machines.
if !patchHelper.HasSpecChanges() {
log.Infof("Patching %s", tlog.KObj{Obj: in.desired})
changes := patchHelper.Changes()
if len(changes) == 0 {
log.Infof("Patching %s", tlog.KObj{Obj: in.desired})
} else {
log.Infof("Patching %s, diff: %s", tlog.KObj{Obj: in.desired}, changes)
}
if err := patchHelper.Patch(ctx); err != nil {
return false, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: in.desired})
}
Expand All @@ -1275,7 +1300,12 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci
newName := names.SimpleNameGenerator.GenerateName(in.templateNamePrefix)
in.desired.SetName(newName)

log.Infof("Rotating %s, new name %s", tlog.KObj{Obj: in.current}, newName)
changes := patchHelper.Changes()
if len(changes) == 0 {
log.Infof("Rotating %s, new name %s", tlog.KObj{Obj: in.current}, newName)
} else {
log.Infof("Rotating %s, new name %s, diff: %s", tlog.KObj{Obj: in.current}, newName, changes)
}
log.Infof("Creating %s", tlog.KObj{Obj: in.desired})
helper, err := r.patchHelperFactory(ctx, nil, in.desired)
if err != nil {
Expand Down
43 changes: 29 additions & 14 deletions internal/controllers/topology/cluster/structuredmerge/dryrun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ type dryRunSSAPatchInput struct {
}

// dryRunSSAPatch uses server side apply dry run to determine if the operation is going to change the actual object.
func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool, bool, error) {
func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool, bool, []byte, error) {
// Compute a request identifier.
// The identifier is unique for a specific request to ensure we don't have to re-run the request
// once we found out that it would not produce a diff.
Expand All @@ -56,13 +56,13 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool,
// This ensures that we re-run the request as soon as either original or modified changes.
requestIdentifier, err := ssa.ComputeRequestIdentifier(dryRunCtx.client.Scheme(), dryRunCtx.originalUnstructured, dryRunCtx.modifiedUnstructured)
if err != nil {
return false, false, err
return false, false, nil, err
}

// Check if we already ran this request before by checking if the cache already contains this identifier.
// Note: We only add an identifier to the cache if the result of the dry run was no diff.
if exists := dryRunCtx.ssaCache.Has(requestIdentifier); exists {
return false, false, nil
return false, false, nil, nil
}

// For dry run we use the same options as for the intent but with adding metadata.managedFields
Expand All @@ -74,17 +74,17 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool,

// Add TopologyDryRunAnnotation to notify validation webhooks to skip immutability checks.
if err := unstructured.SetNestedField(dryRunCtx.originalUnstructured.Object, "", "metadata", "annotations", clusterv1.TopologyDryRunAnnotation); err != nil {
return false, false, errors.Wrap(err, "failed to add topology dry-run annotation to original object")
return false, false, nil, errors.Wrap(err, "failed to add topology dry-run annotation to original object")
}
if err := unstructured.SetNestedField(dryRunCtx.modifiedUnstructured.Object, "", "metadata", "annotations", clusterv1.TopologyDryRunAnnotation); err != nil {
return false, false, errors.Wrap(err, "failed to add topology dry-run annotation to modified object")
return false, false, nil, errors.Wrap(err, "failed to add topology dry-run annotation to modified object")
}

// Do a server-side apply dry-run with modifiedUnstructured to get the updated object.
err = dryRunCtx.client.Patch(ctx, dryRunCtx.modifiedUnstructured, client.Apply, client.DryRunAll, client.FieldOwner(TopologyManagerName), client.ForceOwnership)
if err != nil {
// This catches errors like metadata.uid changes.
return false, false, errors.Wrap(err, "server side apply dry-run failed for modified object")
return false, false, nil, errors.Wrap(err, "server side apply dry-run failed for modified object")
}

// Do a server-side apply dry-run with originalUnstructured to ensure the latest defaulting is applied.
Expand All @@ -109,7 +109,7 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool,
dryRunCtx.originalUnstructured.SetManagedFields(nil)
err = dryRunCtx.client.Patch(ctx, dryRunCtx.originalUnstructured, client.Apply, client.DryRunAll, client.FieldOwner(TopologyManagerName), client.ForceOwnership)
if err != nil {
return false, false, errors.Wrap(err, "server side apply dry-run failed for original object")
return false, false, nil, errors.Wrap(err, "server side apply dry-run failed for original object")
}
// Restore managed fields.
dryRunCtx.originalUnstructured.SetManagedFields(originalUnstructuredManagedFieldsBeforeSSA)
Expand All @@ -124,7 +124,7 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool,
// Please note that if other managers made changes to fields that we care about and thus ownership changed,
// this would affect our managed fields as well and we would still detect it by diffing our managed fields.
if err := cleanupManagedFieldsAndAnnotation(dryRunCtx.modifiedUnstructured); err != nil {
return false, false, errors.Wrap(err, "failed to filter topology dry-run annotation on modified object")
return false, false, nil, errors.Wrap(err, "failed to filter topology dry-run annotation on modified object")
}

// Also run the function for the originalUnstructured to remove the managedField
Expand All @@ -135,7 +135,7 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool,
// Please note that if other managers made changes to fields that we care about and thus ownership changed,
// this would affect our managed fields as well and we would still detect it by diffing our managed fields.
if err := cleanupManagedFieldsAndAnnotation(dryRunCtx.originalUnstructured); err != nil {
return false, false, errors.Wrap(err, "failed to filter topology dry-run annotation on original object")
return false, false, nil, errors.Wrap(err, "failed to filter topology dry-run annotation on original object")
}

// Drop the other fields which are not part of our intent.
Expand All @@ -145,33 +145,48 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool,
// Compare the output of dry run to the original object.
originalJSON, err := json.Marshal(dryRunCtx.originalUnstructured)
if err != nil {
return false, false, err
return false, false, nil, err
}
modifiedJSON, err := json.Marshal(dryRunCtx.modifiedUnstructured)
if err != nil {
return false, false, err
return false, false, nil, err
}

rawDiff, err := jsonpatch.CreateMergePatch(originalJSON, modifiedJSON)
if err != nil {
return false, false, err
return false, false, nil, err
}

// Determine if there are changes to the spec and object.
diff := &unstructured.Unstructured{}
if err := json.Unmarshal(rawDiff, &diff.Object); err != nil {
return false, false, err
return false, false, nil, err
}

hasChanges := len(diff.Object) > 0
_, hasSpecChanges := diff.Object["spec"]

var changes []byte
if hasChanges {
// Cleanup diff by dropping .metadata.managedFields.
ssa.FilterIntent(&ssa.FilterIntentInput{
Path: contract.Path{},
Value: diff.Object,
ShouldFilter: ssa.IsPathIgnored([]contract.Path{[]string{"metadata", "managedFields"}}),
})

changes, err = json.Marshal(diff.Object)
if err != nil {
return false, false, nil, errors.Wrapf(err, "failed to marshal diff")
}
}

// If there is no diff add the request identifier to the cache.
if !hasChanges {
dryRunCtx.ssaCache.Add(requestIdentifier)
}

return hasChanges, hasSpecChanges, nil
return hasChanges, hasSpecChanges, changes, nil
}

// cleanupManagedFieldsAndAnnotation adjusts the obj to remove the topology.cluster.x-k8s.io/dry-run
Expand Down
3 changes: 3 additions & 0 deletions internal/controllers/topology/cluster/structuredmerge/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ type PatchHelper interface {
// HasSpecChanges return true if the modified object is generating spec changes vs the original object.
HasSpecChanges() bool

// Changes returns the changes vs the original object.
Changes() []byte

// Patch patches the given obj in the Kubernetes cluster.
Patch(ctx context.Context) error
}
10 changes: 9 additions & 1 deletion internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ type serverSidePatchHelper struct {
modified *unstructured.Unstructured
hasChanges bool
hasSpecChanges bool
changes []byte
}

// NewServerSidePatchHelper returns a new PatchHelper using server side apply.
Expand Down Expand Up @@ -89,12 +90,13 @@ func NewServerSidePatchHelper(ctx context.Context, original, modified client.Obj
// Determine if the intent defined in the modified object is going to trigger
// an actual change when running server side apply, and if this change might impact the object spec or not.
var hasChanges, hasSpecChanges bool
var changes []byte
switch {
case util.IsNil(original):
hasChanges, hasSpecChanges = true, true
default:
var err error
hasChanges, hasSpecChanges, err = dryRunSSAPatch(ctx, &dryRunSSAPatchInput{
hasChanges, hasSpecChanges, changes, err = dryRunSSAPatch(ctx, &dryRunSSAPatchInput{
client: c,
ssaCache: ssaCache,
originalUnstructured: originalUnstructured,
Expand All @@ -111,6 +113,7 @@ func NewServerSidePatchHelper(ctx context.Context, original, modified client.Obj
modified: modifiedUnstructured,
hasChanges: hasChanges,
hasSpecChanges: hasSpecChanges,
changes: changes,
}, nil
}

Expand All @@ -119,6 +122,11 @@ func (h *serverSidePatchHelper) HasSpecChanges() bool {
return h.hasSpecChanges
}

// Changes return the changes.
func (h *serverSidePatchHelper) Changes() []byte {
return h.changes
}

// HasChanges return true if the patch has changes.
func (h *serverSidePatchHelper) HasChanges() bool {
return h.hasChanges
Expand Down
16 changes: 16 additions & 0 deletions internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeTrue())
g.Expect(p0.HasSpecChanges()).To(BeTrue())
g.Expect(p0.Changes()).To(BeNil()) // changes are expected to be nil on create.
})
t.Run("Server side apply detect changes on object creation (typed)", func(t *testing.T) {
g := NewWithT(t)
Expand All @@ -88,6 +89,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeTrue())
g.Expect(p0.HasSpecChanges()).To(BeTrue())
g.Expect(p0.Changes()).To(BeNil()) // changes are expected to be nil on create.
})
t.Run("When creating an object using server side apply, it should track managed fields for the topology controller", func(t *testing.T) {
g := NewWithT(t)
Expand All @@ -97,6 +99,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeTrue())
g.Expect(p0.HasSpecChanges()).To(BeTrue())
g.Expect(p0.Changes()).To(BeNil()) // changes are expected to be nil on create.

// Create the object using server side apply
g.Expect(p0.Patch(ctx)).To(Succeed())
Expand Down Expand Up @@ -132,6 +135,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeFalse())
g.Expect(p0.HasSpecChanges()).To(BeFalse())
g.Expect(p0.Changes()).To(BeNil())
})

t.Run("Server side apply patch helper discard changes in not allowed fields, e.g. status", func(t *testing.T) {
Expand All @@ -149,6 +153,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeFalse())
g.Expect(p0.HasSpecChanges()).To(BeFalse())
g.Expect(p0.Changes()).To(BeNil())
})

t.Run("Server side apply patch helper detect changes", func(t *testing.T) {
Expand All @@ -166,6 +171,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeTrue())
g.Expect(p0.HasSpecChanges()).To(BeTrue())
g.Expect(p0.Changes()).To(Equal([]byte(`{"spec":{"bar":"changed"}}`)))
})

t.Run("Server side apply patch helper detect changes impacting only metadata.labels", func(t *testing.T) {
Expand All @@ -183,6 +189,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeTrue())
g.Expect(p0.HasSpecChanges()).To(BeFalse())
g.Expect(p0.Changes()).To(Equal([]byte(`{"metadata":{"labels":{"foo":"changed"}}}`)))
})

t.Run("Server side apply patch helper detect changes impacting only metadata.annotations", func(t *testing.T) {
Expand All @@ -200,6 +207,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeTrue())
g.Expect(p0.HasSpecChanges()).To(BeFalse())
g.Expect(p0.Changes()).To(Equal([]byte(`{"metadata":{"annotations":{"foo":"changed"}}}`)))
})

t.Run("Server side apply patch helper detect changes impacting only metadata.ownerReferences", func(t *testing.T) {
Expand All @@ -224,6 +232,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeTrue())
g.Expect(p0.HasSpecChanges()).To(BeFalse())
g.Expect(p0.Changes()).To(Equal([]byte(`{"metadata":{"ownerReferences":[{"apiVersion":"foo/v1alpha1","kind":"foo","name":"foo","uid":"foo"}]}}`)))
})

t.Run("Server side apply patch helper discard changes in ignore paths", func(t *testing.T) {
Expand All @@ -241,6 +250,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeFalse())
g.Expect(p0.HasSpecChanges()).To(BeFalse())
g.Expect(p0.Changes()).To(BeNil())
})

t.Run("Another controller applies changes", func(t *testing.T) {
Expand Down Expand Up @@ -274,6 +284,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeFalse())
g.Expect(p0.HasSpecChanges()).To(BeFalse())
g.Expect(p0.Changes()).To(BeNil())
})

t.Run("Topology controller reconcile again with no changes on topology managed fields", func(t *testing.T) {
Expand All @@ -290,6 +301,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeFalse())
g.Expect(p0.HasSpecChanges()).To(BeFalse())
g.Expect(p0.Changes()).To(BeNil())

// Change the object using server side apply
g.Expect(p0.Patch(ctx)).To(Succeed())
Expand Down Expand Up @@ -337,6 +349,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeTrue())
g.Expect(p0.HasSpecChanges()).To(BeTrue())
g.Expect(p0.Changes()).To(Equal([]byte(`{"spec":{"controlPlaneEndpoint":{"host":"changed"}}}`)))

// Create the object using server side apply
g.Expect(p0.Patch(ctx)).To(Succeed())
Expand Down Expand Up @@ -375,6 +388,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeTrue())
g.Expect(p0.HasSpecChanges()).To(BeFalse())
g.Expect(p0.Changes()).To(Equal([]byte(`{}`))) // Note: metadata.managedFields have been removed from the diff to reduce log verbosity.

// Create the object using server side apply
g.Expect(p0.Patch(ctx)).To(Succeed())
Expand Down Expand Up @@ -415,6 +429,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeTrue())
g.Expect(p0.HasSpecChanges()).To(BeTrue())
g.Expect(p0.Changes()).To(Equal([]byte(`{"spec":{"bar":"changed-by-topology-controller"}}`)))

// Create the object using server side apply
g.Expect(p0.Patch(ctx)).To(Succeed())
Expand Down Expand Up @@ -463,6 +478,7 @@ func TestServerSideApply(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())
g.Expect(p0.HasChanges()).To(BeFalse())
g.Expect(p0.HasSpecChanges()).To(BeFalse())
g.Expect(p0.Changes()).To(BeNil())
})
t.Run("Error on object which has another uid due to immutability", func(t *testing.T) {
g := NewWithT(t)
Expand Down
Loading