Modernize the Velero code base

[carlisia]

TODO

v1.5

• add kubebuilder framework + convert BSL resource - Add kubebuilder framework + convert BSL resource #2604
• add a BSL controller to handle validation + update BSL status phase - Add a BSL controller to handle validation + update BSL status phase #1967
• convert ServerStatusRequest resource/controller to the kubebuilder framework - Convert ServerStatusRequest resource/controller to the kubebuilder framework #2605

v1.6

• convert download request resource/controller - Convert DownloadRequest resource/controller to the kubebuilder framework #2656

v1.7

• focus on reducing tech debt through improving testing

v1.8

• convert podvolume backup - Convert Pod Volume Backup resource/controller to the Kubebuilder framework #3454
• convert podvolume restore - Convert Pod Volume Restore resource/controller to the Kubebuilder framework #4134
• convert backup sync controller - Convert backup sync resource/controller to the Kubebuilder framework #4135

v1.9

• convert delete backup request
• convert restic repository
• convert schedule

v1.10

• refactor VolumeSnapshotLocation - Refactor VolumeSnapshotLocation CRD and related code with kubebuilder #5025
• refactor ResticRepositoryEnsurer - Refactor ResticRepositoryEnsurer controller with kubebuilder #5024
• refactor GC controller - Refactor GC controller with kubebuilder #5023
• refactor Backup Sync controller - Refactor BackupSync Controller with kubebuilder #5022

v1.11

• refactor Backup - Refactor backup controller with kubebuilder #5026
• refactor Restore - Refactor Restore controller with kubebuilder #5027

v1.12

• DownloadRequest - Need to modify DownloadRequest Controller's status update logic #6010
• investigate how to remove generated client-go code - Investigate how to remove the generated legacy client-go code. #6190

v1.1x

• remove obsolete code - Remove obsolete, generated client code #2607
• cleanup: Remove obsolete, generated client code · Issue #2607 · vmware-tanzu/velero

---

Recipe for converting:

• Change controller
• Change server.go file to call this new controller; don't forget to delete all the controller related variables (look at previous PRs for examples)
• Move any logic to under internal; add unit tests if there are none
• Change controller tests to use gomega, for consistency with the new controller tests
• Important: change the corresponding API file to have kubebuilder markers in addition to the existing markers (look at previous PRs for examples)
• add _types to the end of the api file name

Note for developers:

If you get a " not found" error when attempting to patch a status, it is very likely a case of the status subresource not being enabled in the CRD. The way to verify this is by running kubectl get crd $CRDNAME and then checking if .spec.versions.subresources.status is set. Here are the usual ways to address this:

• the equivalent of these lines:

  velero/pkg/apis/velero/v1/backupstoragelocation_types.go

  Lines 109 to 110 in c663ce1

  	// +kubebuilder:rbac:groups=velero.io,resources=backupstoragelocations,verbs=get;list;watch;create;update;patch;delete
  	// +kubebuilder:rbac:groups=velero.io,resources=backupstoragelocations/status,verbs=get;update;patch

• run make update
• run v install --crds-only --dry-run -o yaml | kubectl apply -f - to ensure the updated CRD is re-installed.

---

What

To modernize the Velero code base and tests and bring it more up-to-date and consistent with newer Kubernetes apps, while cleaning up technical debt along the way.

Why

• opportunity to clean up some technical debt by way of removing and keeping logic out of controllers
• (much) better and sustainable tests
• conformity with many k8s apps
• ability to use the runtime-controller library/client instead of generated informers/listers that live in our tree

Update: it will also greatly speed up our make update, make ci and make verify targets since it will no longer involve generating code when running those.

How

The end result will...

• follow what kubebuilder generates as far as files and folder structure, which will make the project look consistent with many other newer projects/apps
• use the controller-runtime library client to CRUD resources instead of the go-client library, which is more convoluted by comparison
• use kubebuilder/controller-runtime related test libraries instead of current mocks and action oriented style of testing for controllers, which is a very ineffective and convoluted way to test in the opinions of many
• follow the controller-runtime examples and best practices for both controller code and tests, which should help us move and consistently keep business logic out of controllers, which is not the case today

Strategy

For this change to work in an incremental manner, we need to be able to CRUD some resources with the existing go-client library, and some resources with the runtime-controller library. This has been accomplished with the PR below, which instantiates the runtime-controller Manager and uses this manager's client to CRUD the BSL resource:

#2561

The idea is to build on this PR and convert one resource/controller at a time. We have 10 resources and 1 (BSL) is done, so that leaves us with 9.

We would leave the backup and restore ones for last, since they are the most involved in terms of logic and tests.

We would start converting the smallest resource/controller, which is the ServerStatusRequest, to get an idea of how long it takes to do this work for 1 resource/controller, end-to-end. This would help estimate the reminder of the work.

While we work on this transition, we will keep the existing api package/library in its entirety, in case anyone is importing and using this package. Keeping this around is of no cost since it's all generated in an automated way.

For the Velero to transition completely, it would entail removing this (by then) obsolete package, which is a braking change. So we would like to spread this work over the current and next few releases to be able to introduce this change with v2.0.

Vote on this issue!

This is an invitation to the Velero community to vote on issues, you can see the project's top voted issues listed here.
Use the "reaction smiley face" up to the right of this comment to vote.

• 👍 for "The project would be better with this feature added"
• 👎 for "This feature will not enhance the project in a meaningful way"

[carlisia]

@vmware-tanzu/velero-maintainers PTAL.

[carlisia]

@nrb / @ashish-amarnath: I'm thinking https://github.com/vmware-tanzu/velero/blob/main/pkg/controller/backup_tracker.go, even though is in the controller package, is not a controller and has nothing to be converted. Just wanted to get a sanity check.

[blackpiglet]

By far, there are several controllers and CRDs not converted yet, including:

• BackupSync Controller
• GC Controller
• VolumeSnapshotLocation (only CRD, no controller)
• ResticRepositoryEnsurer Controller
• Backup Controller
• Restore Controller

[kaovilai]

Hopefully we makes it to kubebuilder/v3 layout

[blackpiglet]

Do you mean changing the layout to something like this project?
https://github.com/jetstack/kubebuilder-sample-controller

Could you give some scenarios that need the change? Or What benefit do we get from the change?

[kaovilai]

The benefit would be we can use kubebuilder CLI from now on to generate new API and controllers boilerplate code

https://book.kubebuilder.io/migration/legacy/migration_guide_v1tov2.html

[kaovilai]

Specifically we should be able to call

kubebuilder create api --group batch --version v1 --kind CronJob

And get boilerplate code for a new controller done automatically. And any future migrations should be simple as there are already many projects using the v3 layout.

In addition it standardizes the project to a layout that is more common to a new contributors.

https://book.kubebuilder.io/cronjob-tutorial/new-api.html