Skip to content

Commit 841bdbb

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #5416 from fabriziopandini/topology-should-upgrade-template-inplace-when-possible
🌱 Topology controller upgrades templates in place when there are only metadata changes
2 parents 2110151 + f185d8c commit 841bdbb

File tree

4 files changed

+142
-58
lines changed

4 files changed

+142
-58
lines changed

‎controllers/topology/internal/mergepatch/mergepatch.go

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,9 @@ type Helper struct {
4444

4545
// patch holds the merge patch in json format.
4646
patch []byte
47+
48+
// hasSpecChanges documents if the patch impacts the object spec
49+
hasSpecChanges bool
4750
}
4851

4952
// NewHelper will return a patch that yields the modified document when applied to the original document.
@@ -81,25 +84,27 @@ func NewHelper(original, modified client.Object, c client.Client, opts ...Helper
8184

8285
// We should consider only the changes that are relevant for the topology, removing
8386
// changes for metadata fields computed by the system or changes to the status.
84-
patch, err := filterPatch(rawPatch, allowedPaths, helperOptions.ignorePaths)
87+
patch, hasSpecChanges, err := filterPatch(rawPatch, allowedPaths, helperOptions.ignorePaths)
8588
if err != nil {
8689
return nil, errors.Wrap(err, "failed to remove fields merge patch")
8790
}
8891

8992
return &Helper{
90-
client: c,
91-
patch: patch,
92-
original: original,
93+
client: c,
94+
patch: patch,
95+
hasSpecChanges: hasSpecChanges,
96+
original: original,
9397
}, nil
9498
}
9599

