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

Backup implementation: velero installation #815

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zerospiel
Copy link
Member

  • new component labels on most components (required for backups' label selectors)
  • wide range of new RBAC permission for the backup ctrl
  • velero stack reconciliation
  • adjusted backup user-facing roles
  • added backup ctrl configuration in the hmc chart
  • removed schema defaults in favor of mutation

Note: the controller is still turned off to avoid any potential regression.

#605

@zerospiel zerospiel marked this pull request as ready for review December 19, 2024 15:19
@zerospiel zerospiel linked an issue Dec 19, 2024 that may be closed by this pull request
@zerospiel zerospiel force-pushed the backup_impl branch 8 times, most recently from bce8d98 to e3ce508 Compare December 24, 2024 11:45
Copy link
Member

@eromanova eromanova left a comment

Choose a reason for hiding this comment

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

A general question about the hmc.mirantis.com/component: hmc label and credentials backup. As I understood only objects with this label will be backed up. But I'm concerned about the credentials' identity references. Our controller does not modify the identity refs (Secrets, AzureClusterIdentity, etc) when it was provided by the user. Maybe the credentials controller should be adjusted to label those objects too?

Also, please add this label on newly added config/dev/adoptedcluster-*.yaml objects.

internal/controller/backup/type.go Outdated Show resolved Hide resolved
Copy link
Member

@eromanova eromanova left a comment

Choose a reason for hiding this comment

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

Same question about the HelmCharts and HelmRepositories.
I haven't found any logic about labeling the generated HelmCharts (sorry if it's present and I've missed it).
When the user creates the BYO Template it can reference the helm chart from the custom HelmRepository (manually created). Should it also be labeled?
Please ignore my concerns if it's something that you're aware but it's not in the scope of this PR.

* new component labels on most components
* wide range of new RBAC permission for the backup ctrl
* velero stack reconciliation
* adjusted backup user-facing roles
* added backup ctrl configuration in the hmc chart
* removed schema defaults in favor of mutation
@zerospiel
Copy link
Member Author

A general question about the hmc.mirantis.com/component: hmc label and credentials backup. As I understood only objects with this label will be backed up. But I'm concerned about the credentials' identity references. Our controller does not modify the identity refs (Secrets, AzureClusterIdentity, etc) when it was provided by the user. Maybe the credentials controller should be adjusted to label those objects too?

Also, please add this label on newly added config/dev/adoptedcluster-*.yaml objects.

Not quite, there will be many more label selectors, and those sentinel labels are just for objects that do not have any labels at the moment but should also be presented within a backup. One of the selectors also covers the HelmRepositories but not the BYO, the general recommendation (as I can see now) is put a special label to all your BYO objects but it'd be amended if required.

In particular, the question about the credentials' references: I had no intention to change permissions, and such objects referenced in a credentials object might have no labels, so I either will address it via the velero API or will add the sentinel label changing permissions along (the former is preferable). The problem is to be addressed in the next PR along with numerous enhancements to the installation and the controller itself.

@zerospiel zerospiel requested a review from eromanova December 27, 2024 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

implement velero installation reconciliation
2 participants