fix(import-populator): Make copying of annotations more robust #3388
Conversation
kubevirt-bot
added
release-note-none
|
@akalenyu PTAL, not sure this is 100% required. |
kubevirt-bot
added
the
needs-rebase
|
@akalenyu Do you think this is still needed? |
I think so, yeah |
Similar to 2fa5f27, make copying of annotations from a prime PVC to a target PVC more robust, by doing it before rebinding the PV from prime to target. This way we can ensure the annotations are already present once the PVC becomes ready. Signed-off-by: Felix Matouschek <[email protected]>
0xFelix
force-pushed
the
import-annotations
branch
from
be660ec to
3060548
Compare
kubevirt-bot
removed
the
needs-rebase
|
@akalenyu It looks better now, wdyt? |
|
/cc @alromeros @mhenriks wdyt? |
|
looks good I think but I'll let @alromeros be the judge |
| @@ -160,7 +160,10 @@ func (r *ImportPopulatorReconciler) reconcileTargetPVC(pvc, pvcPrime *corev1.Per | |||
| } | |||
|
|
|||
| if cc.IsPVCComplete(pvcPrime) && cc.IsUnbound(pvc) { | |||
| // Once the import is succeeded, we copy labels and rebind the PV from PVC to target PVC | |||
| // Once the import is succeeded, we copy annotations and labels and rebind the PV from PVC to target PVC | |||
There was a problem hiding this comment.
We will update the annotations again after the rebind, are we sure we need that? Having three updatePVCWithPVCPrimeAnnotations calls in the same function makes this harder to maintain. It's not this PR's fault though. Maybe we could move the two updateAnnotations and updateLabels just above the switch?
There was a problem hiding this comment.
Nevermind, UpdatesMultistageImportSucceeded updates the PVC prime annotations and we would need to call it afterwards (though that would leave us with two calls instead of three.)
There was a problem hiding this comment.
You are right that the order of calls to updatePVCWithPVCPrimeAnnotations is a bit weird here. But we need the call in L173, so that the annotations are also updated when no Pod condition above has matched. I tried without this call and this caused imports to fail because Pods in Pending condition never triggered the required annotation updates.
There was a problem hiding this comment.
What about having one at the top before entering the switch and another one after checking UpdatesMultistageImportSucceeded? That would leave us with two calls. Not a must though, I'll lgtm and we can consider improving this code a bit in the near future.
|
Thanks! |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mhenriks 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:
Approvers can indicate their approval by writing |
kubevirt-bot
added
the
approved
|
/retest-required |
|
/test pull-containerized-data-importer-fossa |
…irt#3388) Similar to 2fa5f27, make copying of annotations from a prime PVC to a target PVC more robust, by doing it before rebinding the PV from prime to target. This way we can ensure the annotations are already present once the PVC becomes ready. Signed-off-by: Felix Matouschek <[email protected]>
…irt#3388) Similar to 2fa5f27, make copying of annotations from a prime PVC to a target PVC more robust, by doing it before rebinding the PV from prime to target. This way we can ensure the annotations are already present once the PVC becomes ready. Signed-off-by: Felix Matouschek <[email protected]>
What this PR does / why we need it:
Similar to 2fa5f27, make copying of annotations from a prime PVC to a target PVC more robust, by moving it before rebinding the PV from prime to target. This way we can ensure the annotations are already present once the PVC becomes ready.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Release note: