-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Convert Pod Volume Backup resource/controller to the Kubebuilder V3 framework #4436
Convert Pod Volume Backup resource/controller to the Kubebuilder V3 framework #4436
Conversation
c.processBackupFunc = c.processBackup | ||
|
||
podVolumeBackupInformer.Informer().AddEventHandler( | ||
cache.ResourceEventHandlerFuncs{ |
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.
In previous version, controller only care about Create and Update event of Pod Volume Backup.
Can the new version controller still keep the event filter function?
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'm afraid I don't understand your question, "Can the new version controller still keep the event filter function?"
Did you want me to keep the podVolumeBackupInformer.Informer().AddEventHandler(...)
function?
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.
Yes. Event handler function filters out the Delete event.
But this may be a minor change. Even Delete event enters reconcile loop, it should not do anything harmful.
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.
A common comment is to remove the error log in reconcile function's error handling logic, because a new logger is already added into controller manager to log the return error. You can reference to #4381
Based on @blackpiglet's findings in a separate PR, I should look and ensure the new context from kubebuilder is being used and not the old one. Update: I believe I should follow some of what was done for removing context in this commit: blackpiglet@98444be |
As we are very close to FC of v1.8, moving this out of v1.8 milestone. |
6a01814
to
6868dba
Compare
6868dba
to
4d2e3bf
Compare
6091e53
to
4d2e3bf
Compare
Signed-off-by: F. Gold <fgold@vmware.com>
Signed-off-by: F. Gold <fgold@vmware.com>
45213bb
to
48827d6
Compare
✔️ Deploy Preview for fervent-minsky-ed95cc canceled. 🔨 Explore the source changes: ec23f3b 🔍 Inspect the deploy log: https://app.netlify.com/sites/fervent-minsky-ed95cc/deploys/61f4a1b6aea0e50007ba0c17 |
Signed-off-by: F. Gold <fgold@vmware.com>
Signed-off-by: F. Gold <fgold@vmware.com>
Although Ginkgo Basic e2e tests pass (the e2e tests in this PR's checks), when I do a manual test to backup a stateful pod, I get this error: time="2022-01-29T02:11:45Z" level=error msg="updating PodVolumeBackup resource" backup=velero/j28-c controller=podvolumebackup error="podvolumebackups.velero.io \"j28-c-hkp55\" not found" logSource="pkg/controller/pod_volume_backup_controller.go:104" podvolumebackup=velero/j28-c-hkp55 This is not the first time I've seen this problem. I fixed this error in the past by not using patch helper, but rather In one situation (Ginkgo Basic e2e tests), patch helper is able to update PVB status, but unable to update PVB status during manual testing. In another situation (manual backup), Despite passing the automated Ginkgo Basic e2e tests, this PR is NOT ready for review due to inability to pass manual e2e testing on a stateful pod. UPDATE Feb. 7, 2022: My only warning is, when this PR is merged, the client needs to be updated to use the upgraded controller-gen But we can't upgrade the client sooner because the older controller-gen More details are in the Kubernetes Slack channel. This PR is now ready for review @blackpiglet, @dsu-igeek, and @reasonerjt. /cc @danfengliu |
Signed-off-by: F. Gold <fgold@vmware.com>
c.enqueue(obj) | ||
// PodVolumeBackupReconciler reconciles a PodVolumeBackup object | ||
type PodVolumeBackupReconciler struct { | ||
Scheme *runtime.Scheme |
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.
What is the Scheme
used for?
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 Kubebuilder uses it (but I don't know what for) because it automatically adds Scheme as part of scaffolding a basic reconciler struct: https://book.kubebuilder.io/cronjob-tutorial/controller-overview.html
Summary of change
Convert Pod Volume Backup resource/controller to the Kubebuilder V3 framework. Write Ginkgo tests.
Fixes #3454
Please indicate you've done the following:
/kind changelog-not-required
.site/content/docs/main
.