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

Fix issue 6561 #6564

Merged
merged 1 commit into from
Aug 2, 2023
Merged

Conversation

Lyndon-Li
Copy link
Contributor

@Lyndon-Li Lyndon-Li commented Jul 31, 2023

Fix #6561, change the backup/restore pod's image to the same image used by the node-agent, but with a different binary velero-helper, so as not to depend on Alpine which may have compliance or security issues or image pull issues.

@Lyndon-Li Lyndon-Li added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Jul 31, 2023
@Lyndon-Li Lyndon-Li self-assigned this Jul 31, 2023
@Lyndon-Li Lyndon-Li marked this pull request as ready for review July 31, 2023 13:48
sseago
sseago previously approved these changes Jul 31, 2023
Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

would be interesting to use cobra here? it would be more verbose, but we would have a CLI by default (and it also have the 2 validations builtin: cobra.MinimumNArgs(1) and cobra.OnlyValidArgs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am hesitating to involve cobra:

  • The binary is targeted to implement simply tasks (otherwise, we can use velero binary instead), i.e., the current pause functionality, it is not expected to be with complex parameters, for which cobra could make a big help.
  • Involving cobra will involve some more packages, which will make the total binary size larger.

Copy link
Contributor

Choose a reason for hiding this comment

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

We may even make it simpler to make it velero-pause, b/c I don't think we wanna support other subcommand, it's simply a placeholder for DM.

@Lyndon-Li
Copy link
Contributor Author

We have more inputs/requirements today, so changed the solution from end to end and see if this can meet them:

  • Use Velero image for data mover pods
  • Create a binary velero-helper with small size into Velero image
  • Data mover pods launches velero-helper instead of velero, so that data mover pods memory footprint is small
  • Duplicate all the image options(including the image itself) from node-agent so that data mover pods never need to pull images themselves, so as to eliminate the complication for image pulling

Copy link
Contributor

@reasonerjt reasonerjt left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-unit-tests kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

alpine can not be pulled in some environments when moving snapshot
6 participants