🌱 Optimise patch calls#13367
Conversation
k8s-ci-robot
added
size/L
|
/hold Need rebase |
k8s-ci-robot
added
the
do-not-merge/hold
fabriziopandini
force-pushed
the
patch-improvements
branch
from
1b3d1d4 to
7a7828a
Compare
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
| kcpRef := *metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind(kubeadmControlPlaneKind)) | ||
| for _, m := range machines { | ||
| // No op if OwnerReferences is set and up to date. | ||
| if util.HasExactOwnerRef(m.OwnerReferences, kcpRef) { |
There was a problem hiding this comment.
Is it really correct to skip all this code if m has the ownerRef? (especially including adoptOwnedSecrets)
I would move this down to l.1591
There was a problem hiding this comment.
I moved the code below, but I also think that most of this code can be entirely dropped.
This code was introduced by #2489 before the 0.4 release. when KCP was introduced and it was necessary to handle the adoption of standalone machines and related PKI material.
We are now well past this transition, and I think that we can safely drop the transition of PKI material.
We can debate about adoption.
My opinion is that the only part that should remain is EnforceOwnerRef
There was a problem hiding this comment.
I think I agree, let's just make this a separate effort.
In general, I'm just not entirely sure how much of this kind of code is for legacy code paths and how much is to restore ownerRef chains after backup/restore. (we have similar code in MS IIRC)
So I would like to be careful with changes like this and tackle them in a separate effort
There was a problem hiding this comment.
💯 agreed on considering this as a follow up (low priority)
| @@ -1647,29 +1640,25 @@ func (r *KubeadmControlPlaneReconciler) ensureCertificatesOwnerRef(ctx context.C | |||
| continue | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
To me it looks like this code is not equivalent.
Previously we also did something for secrets that did not have ClusterSecretType. Is this change intentional? (probably yes, but wanted to double check)
There was a problem hiding this comment.
I reviewed this change to reduce the diff from original
(not worth the effort to risk regressions for micro optimizations)
There was a problem hiding this comment.
Thx! It was just pretty tough to review for me
|
/test pull-cluster-api-test-main |
fabriziopandini
force-pushed
the
patch-improvements
branch
from
7a7828a to
ae8635e
Compare
fabriziopandini
added
the
tide/merge-method-squash
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. DetailsGit tree hash: 8a6174d80da4c2d1521e6453d5f5dd82605f882f |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
3 participants
What this PR does / why we need it:
This PR drops usage of patch helper when we are modifying only few fields, not including conditions.
Also, avoid patching entirely when the object is not changed,.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Part of #13305
/area util