Fix reusing merge objects for patch without copying

[RedbackThomson]

I was seeing an issue where there was a delta in the spec, but it was never being applied back to the object. I tracked down that the latest object returned from sdkFind was correct, but the patch was not being applied.

When using the method that patches both spec and status, changes to the latest were being applied in the spec, but not in the status. The root cause of this is that the client.MergeFrom is overriding the status fields when merging the spec with desired. Therefore after patching the spec desired == latest. This patch ensures that we copy the latest object as part of patching so as to not override changes when using client.MergeFrom.

[vijtrip2]

I intentionally did not use the DeepCopyObject() here and I hope this does not introduce the exception that underlying object has been changed.

The doc of this method says that pass the pointer of the object so that changes applied from patch can be reflected back. If we use .DeepCopyObject() then latest will not have the changes reflected back in latest object.

if this new way works, I may have misunderstood the working. Can you share your delve logs to help my understanding?

Also you will have to update the unit tests to incorporate this new DeepCopyObject method.

[a-hilaly]

Do we really need to DeepCopyObject() the latest here?

[vijtrip2]

/approve

[a-hilaly]

@RedbackThomson nice detective job!

/lgtm
/approve

[ack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, RedbackThomson, vijtrip2

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [A-Hilaly,RedbackThomson,vijtrip2]

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

[jaypipes]

I learned from @RedbackThomson that he had had private conversations with @vijtrip2 and @a-hilaly about this where he explained that the reason his code works is because in the original code, the runtime object pointer that is passed to patchResourceMetadataAndSpec ends up being modified by MergeFrom so that the Status struct of both the latest and desired runtime object pointers are identical, which causes patchResourceStatus to not actually patch anything.

I'm saying this PR needs changes because this is NON-obvious and therefore requires a code comment explaining exactly WHY we use DeepCopyObject here. Otherwise, I guarantee that in 3 months, when one of us goes looking at this code, one of us will be like "oh, this can be sped up by removing DeepCopyObject and boom, we'll be back in the same bug situation.

[RedbackThomson]

Here are logs which prove the effect I've described above:

Printing on the first reconcile loop

> github.com/aws-controllers-k8s/runtime/pkg/runtime.(*resourceReconciler).patchResourceMetadataAndSpec() /home/nithomso/go/src/github.com/aws-controllers-k8s/runtime/pkg/runtime/reconciler.go:343 (hits goroutine(233):2 total:2) (PC: 0x1bb8807)
   338:                 rlog.Debug("no difference found between metadata and spec for desired and latest object.")
   339:                 return nil
   340:         }
   341:
   342:         rlog.Enter("kc.Patch (metadata + spec)")
=> 343:         err = r.kc.Patch(
   344:                 ctx,
   345:                 latest.RuntimeObject(),
   346:                 client.MergeFrom(desired.RuntimeObject()),
   347:         )
   348:         rlog.Exit("kc.Patch (metadata + spec)", err)
(dlv) print latest.ko.Status
github.com/aws-controllers-k8s/dynamodb-controller/apis/v1alpha1.TableStatus {
        ...
        TableStatus: *"CREATING",}
(dlv) continue
...
> github.com/aws-controllers-k8s/runtime/pkg/runtime.(*resourceReconciler).patchResourceStatus() /home/nithomso/go/src/github.com/aws-controllers-k8s/runtime/pkg/runtime/reconciler.go:376 (hits goroutine(233):1 total:1) (PC: 0x1bb8d27)
   371:         }
   372:         if !changedStatus {
   373:                 return nil
   374:         }
   375:         rlog.Enter("kc.Patch (status)")
=> 376:         err = r.kc.Status().Patch(
   377:                 ctx,
   378:                 latest.RuntimeObject().DeepCopyObject(),
   379:                 client.MergeFrom(desired.RuntimeObject()),
   380:         )
   381:         rlog.Exit("kc.Patch (status)", err)
(dlv) print latest.ko.Status
github.com/aws-controllers-k8s/dynamodb-controller/apis/v1alpha1.TableStatus {
        ...
        TableStatus: *"CREATING",}

latest.ko.Status shows TableStatus: "CREATING" in both methods.

Continuing to the next reconcile loop:

   338:                 rlog.Debug("no difference found between metadata and spec for desired and latest object.")
   339:                 return nil
   340:         }
   341:
   342:         rlog.Enter("kc.Patch (metadata + spec)")
=> 343:         err = r.kc.Patch(
   344:                 ctx,
   345:                 latest.RuntimeObject(),
   346:                 client.MergeFrom(desired.RuntimeObject()),
   347:         )
   348:         rlog.Exit("kc.Patch (metadata + spec)", err)
(dlv) print latest.ko.Status
github.com/aws-controllers-k8s/dynamodb-controller/apis/v1alpha1.TableStatus {
        ...
        TableStatus: *"ACTIVE",}
(dlv) continue
...
   371:         }
   372:         if !changedStatus {
   373:                 return nil
   374:         }
   375:         rlog.Enter("kc.Patch (status)")
=> 376:         err = r.kc.Status().Patch(
   377:                 ctx,
   378:                 latest.RuntimeObject().DeepCopyObject(),
   379:                 client.MergeFrom(desired.RuntimeObject()),
   380:         )
   381:         rlog.Exit("kc.Patch (status)", err)
(dlv) print latest.ko.Status
github.com/aws-controllers-k8s/dynamodb-controller/apis/v1alpha1.TableStatus {
        ...
        TableStatus: *"CREATING",}

Here the latest.ko.Status object in patchSpecAndMetadata is TableStatus: "ACTIVE" but in patchStatus is TableStatus: "CREATING", still.

After the changes in this PR the result is:

   338:                 rlog.Debug("no difference found between metadata and spec for desired and latest object.")
   339:                 return nil
   340:         }
   341:
   342:         rlog.Enter("kc.Patch (metadata + spec)")
=> 343:         err = r.kc.Patch(
   344:                 ctx,
   345:                 latest.RuntimeObject().DeepCopyObject(),
   346:                 client.MergeFrom(desired.RuntimeObject().DeepCopyObject()),
   347:         )
   348:         rlog.Exit("kc.Patch (metadata + spec)", err)
(dlv) print latest.ko.Status
github.com/aws-controllers-k8s/dynamodb-controller/apis/v1alpha1.TableStatus {
        ...
        TableStatus: *"ACTIVE",}
(dlv) continue
...
   371:         }
   372:         if !changedStatus {
   373:                 return nil
   374:         }
   375:         rlog.Enter("kc.Patch (status)")
=> 376:         err = r.kc.Status().Patch(
   377:                 ctx,
   378:                 latest.RuntimeObject().DeepCopyObject(),
   379:                 client.MergeFrom(desired.RuntimeObject().DeepCopyObject()),
   380:         )
   381:         rlog.Exit("kc.Patch (status)", err)
(dlv) print latest.ko.Status
github.com/aws-controllers-k8s/dynamodb-controller/apis/v1alpha1.TableStatus {
        ...
        TableStatus: *"ACTIVE",}

Now you can see latest.ko.Status has TableStatus: "ACTIVE" in both patchSpecAndMetadata and patchStatus.