Convert DownloadRequest resource/controller to kubebuilder #3004

carlisia · 2020-10-12T17:09:13Z

This closes #2656.

nrb · 2020-10-27T18:07:14Z

@carlisia I did a quick pass over the code, but is there anything in particular preventing it from moving out of draft right now?

carlisia · 2020-10-27T18:09:44Z

@carlisia I did a quick pass over the code, but is there anything in particular preventing it from moving out of draft right now?

Yes, I need to fix this test: https://github.com/vmware-tanzu/velero/pull/3004/files#diff-6d5dc1afa6af05350b9f38368bde033f93c478b86304922796d2e6eedcbf1336

carlisia · 2020-11-03T23:33:40Z

Note: @nrb agreed that the test pkg/cmd/util/downloadrequest/downloadrequest_test.go should be removed. To be sure, the controller has thorough testing.

nrb

A couple questions, and I think one of the parameters passed in to the Stream function is no longer used now.

config/rbac/role.yaml

pkg/cmd/util/downloadrequest/downloadrequest.go

carlisia · 2020-12-03T00:09:56Z

Comments so far have been addressed, ready for review.

nrb

Dismissing request for changes, but looks like a rebase is necessary.

carlisia · 2020-12-09T00:24:24Z

Rebased, ready for review.

Changes addressed.

nrb · 2020-12-11T18:12:27Z

@vmware-tanzu/velero-maintainers Please queue this for review before break

dsu-igeek

I just had the places where I think we want to use WithStack or Wrap on the returned error, otherwise looks good.

pkg/controller/download_request_controller.go

zubron

I know you've shown me some of the work to use kubebuilder/controller runtime before, but seeing the changes in a PR make it really clear how much nicer this approach is 😄

Most of the comments I've left are cleanup/suggestions and could be addressed in a follow up. The only thing I'd really like to check before merging is the change to the requeue in the BSL controller. I don't know if that's an intentional change, and if so, what is the impact.

pkg/cmd/util/downloadrequest/downloadrequest.go

pkg/controller/backup_storage_location_controller.go

pkg/controller/download_request_controller.go

pkg/controller/download_request_controller_test.go

Signed-off-by: Carlisia <[email protected]>

carlisia · 2021-02-19T04:43:40Z

Addressed the corrections (thank you!).

Note: I don't know what happened but needed to do a rebase, and the actual commit was "eaten", but the changes were pushed.

Ready for review.

zubron

Thanks, @carlisia! There are just a couple of changes that need to be made in one of the test cases but it's good otherwise 👍

zubron · 2021-02-22T15:52:54Z

pkg/controller/download_request_controller_test.go

+			downloadRequest:      builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindBackupLog, "a-backup-20170912150214").Result(),
+			backup:               defaultBackup(),
+			expired:              true,
+			expectedReconcileErr: "backups.velero.io \"a-backup-20170912150214\" not found",


Like the previous test where the expired DownloadRequest was deleted, we shouldn't expect an error here. There are a few points in the main test code where we only set the expiration on the DownloadRequest when it is Processed when it should be applied to any expired request.

See my other two separate comments, they address both points here.

zubron · 2021-02-22T15:54:40Z

pkg/controller/download_request_controller_test.go

