-
Notifications
You must be signed in to change notification settings - Fork 258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Delete old version DVs with DIC GC #2749
Delete old version DVs with DIC GC #2749
Conversation
Hi @ido106. Thanks for your PR. I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
4edfb75
to
a12b74a
Compare
a12b74a
to
804ceba
Compare
22c28e3
to
8faf5e4
Compare
/ok-to-test |
If there is a bug opened for this, would be great to include it under |
There is only a Jira card |
Yup, if you look at that issue it translates to a bugzilla |
Ok, I added the Bugzilla bug number |
return err | ||
} | ||
|
||
for _, dv := range dvList.Items { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why don't we apply here the logic used above?
if len(pvcList.Items) > maxImports {
sort.Slice(pvcList.Items, func(i, j int) bool {
return pvcList.Items[i].Annotations[AnnLastUseTime] > pvcList.Items[j].Annotations[AnnLastUseTime]
})
for _, pvc := range pvcList.Items[maxImports:] {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I did that, but then we decided to delete all those DVs because they are all old.
@arnongilboa
Maybe we'll change it back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we would like to delete these (very) old ones and no need to sort, but we can sure add
if len(dvList.Items) > maxImports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
} | ||
|
||
if pvc.Labels[common.DataImportCronLabel] != cronName { | ||
if err := r.deleteDvPvc(ctx, dv.Name, dv.Namespace); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe log the deletion of these dvs/pvcs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
9796b25
to
91a73d7
Compare
/retest |
Great, thanks ! |
2135092
to
00cfb22
Compare
Is 7406 cool now ? |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good just not sure if you meant to keep the separate test
tests/dataimportcron_test.go
Outdated
@@ -367,6 +369,120 @@ var _ = Describe("DataImportCron", func() { | |||
Entry("[test_id:8266] succeed deleting error DVs when importing new ones", false, true, 2, cdiv1.DataImportCronSourceFormatPvc), | |||
) | |||
|
|||
It("[test_id:XXXX] Succeed garbage collecting old version DVs", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can delete this test now that we are doing the check in 7406
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I was just waiting to see if 7406
is good before deleting the old test
tests/dataimportcron_test.go
Outdated
Eventually(func() bool { | ||
_, err := f.CdiClient.CdiV1beta1().DataVolumes(ns).Get(context.TODO(), oldDvName, metav1.GetOptions{}) | ||
return err != nil | ||
}, dataImportCronTimeout, pollingInterval).Should(Equal(true), "Garbage collection failed cleaning old DV") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit, if you want, can do something like this:
Eventually(func() error {
_, err := f.CdiClient.CdiV1beta1().DataVolumes(ns).Get(context.TODO(), oldDvName, metav1.GetOptions{})
return err
}, dataImportCronTimeout, pollingInterval).Should(Satisfy(errors.IsNotFound), "Garbage collection failed cleaning old DV")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I will fix it
f635c1a
to
fc7884f
Compare
/approve |
/cc @arnongilboa |
[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 |
} | ||
|
||
if pvc.Labels[common.DataImportCronLabel] != cronName { | ||
r.log.Info("Deleting dv/pvc", "name", pvc.Name, "pvc.uid", pvc.UID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this log line should be a bit different from the one above, e.g. "Deleting old version dv/pvc"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
cron = newDataImportCron(cronName) | ||
importsToKeep := int32(1) | ||
cron.Spec.ImportsToKeep = &importsToKeep | ||
cron.Annotations[AnnSourceDesiredDigest] = testDigest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need to set digest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, we don't need it. Probably left over from the old test I wrote.
Fixed
tests/dataimportcron_test.go
Outdated
@@ -48,6 +48,7 @@ var _ = Describe("DataImportCron", func() { | |||
ns string | |||
scName string | |||
originalProfileSpec *cdiv1.StorageProfileSpec | |||
oldDvName = "src-garbage-0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oldDvName should move into the function, as nobody else is using it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
tests/dataimportcron_test.go
Outdated
for i := 1; i <= garbageSources; i++ { | ||
srcName := fmt.Sprintf("src-garbage-%d", i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I like this naming hack, oldDvName should have a better name than "src-garbage-0", e.g. "old-version-dv", and the loop can be reverted to 0-based.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, no problem. Fixed
fc7884f
to
8d42850
Compare
Signed-off-by: Ido Aharon <iaharon@redhat.com>
8d42850
to
795c34a
Compare
Do you have any more comments? |
/lgtm |
Manual backport of kubevirt#2749 Signed-off-by: Ido Aharon <iaharon@redhat.com>
Manual backport of #2749 Signed-off-by: Ido Aharon <iaharon@redhat.com> Co-authored-by: Ido Aharon <iaharon@redhat.com>
Signed-off-by: Ido Aharon iaharon@redhat.com
What this PR does / why we need it:
In the old versions, the garbage collection label was added to DV only. In the new version, the GC looks for this label in the PVC, so it does not find it and does not delete the DV's from the old versions
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 bz #2209969
Special notes for your reviewer:
Release note: