Skip to content

Commit 882d94c

Browse files
sbueringersbueringer
committed
Log diffs for Cluster topology rollouts/patches
Signed-off-by: Stefan Büringer [email protected]
1 parent 25fa000 commit 882d94c

File tree

7 files changed

+117
-22
lines changed

7 files changed

+117
-22
lines changed

‎internal/controllers/topology/cluster/reconcile_state.go

+36-6
Original file line numberDiff line numberDiff line change
@@ -450,7 +450,12 @@ func (r *Reconciler) reconcileCluster(ctx context.Context, s *scope.Scope) error
450450
return nil
451451
}
452452

453-
log.Infof("Patching %s", tlog.KObj{Obj: s.Current.Cluster})
453+
changes := patchHelper.Changes()
454+
if len(changes) == 0 {
455+
log.Infof("Patching %s", tlog.KObj{Obj: s.Current.Cluster})
456+
} else {
457+
log.Infof("Patching %s, diff: %s", tlog.KObj{Obj: s.Current.Cluster}, changes)
458+
}
454459
if err := patchHelper.Patch(ctx); err != nil {
455460
return errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: s.Current.Cluster})
456461
}
@@ -751,7 +756,12 @@ func (r *Reconciler) updateMachineDeployment(ctx context.Context, s *scope.Scope
751756
return nil
752757
}
753758

754-
log.Infof("Patching %s", tlog.KObj{Obj: currentMD.Object})
759+
changes := patchHelper.Changes()
760+
if len(changes) == 0 {
761+
log.Infof("Patching %s", tlog.KObj{Obj: currentMD.Object})
762+
} else {
763+
log.Infof("Patching %s, diff: %s", tlog.KObj{Obj: currentMD.Object}, changes)
764+
}
755765
if err := patchHelper.Patch(ctx); err != nil {
756766
// Best effort cleanup of the InfrastructureMachineTemplate & BootstrapTemplate (only on template rotation).
757767
infrastructureMachineCleanupFunc()
@@ -1029,7 +1039,12 @@ func (r *Reconciler) updateMachinePool(ctx context.Context, s *scope.Scope, curr
10291039
return nil
10301040
}
10311041

1032-
log.Infof("Patching %s", tlog.KObj{Obj: currentMP.Object})
1042+
changes := patchHelper.Changes()
1043+
if len(changes) == 0 {
1044+
log.Infof("Patching %s", tlog.KObj{Obj: currentMP.Object})
1045+
} else {
1046+
log.Infof("Patching %s, diff: %s", tlog.KObj{Obj: currentMP.Object}, changes)
1047+
}
10331048
if err := patchHelper.Patch(ctx); err != nil {
10341049
return errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: currentMP.Object})
10351050
}
@@ -1173,7 +1188,12 @@ func (r *Reconciler) reconcileReferencedObject(ctx context.Context, in reconcile
11731188
return false, nil
11741189
}
11751190

1176-
log.Infof("Patching %s", tlog.KObj{Obj: in.desired})
1191+
changes := patchHelper.Changes()
1192+
if len(changes) == 0 {
1193+
log.Infof("Patching %s", tlog.KObj{Obj: in.desired})
1194+
} else {
1195+
log.Infof("Patching %s, diff: %s", tlog.KObj{Obj: in.desired}, changes)
1196+
}
11771197
if err := patchHelper.Patch(ctx); err != nil {
11781198
return false, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: in.current})
11791199
}
@@ -1260,7 +1280,12 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci
12601280
// If there are no changes in the spec, and thus only changes in metadata, instead of doing a full template
12611281
// rotation we patch the object in place. This avoids recreating machines.
12621282
if !patchHelper.HasSpecChanges() {
1263-
log.Infof("Patching %s", tlog.KObj{Obj: in.desired})
1283+
changes := patchHelper.Changes()
1284+
if len(changes) == 0 {
1285+
log.Infof("Patching %s", tlog.KObj{Obj: in.desired})
1286+
} else {
1287+
log.Infof("Patching %s, diff: %s", tlog.KObj{Obj: in.desired}, changes)
1288+
}
12641289
if err := patchHelper.Patch(ctx); err != nil {
12651290
return false, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: in.desired})
12661291
}
@@ -1275,7 +1300,12 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci
12751300
newName := names.SimpleNameGenerator.GenerateName(in.templateNamePrefix)
12761301
in.desired.SetName(newName)
12771302

1278-
log.Infof("Rotating %s, new name %s", tlog.KObj{Obj: in.current}, newName)
1303+
changes := patchHelper.Changes()
1304+
if len(changes) == 0 {
1305+
log.Infof("Rotating %s, new name %s", tlog.KObj{Obj: in.current}, newName)
1306+
} else {
1307+
log.Infof("Rotating %s, new name %s, diff: %s", tlog.KObj{Obj: in.current}, newName, changes)
1308+
}
12791309
log.Infof("Creating %s", tlog.KObj{Obj: in.desired})
12801310
helper, err := r.patchHelperFactory(ctx, nil, in.desired)
12811311
if err != nil {

‎internal/controllers/topology/cluster/structuredmerge/dryrun.go

+29-14
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ type dryRunSSAPatchInput struct {
4747
}
4848

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

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

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

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

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

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

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

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

155155
rawDiff, err := jsonpatch.CreateMergePatch(originalJSON, modifiedJSON)
156156
if err != nil {
157-
return false, false, err
157+
return false, false, nil, err
158158
}
159159

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

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

169+
var changes []byte
170+
if hasChanges {
171+
// Cleanup diff by dropping .metadata.managedFields.
172+
ssa.FilterIntent(&ssa.FilterIntentInput{
173+
Path: contract.Path{},
174+
Value: diff.Object,
175+
ShouldFilter: ssa.IsPathIgnored([]contract.Path{[]string{"metadata", "managedFields"}}),
176+
})
177+
178+
changes, err = json.Marshal(diff.Object)
179+
if err != nil {
180+
return false, false, nil, errors.Wrapf(err, "failed to marshal diff")
181+
}
182+
}
183+
169184
// If there is no diff add the request identifier to the cache.
170185
if !hasChanges {
171186
dryRunCtx.ssaCache.Add(requestIdentifier)
172187
}
173188

174-
return hasChanges, hasSpecChanges, nil
189+
return hasChanges, hasSpecChanges, changes, nil
175190
}
176191