-			if tc.downloadRequest != nil && tc.downloadRequest.Status.Phase == velerov1api.DownloadRequestPhaseProcessed {
-				if tc.expired {
-					tc.downloadRequest.Status.Expiration = &metav1.Time{Time: harness.controller.clock.Now().Add(-1 * time.Minute)}
+			if test.downloadRequest != nil && test.downloadRequest.Status != (velerov1api.DownloadRequestStatus{}) && test.downloadRequest.Status.Phase == velerov1api.DownloadRequestPhaseProcessed {


I think this whole outer check can be removed as we need to set the expiration for all requests.

The expiration gets set only in the same block of code where we update the request status to processed (

velero/pkg/controller/download_request_controller.go

Line 113 in 1c536ac

downloadRequest.Status.Expiration = &metav1.Time{Time: r.Clock.Now().Add(persistence.DownloadURLTTL)}

).

This means, not all requests have an expiration. A request that has the phase "" or New will not have its expiration set until it gets processed and returned successfully from the reconcile loop.

Which is to say, only requests that have been successfully processed will have it's expiration set.

Hm. You are right, of course. Nvm what I said. Let me re-evaluate these tests.

zubron · 2021-02-22T15:55:28Z

pkg/controller/download_request_controller_test.go

-				if tc.expired {
-					assert.True(t, apierrors.IsNotFound(err))
+			if test.expired {
+				if test.downloadRequest.Status.Phase == velerov1api.DownloadRequestPhaseProcessed {


If test.expired is true, then we should always expect the err to be a NotFound error.

Ah. I was thinking this test block was a mess. Thanks to your other comment, I realize a bug in the test: a request cannot be both expired and have a phase other than processed, and the last test case if braking this rule. Fixing this.

Signed-off-by: Carlisia <[email protected]>

carlisia · 2021-02-22T22:00:54Z

Thanks for the very attentive reviews @zubron. I was making some incorrect assumptions. All your points were valid and I think I addressed everything.

Ready for review.

zubron

Thanks for fixing the tests - LGTM! :)

…nzu#3004) * Migrate DownloadRequest types to kubebuilder Signed-off-by: Carlisia <[email protected]> * Migrate controller to kubebuilder Signed-off-by: Carlisia <[email protected]> * Migrate download request cli to kubebuilder Signed-off-by: Carlisia <[email protected]> * Format w make update Signed-off-by: Carlisia <[email protected]> * Remove download file Signed-off-by: Carlisia <[email protected]> * Remove kubebuilder from backup/restore apis Signed-off-by: Carlisia <[email protected]> * Fix test description Signed-off-by: Carlisia <[email protected]> * Import cleanups Signed-off-by: Carlisia <[email protected]> * Refactor for controller runtime version update Signed-off-by: Carlisia <[email protected]> * Remove year from the copyright Signed-off-by: Carlisia <[email protected]> * Check for expiration regardless of phase Signed-off-by: Carlisia <[email protected]> * Fix typos and godoc Signed-off-by: Carlisia <[email protected]> * Fix test setup and fix a test case Signed-off-by: Carlisia <[email protected]>

carlisia added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Oct 12, 2020

github-actions bot requested review from ashish-amarnath, nrb and zubron October 12, 2020 17:09

github-actions bot assigned carlisia Oct 12, 2020

carlisia marked this pull request as draft October 12, 2020 17:27

carlisia added this to the v1.6.0 milestone Oct 12, 2020

carlisia force-pushed the c-downloadII branch from 70f0620 to 7aa7aa1 Compare November 3, 2020 21:13

carlisia marked this pull request as ready for review November 3, 2020 23:24

nrb previously requested changes Dec 1, 2020

View reviewed changes

config/rbac/role.yaml Show resolved Hide resolved

pkg/cmd/util/downloadrequest/downloadrequest.go Show resolved Hide resolved

pkg/cmd/util/downloadrequest/downloadrequest.go Show resolved Hide resolved

carlisia mentioned this pull request Dec 3, 2020

Remove check for if updated.Name != created.Name on downloadrequest code #3126

Open

carlisia requested a review from nrb December 3, 2020 00:10

nrb reviewed Dec 8, 2020

View reviewed changes

carlisia force-pushed the c-downloadII branch from 7aa7aa1 to 2be26e4 Compare December 8, 2020 23:58

github-actions bot requested review from dsu-igeek and nrb December 8, 2020 23:58

nrb previously approved these changes Dec 11, 2020

View reviewed changes

dsu-igeek previously approved these changes Dec 16, 2020

View reviewed changes

pkg/controller/download_request_controller.go Outdated Show resolved Hide resolved

pkg/controller/download_request_controller.go Outdated Show resolved Hide resolved

zubron suggested changes Dec 16, 2020

View reviewed changes

Carlisia added 10 commits February 18, 2021 20:08

Migrate download request cli to kubebuilder

816d5dd

Signed-off-by: Carlisia <[email protected]>

Format w make update

8ae69a8

Signed-off-by: Carlisia <[email protected]>

Remove download file

bb2756d

Signed-off-by: Carlisia <[email protected]>

Remove kubebuilder from backup/restore apis

f39e291

Signed-off-by: Carlisia <[email protected]>

Fix test description

affcb9a

Signed-off-by: Carlisia <[email protected]>

Import cleanups

50b8beb

Signed-off-by: Carlisia <[email protected]>

Refactor for controller runtime version update

238d84e

Signed-off-by: Carlisia <[email protected]>

Remove year from the copyright

31a8870

Signed-off-by: Carlisia <[email protected]>

Check for expiration regardless of phase

b7ac49c

Signed-off-by: Carlisia <[email protected]>

Fix typos and godoc

1c536ac

Signed-off-by: Carlisia <[email protected]>

carlisia force-pushed the c-downloadII branch from 777c875 to 1c536ac Compare February 19, 2021 04:30

github-actions bot requested review from nrb and zubron February 19, 2021 04:30

nrb previously approved these changes Feb 21, 2021

View reviewed changes

zubron suggested changes Feb 22, 2021

View reviewed changes

Fix test setup and fix a test case

fcdba64

Signed-off-by: Carlisia <[email protected]>

carlisia dismissed nrb’s stale review via fcdba64 February 22, 2021 21:57

github-actions bot requested review from nrb and zubron February 22, 2021 21:58

zubron approved these changes Feb 22, 2021

View reviewed changes

nrb approved these changes Mar 1, 2021

View reviewed changes

nrb merged commit 11bfe82 into vmware-tanzu:main Mar 1, 2021

cleverhu mentioned this pull request Oct 20, 2022

remove check for downloadrequest #5440

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert DownloadRequest resource/controller to kubebuilder #3004

Convert DownloadRequest resource/controller to kubebuilder #3004

carlisia commented Oct 12, 2020 •

edited

Loading

nrb commented Oct 27, 2020

carlisia commented Oct 27, 2020

carlisia commented Nov 3, 2020

nrb left a comment

carlisia commented Dec 3, 2020

nrb left a comment

carlisia commented Dec 9, 2020

nrb commented Dec 11, 2020

dsu-igeek left a comment

zubron left a comment

carlisia commented Feb 19, 2021 •

edited

Loading

zubron left a comment

zubron Feb 22, 2021

carlisia Feb 22, 2021

zubron Feb 22, 2021

carlisia Feb 22, 2021

carlisia Feb 22, 2021

carlisia Feb 22, 2021

zubron Feb 22, 2021

carlisia Feb 22, 2021

carlisia commented Feb 22, 2021

zubron left a comment

Convert DownloadRequest resource/controller to kubebuilder #3004

Convert DownloadRequest resource/controller to kubebuilder #3004

Conversation

carlisia commented Oct 12, 2020 • edited Loading

nrb commented Oct 27, 2020

carlisia commented Oct 27, 2020

carlisia commented Nov 3, 2020

nrb left a comment

Choose a reason for hiding this comment

carlisia commented Dec 3, 2020

nrb left a comment

Choose a reason for hiding this comment

carlisia commented Dec 9, 2020

nrb commented Dec 11, 2020

dsu-igeek left a comment

Choose a reason for hiding this comment

zubron left a comment

Choose a reason for hiding this comment

carlisia commented Feb 19, 2021 • edited Loading

zubron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlisia commented Feb 22, 2021

zubron left a comment

Choose a reason for hiding this comment

carlisia commented Oct 12, 2020 •

edited

Loading

carlisia commented Feb 19, 2021 •

edited

Loading