96-
// filterPatch removes from the patch diffs not in the allowed paths.
97-
func filterPatch(patch []byte, allowedPaths, ignorePaths []contract.Path) ([]byte, error) {
100+
// filterPatch removes from the patch diffs not in the allowed paths;
101+
// it also return a flag indicating if the resulting patch has spec changes or not.
102+
func filterPatch(patch []byte, allowedPaths, ignorePaths []contract.Path) ([]byte, bool, error) {
98103
// converts the patch into a Map
99104
patchMap := make(map[string]interface{})
100-
err := json.Unmarshal(patch, &patchMap)
101-
if err != nil {
102-
return nil, errors.Wrap(err, "failed to unmarshal merge patch")
105+
106+
if err := json.Unmarshal(patch, &patchMap); err != nil {
107+
return nil, false, errors.Wrap(err, "failed to unmarshal merge patch")
103108
}
104109

105110
// Removes from diffs everything not in the allowed paths.
@@ -110,12 +115,15 @@ func filterPatch(patch []byte, allowedPaths, ignorePaths []contract.Path) ([]byt
110115
removePath(patchMap, path)
111116
}
112117

118+
// check if the changes impact the spec field.
119+
hasSpecChanges := patchMap["spec"] != nil
120+
113121
// converts Map back into the patch
114-
patch, err = json.Marshal(&patchMap)
122+
filteredPatch, err := json.Marshal(&patchMap)
115123
if err != nil {
116-
return nil, errors.Wrap(err, "failed to marshal merge patch")
124+
return nil, false, errors.Wrap(err, "failed to marshal merge patch")
117125
}
118-
return patch, nil
126+
return filteredPatch, hasSpecChanges, nil
119127
}
120128

121129
// filterPatch removes from the patchMap diffs not in the allowed paths.
@@ -185,6 +193,11 @@ func removePath(patchMap map[string]interface{}, path contract.Path) {
185193
}
186194
}
187195

196+
// HasSpecChanges return true if the patch has changes to the spec field.
197+
func (h *Helper) HasSpecChanges() bool {
198+
return h.hasSpecChanges
199+
}
200+
188201
// HasChanges return true if the patch has changes.
189202
func (h *Helper) HasChanges() bool {
190203
return !bytes.Equal(h.patch, []byte("{}"))

‎controllers/topology/internal/mergepatch/mergepatch_test.go

Lines changed: 80 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -26,17 +26,18 @@ import (
2626

2727
func TestNewHelper(t *testing.T) {
2828
tests := []struct {
29-
name string
30-
original *unstructured.Unstructured // current
31-
modified *unstructured.Unstructured // desired
32-
options []HelperOption
33-
wantHasChanges bool
34-
wantPatch []byte
29+
name string
30+
original *unstructured.Unstructured // current
31+
modified *unstructured.Unstructured // desired
32+
options []HelperOption
33+
wantHasChanges bool
34+
wantHasSpecChanges bool
35+
wantPatch []byte
3536
}{
3637
// Field both in original and in modified --> align to modified
3738

3839
{
39-
name: "Field both in original and in modified, no-op when equal",
40+
name: "Field (spec) both in original and in modified, no-op when equal",
4041
original: &unstructured.Unstructured{ // current
4142
Object: map[string]interface{}{
4243
"spec": map[string]interface{}{
@@ -51,8 +52,9 @@ func TestNewHelper(t *testing.T) {
5152
},
5253
},
5354
},
54-
wantHasChanges: false,
55-
wantPatch: []byte("{}"),
55+
wantHasChanges: false,
56+
wantHasSpecChanges: false,
57+
wantPatch: []byte("{}"),
5658
},
5759
{
5860
name: "Field both in original and in modified, align to modified when different",
@@ -70,8 +72,33 @@ func TestNewHelper(t *testing.T) {
7072
},
7173
},
7274
},
73-
wantHasChanges: true,
74-
wantPatch: []byte("{\"spec\":{\"foo\":\"bar\"}}"),
75+
wantHasChanges: true,
76+
wantHasSpecChanges: true,
77+
wantPatch: []byte("{\"spec\":{\"foo\":\"bar\"}}"),
78+
},
79+
{
80+
name: "Field (metadata.label) both in original and in modified, align to modified when different",
81+
original: &unstructured.Unstructured{ // current
82+
Object: map[string]interface{}{
83+
"metadata": map[string]interface{}{
84+
"labels": map[string]interface{}{
85+
"foo": "bar-changed",
86+
},
87+
},
88+
},
89+
},
90+
modified: &unstructured.Unstructured{ // desired
91+
Object: map[string]interface{}{
92+
"metadata": map[string]interface{}{
93+
"labels": map[string]interface{}{
94+
"foo": "bar",
95+
},
96+
},
97+
},
98+
},
99+
wantHasChanges: true,
100+
wantHasSpecChanges: false,
101+
wantPatch: []byte("{\"metadata\":{\"labels\":{\"foo\":\"bar\"}}}"),
75102
},
76103
{
77104
name: "Nested field both in original and in modified, no-op when equal",
@@ -97,8 +124,9 @@ func TestNewHelper(t *testing.T) {
97124
},
98125
},
99126
},
100-
wantHasChanges: false,
101-
wantPatch: []byte("{}"),
127+
wantHasChanges: false,
128+
wantHasSpecChanges: false,
129+
wantPatch: []byte("{}"),
102130
},
103131
{
104132
name: "Nested field both in original and in modified, align to modified when different",
@@ -124,8 +152,9 @@ func TestNewHelper(t *testing.T) {
124152
},
125153
},
126154
},
127-
wantHasChanges: true,
128-
wantPatch: []byte("{\"spec\":{\"template\":{\"spec\":{\"A\":\"A\"}}}}"),
155+
wantHasChanges: true,
156+
wantHasSpecChanges: true,
157+
wantPatch: []byte("{\"spec\":{\"template\":{\"spec\":{\"A\":\"A\"}}}}"),
129158
},
130159
{
131160
name: "Value of type map, enforces entries from modified, preserve entries only in original",
@@ -151,8 +180,9 @@ func TestNewHelper(t *testing.T) {
151180
},
152181
},
153182
},
154-
wantHasChanges: true,
155-
wantPatch: []byte("{\"spec\":{\"map\":{\"A\":\"A\",\"C\":\"C\"}}}"),
183+
wantHasChanges: true,
184+
wantHasSpecChanges: true,
185+
wantPatch: []byte("{\"spec\":{\"map\":{\"A\":\"A\",\"C\":\"C\"}}}"),
156186
},
157187
{
158188
name: "Value of type Array or Slice, align to modified",
@@ -178,8 +208,9 @@ func TestNewHelper(t *testing.T) {
178208
},
179209
},
180210
},
181-
wantHasChanges: true,
182-
wantPatch: []byte("{\"spec\":{\"slice\":[\"A\",\"B\",\"C\"]}}"),
211+
wantHasChanges: true,
212+
wantHasSpecChanges: true,
213+
wantPatch: []byte("{\"spec\":{\"slice\":[\"A\",\"B\",\"C\"]}}"),
183214
},
184215

185216
// Field only in modified (not existing in original) --> align to modified
@@ -196,8 +227,9 @@ func TestNewHelper(t *testing.T) {
196227
},
197228
},
198229
},
199-
wantHasChanges: true,
200-
wantPatch: []byte("{\"spec\":{\"foo\":\"bar\"}}"),
230+
wantHasChanges: true,
231+
wantHasSpecChanges: true,
232+
wantPatch: []byte("{\"spec\":{\"foo\":\"bar\"}}"),
201233
},
202234
{
203235
name: "Nested field only in modified, align to modified",
@@ -215,8 +247,9 @@ func TestNewHelper(t *testing.T) {
215247
},
216248
},
217249
},
218-
wantHasChanges: true,
219-
wantPatch: []byte("{\"spec\":{\"template\":{\"spec\":{\"A\":\"A\"}}}}"),
250+
wantHasChanges: true,
251+
wantHasSpecChanges: true,
252+
wantPatch: []byte("{\"spec\":{\"template\":{\"spec\":{\"A\":\"A\"}}}}"),
220253
},
221254

