-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fixes Restore for Workerless Clusters #9265
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
base: main
Are you sure you want to change the base?
Fixes Restore for Workerless Clusters #9265
Conversation
- helps restore on workerless clusters - logs error message, but continues Signed-off-by: Nico Weisenauer <nico.weisenauer@sap.com>
Signed-off-by: Nico Weisenauer <nico.weisenauer@sap.com>
Signed-off-by: Nico Weisenauer <nico.weisenauer@sap.com>
|
IMO, this fix MAY fix your problem, but it delays the error in normal use cases. I'm a little reluctant to make it upstream. Because in general we don't support velero running on another cluster at this moment and this looks like a workaround and is very limited. |
|
Any other opinions on this? |
| if err := crClient.Get(context.TODO(), types.NamespacedName{Name: veleroDeploymentName, Namespace: namespace}, deployment); err != nil { | ||
| logger.WithError(err).Warnf("Could not find deployment %q in namespace %q", veleroDeploymentName, namespace) | ||
| } else { | ||
| image = veleroutil.GetVeleroServerImage(deployment) |
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.
So based on feedback saw earlier, my tip for getting this in would be to hide this behavior behind a server feature flag or something.
Like velero server --workerless-mode and you will then perhaps not have to use warn, but just info as it is an expected operating mode.
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.
Does that sound reasonable? @reasonerjt perhaps you want this implementation explained and detailed as a design for the epic #7843
Please add a summary of your change
Fixes restore for workerless clusters. Previously restore of custom resources would crash, because Velero Deployment is not running in the workerless cluster. This simple change prevents the crash and does not change behaviour for "normal" usage with standard clusters.
Does your change fix a particular issue?
Fixes #9251
Please indicate you've done the following:
make new-changelog) or comment/kind changelog-not-requiredon this PR.site/content/docs/main. (not needed imo)