Closed juldrixx closed 6 months ago
Interesting - my apologies on this. I thought we'd want to always operate on the latest version of the objects.
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?
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
:
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
:
Should I put back clusterPatch := runtimeClient.MergeFromWithOptions(cluster.DeepCopy(), runtimeClient.MergeFromWithOptimisticLock{})
in the NifiNodeGroupAutoscaler
?
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-f1c55d9dd87a65e56685a8997559770de8e2bbe3658d53cf934ad7d0d29f7c6eR134Should I put back
clusterPatch := runtimeClient.MergeFromWithOptions(cluster.DeepCopy(), runtimeClient.MergeFromWithOptimisticLock{})
in theNifiNodeGroupAutoscaler
?
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.
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-f1c55d9dd87a65e56685a8997559770de8e2bbe3658d53cf934ad7d0d29f7c6eR134Should I put back
clusterPatch := runtimeClient.MergeFromWithOptions(cluster.DeepCopy(), runtimeClient.MergeFromWithOptimisticLock{})
in theNifiNodeGroupAutoscaler
?
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.
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-f1c55d9dd87a65e56685a8997559770de8e2bbe3658d53cf934ad7d0d29f7c6eR134Should I put back
clusterPatch := runtimeClient.MergeFromWithOptions(cluster.DeepCopy(), runtimeClient.MergeFromWithOptimisticLock{})
in theNifiNodeGroupAutoscaler
?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.
I'm going to merge and we should monitor for any issues in the next release.
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