Skip to content
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

Add Watch of Velero Backup and remove Backup controller #12

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

mpryc
Copy link
Collaborator

@mpryc mpryc commented Mar 5, 2024

Change which removes Backup Controller and adds the Watch of the Backup object within NonAdminBackup Controller.

Added two main predicates with 'upper' logic within composite predicate to easily distinguish between predicate for NonAdminBackup and the Backup one.

The reconcile loop updates the Status and the Spec of the NonAdminBackup from the Velero Backup object. The Spec in the Velero Backup is updated by the Velero, so we use similar logic to update our NonAdminBackup one.

Fixes #11

@mpryc mpryc force-pushed the remove_velero_controller branch from 44419d8 to 60de9d0 Compare March 5, 2024 15:58
cmd/main.go Outdated Show resolved Hide resolved
@mateusoliveira43
Copy link
Contributor

@mpryc
Copy link
Collaborator Author

mpryc commented Mar 5, 2024

Also, undo changes from PROJECT file 5ff6f33#diff-d1fb402f1269ba7396dca9cbd03b44ac055874669bce1d0f239fa2f54ea3584bR20

We can't because we will get error:

ERROR	controller-runtime.source.EventHandler	kind must be registered to the Scheme	{"error": "no kind is registered for the type v1.Backup in scheme \"pkg/runtime/scheme.go:100\""

return true
}
// Otherwise, apply VeleroBackupPredicate
return p.VeleroBackupPredicate.Generic(p.Context, evt)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can these functions be simplified to

return p.NonAdminBackupPredicate.Generic(...) || p.VeleroBackupPredicate.Generic(...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could but then it's harder to put some clear comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus there may be other conditions for predicate for each of the objects, so having each block for each "type" makes sense imo.

return false
}

func (veleroBackupPredicate VeleroBackupPredicate) Generic(ctx context.Context, evt event.GenericEvent) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would not create these "placeholder" functions

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need as we are implementing interface.

Copy link
Member

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

predicates should be very performant cause there can be many hundreds of these at any given time that need filtering out.

internal/controller/velerobackup_predicate.go Outdated Show resolved Hide resolved
Change which removes Backup Controller and adds the Watch of the
Backup object within NonAdminBackup Controller.

Added two main predicates with 'upper' logic within composite
predicate to easily distinguish between predicate for NonAdminBackup
and the Backup one.

The reconcile loop updates the Status and the Spec of the NonAdminBackup
from the Velero Backup object. The Spec in the Velero Backup is updated
by the Velero, so we use similar logic to update our NonAdminBackup one.

Signed-off-by: Michal Pryc <[email protected]>
@mpryc mpryc force-pushed the remove_velero_controller branch from 60de9d0 to c882fea Compare March 5, 2024 20:16
Co-authored-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Michal Pryc <[email protected]>
@mpryc
Copy link
Collaborator Author

mpryc commented Mar 6, 2024

@mateusoliveira43 could we merge this now?

Copy link
Contributor

@mateusoliveira43 mateusoliveira43 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets merge to unblock PRs, we can analyze code more in depth next week

@mateusoliveira43 mateusoliveira43 merged commit 5bdd750 into migtools:master Mar 6, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged / Ready for build
Development

Successfully merging this pull request may close these issues.

Remove VeleroBackup controller and replace it with watch
3 participants