Migrate ServerStatusRequest controller and resource to kubebuilder #2838

carlisia · 2020-08-18T22:51:31Z

Closes: #2605.

nrb

A couple small changes, but also one big one that I think introduces a breaking change in functionality in terms of how users were able to choose which controllers were allowed to start up.

pkg/controller/server_status_request_controller.go

pkg/cmd/server/server.go

ashish-amarnath

I've added some comments.
PTAL.

config/rbac/role.yaml

internal/velero/serverstatusrequest.go

pkg/cmd/server/server.go

carlisia · 2020-08-21T20:40:23Z

For anyone testing this, heads up: #2851.

Signed-off-by: Carlisia <carlisia@vmware.com>

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

Signed-off-by: Carlisia <carlisia@vmware.com>

carlisia · 2020-08-21T23:56:31Z

Version works now:

Signed-off-by: Carlisia <carlisia@vmware.com>

carlisia · 2020-08-26T15:42:55Z

@nrb / @ashish-amarnath PTAL!

Signed-off-by: Carlisia <carlisia@vmware.com>

carlisia · 2020-08-27T20:00:00Z

The reason for this last commit:

When I was working and testing #2870 I noticed a change in behavior with the reconciling for the BSL. The reconciling was not kicking in immediately after creating a new BSL. I had more BSL objects in the server this time.

I investigated it and learned that it's expected, since the default # of concurrent workers is 1, so each new request was sitting in the queue waiting for whatever was there before to be processed.

The last commit fixes that by increasing the number of workers that do the processing. The number 10 came from a discussion on the upstream kubebuilder channel, link below, SS here for quick ref. We can change this number, of course, I just don't currently have a better reference, would have to dig deeper if this doesn't seem reasonable.

Source:
https://kubernetes.slack.com/archives/CAR30FCJZ/p1598547745132100?thread_ts=1598539574.118700&cid=CAR30FCJZ

Addendum: I am reasoning that increasing the number of workers that process the reconciles will be smart for all controllers.

nrb · 2020-08-31T16:49:37Z

Going to review this today. I do ask that we hold the generic Patch refactor on this controller til after v1.5.0's release, though.

config/samples/velero_v1_serverstatusrequest.yaml

config/rbac/role.yaml

nrb

I think this looks pretty good now! Thanks for your patience on the review.

Have you tested disabling the controller? I do that the obvious loss of functionality will happen (no server version reporting, no plugin information returned), just seeing if our bases are covered.

pkg/cmd/server/server.go

ashish-amarnath

Added a few comments. PTAL.

pkg/controller/server_status_request_controller.go

internal/velero/serverstatusrequest.go

pkg/controller/server_status_request_controller_test.go

ashish-amarnath

couple more coments.

pkg/cmd/server/server.go

pkg/controller/server_status_request_controller.go

internal/velero/serverstatusrequest.go

ashish-amarnath

🎉

…mware-tanzu#2838) * Convert ServerStatusRequest controller to controller-runtime Signed-off-by: Carlisia <carlisia@vmware.com> * Add select stm Signed-off-by: Carlisia <carlisia@vmware.com> * Fixed status patch bug Signed-off-by: Carlisia <carlisia@vmware.com> * Add mgr start Signed-off-by: Carlisia <carlisia@vmware.com> * Trying to sync Signed-off-by: Carlisia <carlisia@vmware.com> * Clean async now Signed-off-by: Carlisia <carlisia@vmware.com> * Clean up + move context out Signed-off-by: Carlisia <carlisia@vmware.com> * Bug: not closing the channel Signed-off-by: Carlisia <carlisia@vmware.com> * Clean up some tests Signed-off-by: Carlisia <carlisia@vmware.com> * Much better way to fetch an update using a backoff loop Signed-off-by: Carlisia <carlisia@vmware.com> * Even better way to retry: use apimachinery lib Signed-off-by: Carlisia <carlisia@vmware.com> * Refactor controller + add test Signed-off-by: Carlisia <carlisia@vmware.com> * partially fix unit tests Signed-off-by: Ashish Amarnath <ashisham@vmware.com> * Fix and add tests Signed-off-by: Carlisia <carlisia@vmware.com> * Add changelog Signed-off-by: Carlisia <carlisia@vmware.com> * Add ability to disable the controller + cleanups Signed-off-by: Carlisia <carlisia@vmware.com> * Fix bug w/ disabling controllers + fix test + clean up Signed-off-by: Carlisia <carlisia@vmware.com> * Move role.yaml to the correct folder Signed-off-by: Carlisia <carlisia@vmware.com> * Add sample serverstatusrequest.yaml Signed-off-by: Carlisia <carlisia@vmware.com> * Add requeue + better formatting Signed-off-by: Carlisia <carlisia@vmware.com> * Increase # of max concurrent reconciles Signed-off-by: Carlisia <carlisia@vmware.com> Co-authored-by: Ashish Amarnath <ashisham@vmware.com>

carlisia marked this pull request as ready for review August 19, 2020 14:41

carlisia self-assigned this Aug 19, 2020

carlisia requested review from nrb and ashish-amarnath August 19, 2020 14:50

nrb suggested changes Aug 19, 2020

View reviewed changes

carlisia changed the title ~~Migrate ServerStatusRequest to kubebuilder~~ Migrate ServerStatusRequest controller and resource to kubebuilder Aug 19, 2020

carlisia added this to the v1.5 milestone Aug 19, 2020

nrb added the Breaking change Impacts backwards compatibility label Aug 20, 2020

carlisia requested a review from nrb August 20, 2020 15:09

carlisia removed the Breaking change Impacts backwards compatibility label Aug 20, 2020