222255
// Field only in original (not existing in modified) --> preserve original
@@ -233,8 +266,9 @@ func TestNewHelper(t *testing.T) {
233266
modified: &unstructured.Unstructured{ // desired
234267
Object: map[string]interface{}{},
235268
},
236-
wantHasChanges: false,
237-
wantPatch: []byte("{}"),
269+
wantHasChanges: false,
270+
wantHasSpecChanges: false,
271+
wantPatch: []byte("{}"),
238272
},
239273
{
240274
name: "Nested field only in original, align to modified",
@@ -252,8 +286,9 @@ func TestNewHelper(t *testing.T) {
252286
modified: &unstructured.Unstructured{ // desired
253287
Object: map[string]interface{}{},
254288
},
255-
wantHasChanges: false,
256-
wantPatch: []byte("{}"),
289+
wantHasChanges: false,
290+
wantHasSpecChanges: false,
291+
wantPatch: []byte("{}"),
257292
},
258293

259294
// Diff for metadata fields computed by the system or in status are discarded
@@ -277,11 +312,12 @@ func TestNewHelper(t *testing.T) {
277312
},
278313
},
279314
},
280-
wantHasChanges: false,
281-
wantPatch: []byte("{}"),
315+
wantHasChanges: false,
316+
wantHasSpecChanges: false,
317+
wantPatch: []byte("{}"),
282318
},
283319
{
284-
name: "Relevant Diff are preserved",
320+
name: "Relevant Diff for metadata (labels and annotations) are preserved",
285321
original: &unstructured.Unstructured{ // current
286322
Object: map[string]interface{}{},
287323
},
@@ -297,8 +333,9 @@ func TestNewHelper(t *testing.T) {
297333
},
298334
},
299335
},
300-
wantHasChanges: true,
301-
wantPatch: []byte("{\"metadata\":{\"annotations\":{\"foo\":\"bar\"},\"labels\":{\"foo\":\"bar\"}}}"),
336+
wantHasChanges: true,
337+
wantHasSpecChanges: false,
338+
wantPatch: []byte("{\"metadata\":{\"annotations\":{\"foo\":\"bar\"},\"labels\":{\"foo\":\"bar\"}}}"),
302339
},
303340

304341
// Ignore fields
@@ -318,9 +355,10 @@ func TestNewHelper(t *testing.T) {
318355
},
319356
},
320357
},
321-
options: []HelperOption{IgnorePaths{contract.Path{"spec", "controlPlaneEndpoint"}}},
322-
wantHasChanges: false,
323-
wantPatch: []byte("{}"),
358+
options: []HelperOption{IgnorePaths{contract.Path{"spec", "controlPlaneEndpoint"}}},
359+
wantHasChanges: false,
360+
wantHasSpecChanges: false,
361+
wantPatch: []byte("{}"),
324362
},
325363

326364
// More tests
@@ -343,8 +381,9 @@ func TestNewHelper(t *testing.T) {
343381
},
344382
},
345383
},
346-
wantHasChanges: false,
347-
wantPatch: []byte("{}"),
384+
wantHasChanges: false,
385+
wantHasSpecChanges: false,
386+
wantPatch: []byte("{}"),
348387
},
349388
{
350389
name: "Many changes",
@@ -365,8 +404,9 @@ func TestNewHelper(t *testing.T) {
365404
},
366405
},
367406
},
368-
wantHasChanges: true,
369-
wantPatch: []byte("{\"spec\":{\"B\":\"B\"}}"),
407+
wantHasChanges: true,
408+
wantHasSpecChanges: true,
409+
wantPatch: []byte("{\"spec\":{\"B\":\"B\"}}"),
370410
},
371411
}
372412
for _, tt := range tests {
@@ -377,6 +417,7 @@ func TestNewHelper(t *testing.T) {
377417
g.Expect(err).ToNot(HaveOccurred())
378418

379419
g.Expect(patch.HasChanges()).To(Equal(tt.wantHasChanges))
420+
g.Expect(patch.HasSpecChanges()).To(Equal(tt.wantHasSpecChanges))
380421
g.Expect(patch.patch).To(Equal(tt.wantPatch))
381422
})
382423
}

‎controllers/topology/reconcile_state.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,16 @@ func (r *ClusterReconciler) reconcileReferencedTemplate(ctx context.Context, in
372372
return cleanupFunc, nil
373373
}
374374

375+
// If there are no changes in the spec, and thus only changes in metadata, instead of doing a full template
376+
// rotation we patch the object in place. This avoids recreating machines.
377+
if !patchHelper.HasSpecChanges() {
378+
log.Infof("Patching %s", tlog.KObj{Obj: in.desired})
379+
if err := patchHelper.Patch(ctx); err != nil {
380+
return nil, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: in.desired})
381+
}
382+
return cleanupFunc, nil
383+
}
384+
375385
// Create the new template.
376386

377387
// NOTE: it is required to assign a new name, because during compute the desired object name is enforced to be equal to the current one.

0 commit comments

Comments
 (0)