feat: Copy labels from source to DataSource #3377
Conversation
Extract the label copying logic from populator-base.go into the common pkg as CopyAllowedLabels func. Signed-off-by: Felix Matouschek <[email protected]>
kubevirt-bot
added
release-note
tests/datasource_test.go
Outdated
| Expect(ds.Labels).To(HaveKeyWithValue(testKubevirtIoKey, testKubevirtIoValue)) | ||
| Expect(ds.Labels).To(HaveKeyWithValue(testInstancetypeKubevirtIoKey, testInstancetypeKubevirtIoValue)) | ||
| Expect(ds.Labels).To(HaveKeyWithValue(testKubevirtIoKeyExisting, testKubevirtIoNewValueExisting)) |
There was a problem hiding this comment.
I think you're not guaranteed to have the labels when reaching "Ready=true". You could use eventually here
0xFelix
force-pushed
the
copy-labels
branch
2 times, most recently
from
f2478af to
0f46fec
Compare
|
/retest-required |
|
/test pull-containerized-data-importer-e2e-nfs |
|
/retest |
Make copying of labels from a prime PVC to the target PVC more robust, by moving it before rebinding the PV from prime to target. This way we can ensure the labels are already present once the PVC becomes ready. Signed-off-by: Felix Matouschek <[email protected]>
Do not pass labels from a DataImportCron to a DataSource in the dataimportcron-controller anymore. In the future this will be handled by the datasource-controller. Signed-off-by: Felix Matouschek <[email protected]>
Copy labels from the source of a DataSource to the labels of the DataSource in the datasource-controller. Signed-off-by: Felix Matouschek <[email protected]>
Add e2e tests that cover all scenarios where labels should be copied from the source of a DataSource to the DataSource itself. Signed-off-by: Felix Matouschek <[email protected]>
|
/test all |
|
/retest |
|
Fail of NFS lane was unrelated |
|
/test all |
|
Failures are unrelated to the changes, the cluster failed to come up. /retest |
0xFelix
force-pushed
the
copy-labels
branch
2 times, most recently
from
53051e8 to
f8b57cb
Compare
|
/retest |
|
/retest-required |
| return reconcile.Result{}, err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if pvcCopy, err = r.updatePVCWithPVCPrimeAnnotations(pvcCopy, pvcPrime, r.updateImportAnnotations); err != nil { | ||
| if _, err = r.updatePVCWithPVCPrimeAnnotations(pvcCopy, pvcPrime, r.updateImportAnnotations); err != nil { |
There was a problem hiding this comment.
Do we want the same treatment for annotations? #3381 (comment)
| updatedSnapshot := currentSnapshot.DeepCopy() | ||
| cc.CopyAllowedLabels(pvc.GetLabels(), updatedSnapshot, true) | ||
| if !reflect.DeepEqual(currentSnapshot, updatedSnapshot) { | ||
| if err := r.client.Update(ctx, updatedSnapshot); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this would solve the case for an existing snapshots with no labels, maybe we could just do something similar to 755fe0b by copying the labels from the data source to the snapshot. This would only be needed for a brief time window before the cron gets a sha update.
There was a problem hiding this comment.
Updated the approach to match 755fe0b, PTAL!
|
/test pull-cdi-goveralls |
|
/test pull-cdi-apidocs |
|
/test pull-containerized-data-importer-e2e-destructive |
| @@ -377,6 +379,11 @@ func (r *DataImportCronReconciler) update(ctx context.Context, dataImportCron *c | |||
| cc.AddAnnotation(snapshot, cc.AnnSourceVolumeMode, string(*volMode)) | |||
| } | |||
| } | |||
| // Copy labels found on dataSource to the existing snapshot in case of upgrades. | |||
| dataSource, err := r.getDataSource(ctx, dataImportCron) | |||
| if err == nil { | |||
There was a problem hiding this comment.
I think we're "forgetting" to requeue in case the GET fails for a random network err
When using VolumeSnapshots copy the labels found on the source PVC to the created or an existing VolumeSnapshot. Signed-off-by: Felix Matouschek <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akalenyu 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
|
@0xFelix: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/test pull-cdi-unit-test |
|
/test pull-containerized-data-importer-e2e-upg |
|
/lgtm |
What this PR does / why we need it:
Copy labels from the source of a DataSource (DV/PVC/VolumeSnapshot) to the labels of the DataSource. This allows to make use of labels which were originally defined as env vars on a containerdisk.
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: