Remove optimistic lock on Patch

[juldrixx]

Q	A
Bug fix?	no
New feature?	no
API breaks?	no
Deprecations?	no
Related tickets	#346
License	Apache 2.0

What's in this PR?

Removal of the Optimistic Lock on Patch.

Why?

With it, the change from Update to Patch, doesn't improve the error linked to the update on older resource version.
But without it, the error disappears.

And this should prevent the dataflow deployment duplication error.

In the Operator SDK documentation, they seems to favorite this: https://sdk.operatorframework.io/docs/building-operators/golang/references/client/#patch.

Checklist

• Implementation tested
• Error handling code meets the guideline
• Logging code meets the guideline
• User guide and development docs updated (if needed)
• Append changelog with changes

[mh013370]

Interesting - my apologies on this. I thought we'd want to always operate on the latest version of the objects.

[juldrixx]

Interesting - my apologies on this. I thought we'd want to always operate on the latest version of the objects.

I thought to. But It seems that with the Optimistic Lock, it does the same thing than an Update. But with a simple Patch, it should only update what we want to update no? Even if its not the latest version of the resource, the Patch will only change what has change, no?

[mh013370]

Interesting - my apologies on this. I thought we'd want to always operate on the latest version of the objects.

I thought to. But It seems that with the Optimistic Lock, it does the same thing than an Update. But with a simple Patch, it should only update what we want to update no? Even if its not the latest version of the resource, the Patch will only change what has change, no?

This is true with the exception of lists:

// When using MergeFrom, existing lists will be completely replaced by new lists.

source:
https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.14.6/pkg/client#MergeFrom

The work around is to use strategic merge, but those don't work with CRDs. So this is something we need to be conscious of in the operator -- lists will be replaced so we need to compute entirely new lists and not just additions/subtractions. I had to do this in the NifiNodeGroupAutoscaler:

https://github.com/konpyutaika/nifikop/pull/350/files#diff-f1c55d9dd87a65e56685a8997559770de8e2bbe3658d53cf934ad7d0d29f7c6eR134

[juldrixx]

Interesting - my apologies on this. I thought we'd want to always operate on the latest version of the objects.

I thought to. But It seems that with the Optimistic Lock, it does the same thing than an Update. But with a simple Patch, it should only update what we want to update no? Even if its not the latest version of the resource, the Patch will only change what has change, no?

This is true with the exception of lists:

// When using MergeFrom, existing lists will be completely replaced by new lists.

source: https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.14.6/pkg/client#MergeFrom

The work around is to use strategic merge, but those don't work with CRDs. So this is something we need to be conscious of in the operator -- lists will be replaced so we need to compute entirely new lists and not just additions/subtractions. I had to do this in the NifiNodeGroupAutoscaler:

https://github.com/konpyutaika/nifikop/pull/350/files#diff-f1c55d9dd87a65e56685a8997559770de8e2bbe3658d53cf934ad7d0d29f7c6eR134

Should I put back clusterPatch := runtimeClient.MergeFromWithOptions(cluster.DeepCopy(), runtimeClient.MergeFromWithOptimisticLock{}) in the NifiNodeGroupAutoscaler?

[mh013370]

Interesting - my apologies on this. I thought we'd want to always operate on the latest version of the objects.

I thought to. But It seems that with the Optimistic Lock, it does the same thing than an Update. But with a simple Patch, it should only update what we want to update no? Even if its not the latest version of the resource, the Patch will only change what has change, no?

This is true with the exception of lists:

// When using MergeFrom, existing lists will be completely replaced by new lists.

source: https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.14.6/pkg/client#MergeFrom
The work around is to use strategic merge, but those don't work with CRDs. So this is something we need to be conscious of in the operator -- lists will be replaced so we need to compute entirely new lists and not just additions/subtractions. I had to do this in the NifiNodeGroupAutoscaler:
https://github.com/konpyutaika/nifikop/pull/350/files#diff-f1c55d9dd87a65e56685a8997559770de8e2bbe3658d53cf934ad7d0d29f7c6eR134

Should I put back clusterPatch := runtimeClient.MergeFromWithOptions(cluster.DeepCopy(), runtimeClient.MergeFromWithOptimisticLock{}) in the NifiNodeGroupAutoscaler?

I only put that in there to guarantee its patching the latest version of objects. Since we didn't see any issues with that (right?), perhaps we do leave in the optimistic lock.

[mh013370]

Interesting - my apologies on this. I thought we'd want to always operate on the latest version of the objects.

I thought to. But It seems that with the Optimistic Lock, it does the same thing than an Update. But with a simple Patch, it should only update what we want to update no? Even if its not the latest version of the resource, the Patch will only change what has change, no?

This is true with the exception of lists:

// When using MergeFrom, existing lists will be completely replaced by new lists.

source: https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.14.6/pkg/client#MergeFrom
The work around is to use strategic merge, but those don't work with CRDs. So this is something we need to be conscious of in the operator -- lists will be replaced so we need to compute entirely new lists and not just additions/subtractions. I had to do this in the NifiNodeGroupAutoscaler:
https://github.com/konpyutaika/nifikop/pull/350/files#diff-f1c55d9dd87a65e56685a8997559770de8e2bbe3658d53cf934ad7d0d29f7c6eR134

Should I put back clusterPatch := runtimeClient.MergeFromWithOptions(cluster.DeepCopy(), runtimeClient.MergeFromWithOptimisticLock{}) in the NifiNodeGroupAutoscaler?

I only put that in there to guarantee its patching the latest version of objects. Since we didn't see any issues with that (right?), perhaps we do leave in the optimistic lock.

[juldrixx]

Interesting - my apologies on this. I thought we'd want to always operate on the latest version of the objects.

I thought to. But It seems that with the Optimistic Lock, it does the same thing than an Update. But with a simple Patch, it should only update what we want to update no? Even if its not the latest version of the resource, the Patch will only change what has change, no?

This is true with the exception of lists:

// When using MergeFrom, existing lists will be completely replaced by new lists.

source: https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.14.6/pkg/client#MergeFrom
The work around is to use strategic merge, but those don't work with CRDs. So this is something we need to be conscious of in the operator -- lists will be replaced so we need to compute entirely new lists and not just additions/subtractions. I had to do this in the NifiNodeGroupAutoscaler:
https://github.com/konpyutaika/nifikop/pull/350/files#diff-f1c55d9dd87a65e56685a8997559770de8e2bbe3658d53cf934ad7d0d29f7c6eR134

Should I put back clusterPatch := runtimeClient.MergeFromWithOptions(cluster.DeepCopy(), runtimeClient.MergeFromWithOptimisticLock{}) in the NifiNodeGroupAutoscaler?

I only put that in there to guarantee its patching the latest version of objects. Since we didn't see any issues with that (right?), perhaps we do leave in the optimistic lock.

I don't think so, I will put it back.

[juldrixx]

I'm going to merge and we should monitor for any issues in the next release.