177192
// cleanupManagedFieldsAndAnnotation adjusts the obj to remove the topology.cluster.x-k8s.io/dry-run

‎internal/controllers/topology/cluster/structuredmerge/interfaces.go

+3
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,9 @@ type PatchHelper interface {
3737
// HasSpecChanges return true if the modified object is generating spec changes vs the original object.
3838
HasSpecChanges() bool
3939

40+
// Changes returns the changes vs the original object.
41+
Changes() []byte
42+
4043
// Patch patches the given obj in the Kubernetes cluster.
4144
Patch(ctx context.Context) error
4245
}

‎internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go

+9-1
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ type serverSidePatchHelper struct {
3636
modified *unstructured.Unstructured
3737
hasChanges bool
3838
hasSpecChanges bool
39+
changes []byte
3940
}
4041

4142
// NewServerSidePatchHelper returns a new PatchHelper using server side apply.
@@ -89,12 +90,13 @@ func NewServerSidePatchHelper(ctx context.Context, original, modified client.Obj
8990
// Determine if the intent defined in the modified object is going to trigger
9091
// an actual change when running server side apply, and if this change might impact the object spec or not.
9192
var hasChanges, hasSpecChanges bool
93+
var changes []byte
9294
switch {
9395
case util.IsNil(original):
9496
hasChanges, hasSpecChanges = true, true
9597
default:
9698
var err error
97-
hasChanges, hasSpecChanges, err = dryRunSSAPatch(ctx, &dryRunSSAPatchInput{
99+
hasChanges, hasSpecChanges, changes, err = dryRunSSAPatch(ctx, &dryRunSSAPatchInput{
98100
client: c,
99101
ssaCache: ssaCache,
100102
originalUnstructured: originalUnstructured,
@@ -111,6 +113,7 @@ func NewServerSidePatchHelper(ctx context.Context, original, modified client.Obj
111113
modified: modifiedUnstructured,
112114
hasChanges: hasChanges,
113115
hasSpecChanges: hasSpecChanges,
116+
changes: changes,
114117
}, nil
115118
}
116119

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

125+
// Changes return the changes.
126+
func (h *serverSidePatchHelper) Changes() []byte {
127+
return h.changes
128+
}
129+
122130
// HasChanges return true if the patch has changes.
123131
func (h *serverSidePatchHelper) HasChanges() bool {
124132
return h.hasChanges

‎internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go

+16
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ func TestServerSideApply(t *testing.T) {
7777
g.Expect(err).ToNot(HaveOccurred())
7878
g.Expect(p0.HasChanges()).To(BeTrue())
7979
g.Expect(p0.HasSpecChanges()).To(BeTrue())
80+
g.Expect(p0.Changes()).To(BeNil()) // changes are expected to be nil on create.
8081
})
8182
t.Run("Server side apply detect changes on object creation (typed)", func(t *testing.T) {
8283
g := NewWithT(t)
@@ -88,6 +89,7 @@ func TestServerSideApply(t *testing.T) {
8889
g.Expect(err).ToNot(HaveOccurred())
8990
g.Expect(p0.HasChanges()).To(BeTrue())
9091
g.Expect(p0.HasSpecChanges()).To(BeTrue())
92+
g.Expect(p0.Changes()).To(BeNil()) // changes are expected to be nil on create.
9193
})
9294
t.Run("When creating an object using server side apply, it should track managed fields for the topology controller", func(t *testing.T) {
9395
g := NewWithT(t)
@@ -97,6 +99,7 @@ func TestServerSideApply(t *testing.T) {
9799
g.Expect(err).ToNot(HaveOccurred())
98100
g.Expect(p0.HasChanges()).To(BeTrue())
99101
g.Expect(p0.HasSpecChanges()).To(BeTrue())
102+
g.Expect(p0.Changes()).To(BeNil()) // changes are expected to be nil on create.
100103

101104
// Create the object using server side apply
102105
g.Expect(p0.Patch(ctx)).To(Succeed())
@@ -132,6 +135,7 @@ func TestServerSideApply(t *testing.T) {
132135
g.Expect(err).ToNot(HaveOccurred())
133136
g.Expect(p0.HasChanges()).To(BeFalse())
134137
g.Expect(p0.HasSpecChanges()).To(BeFalse())
138+
g.Expect(p0.Changes()).To(BeNil())
135139
})
136140

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

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

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

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

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

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

246256
t.Run("Another controller applies changes", func(t *testing.T) {
@@ -274,6 +284,7 @@ func TestServerSideApply(t *testing.T) {
274284
g.Expect(err).ToNot(HaveOccurred())
275285
g.Expect(p0.HasChanges()).To(BeFalse())
276286
g.Expect(p0.HasSpecChanges()).To(BeFalse())
287+
g.Expect(p0.Changes()).To(BeNil())
277288
})
278289

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

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

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

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

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

0 commit comments

Comments
 (0)