ashish-amarnath reviewed Aug 20, 2020

View reviewed changes

config/rbac/role.yaml Outdated Show resolved Hide resolved

internal/velero/serverstatusrequest.go Show resolved Hide resolved

internal/velero/serverstatusrequest.go Show resolved Hide resolved

internal/velero/serverstatusrequest.go Outdated Show resolved Hide resolved

ashish-amarnath reviewed Aug 21, 2020

View reviewed changes

pkg/cmd/server/server.go Outdated Show resolved Hide resolved

carlisia force-pushed the c-status-request branch 2 times, most recently from bef7007 to b05f497 Compare August 21, 2020 14:39

carlisia requested a review from ashish-amarnath August 21, 2020 14:56

Carlisia and others added 14 commits August 21, 2020 16:21

Convert ServerStatusRequest controller to controller-runtime

d080dc5

Signed-off-by: Carlisia <carlisia@vmware.com>

Add select stm

872c192

Signed-off-by: Carlisia <carlisia@vmware.com>

Fixed status patch bug

f2e391d

Signed-off-by: Carlisia <carlisia@vmware.com>

Add mgr start

7ba7ed0

Signed-off-by: Carlisia <carlisia@vmware.com>

Trying to sync

e3a525b

Signed-off-by: Carlisia <carlisia@vmware.com>

Clean async now

9861c12

Signed-off-by: Carlisia <carlisia@vmware.com>

Clean up + move context out

96283ec

Signed-off-by: Carlisia <carlisia@vmware.com>

Bug: not closing the channel

ebb960f

Signed-off-by: Carlisia <carlisia@vmware.com>

Clean up some tests

9e13996

Signed-off-by: Carlisia <carlisia@vmware.com>

Much better way to fetch an update using a backoff loop

5f34396

Signed-off-by: Carlisia <carlisia@vmware.com>

Even better way to retry: use apimachinery lib

ac29e1b

Signed-off-by: Carlisia <carlisia@vmware.com>

Refactor controller + add test

daede54

Signed-off-by: Carlisia <carlisia@vmware.com>

partially fix unit tests

73caec7

Signed-off-by: Ashish Amarnath <ashisham@vmware.com>

Fix and add tests

7dd2470

Signed-off-by: Carlisia <carlisia@vmware.com>

carlisia force-pushed the c-status-request branch from c9f257c to 6040c3e Compare August 21, 2020 23:55

Add requeue + better formatting

c701f0c

Signed-off-by: Carlisia <carlisia@vmware.com>

carlisia force-pushed the c-status-request branch from 096c520 to c701f0c Compare August 24, 2020 17:40

carlisia requested a review from zubron August 24, 2020 21:02

Increase # of max concurrent reconciles

f5a18cb

Signed-off-by: Carlisia <carlisia@vmware.com>

carlisia mentioned this pull request Aug 27, 2020

Refactor BSL related code #2870

Merged

nrb added the P1 - Important label Aug 31, 2020

nrb reviewed Aug 31, 2020

View reviewed changes

config/samples/velero_v1_serverstatusrequest.yaml Show resolved Hide resolved

nrb reviewed Aug 31, 2020

View reviewed changes

config/rbac/role.yaml Show resolved Hide resolved

nrb approved these changes Aug 31, 2020

View reviewed changes

ashish-amarnath reviewed Aug 31, 2020

View reviewed changes

pkg/cmd/server/server.go Show resolved Hide resolved

ashish-amarnath suggested changes Aug 31, 2020

View reviewed changes

nrb added the kind/release-blocker Must fix issues for the coming release (milestone) label Aug 31, 2020

This was referenced Sep 1, 2020

？Do we want to continue halting when trying to disable a controller that doesn't exist in the disableable list #2889

Closed

Minor kubebuilder related items to clean up #2884

Closed

ashish-amarnath suggested changes Sep 1, 2020

View reviewed changes

ashish-amarnath approved these changes Sep 1, 2020

View reviewed changes

ashish-amarnath merged commit c952932 into vmware-tanzu:main Sep 1, 2020

carlisia deleted the c-status-request branch September 1, 2020 21:16

georgettica mentioned this pull request Dec 23, 2020

georgettica/bump external snapshotter version #3201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate ServerStatusRequest controller and resource to kubebuilder #2838

Migrate ServerStatusRequest controller and resource to kubebuilder #2838

carlisia commented Aug 18, 2020 •

edited

nrb left a comment

ashish-amarnath left a comment

carlisia commented Aug 21, 2020

carlisia commented Aug 21, 2020

carlisia commented Aug 26, 2020

carlisia commented Aug 27, 2020 •

edited

nrb commented Aug 31, 2020

nrb left a comment

ashish-amarnath left a comment

ashish-amarnath left a comment

ashish-amarnath left a comment

Migrate ServerStatusRequest controller and resource to kubebuilder #2838

Migrate ServerStatusRequest controller and resource to kubebuilder #2838

Conversation

carlisia commented Aug 18, 2020 • edited

nrb left a comment

Choose a reason for hiding this comment

ashish-amarnath left a comment

Choose a reason for hiding this comment

carlisia commented Aug 21, 2020

carlisia commented Aug 21, 2020

carlisia commented Aug 26, 2020

carlisia commented Aug 27, 2020 • edited

nrb commented Aug 31, 2020

nrb left a comment

Choose a reason for hiding this comment

ashish-amarnath left a comment

Choose a reason for hiding this comment

ashish-amarnath left a comment

Choose a reason for hiding this comment

ashish-amarnath left a comment

Choose a reason for hiding this comment

carlisia commented Aug 18, 2020 •

edited

carlisia commented Aug 27, 2020 •

edited