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

Velero Kopia Integration - Rename DefaultVolumesToRestic in Backup CRD #5269

Closed
Lyndon-Li opened this issue Aug 31, 2022 · 15 comments
Closed

Comments

@Lyndon-Li
Copy link
Contributor

Lyndon-Li commented Aug 31, 2022

Epic: #5050
Design: #4926

DefaultVolumesToRestic exists in Backup CRD, when adding kopia path, the name is not reasonable, need to rename it.
For compatibility consideration, the old parameter of the existing CRs created by old versions should also be respected.

@reasonerjt
Copy link
Contributor

We need to be careful here b/c it may involve the upgrade of CR.

@Lyndon-Li
Copy link
Contributor Author

As the discussion, completely removing DefaultVolumesToRestic in the CRD requires changing the CRD version and also a mutation webhook to support the CRs created by the old versions.
Therefore, we've decided to defer this to later releases, we may have some other requirements for changing CRD spec, then we change the CRD version for once for all the changes.

@Lyndon-Li Lyndon-Li removed this from the 1.10 milestone Oct 11, 2022
@kaovilai
Copy link
Member

I think it would be reasonable to support multiple CRD versions here which will allow two types of Backup CR to co-exist on the same cluster.

@kaovilai
Copy link
Member

tldr backup/v1 uses DefaultVolumesToRestic
backup/v2 uses DefaultVolumesTo<>

We could have a CRD compatibility matrix to clarify that velero v1.11 (or some other version) is compatible with backup/v1 and backup/v2.

To use kopia, you would use v2 CRD.

@Lyndon-Li
Copy link
Contributor Author

@kaovilai
DefaultVolumesToRestic and DefaultVolumesToFsBackup is not coexistence, instead, DefaultVolumesToFsBackup replaces DefaultVolumesToRestic.
From v1.10 and later, both the Restic path and Kopia path will use DefaultVolumesToFsBackup only, so DefaultVolumesToRestic is for compatibility purpose only, that is, to support CRs created by old versions.

Therefore, it is not necessary to support a dedicate version with DefaultVolumesToRestic. In future, we will totally remove it and if CRs created by old versions use this field, we transfer the value to DefaultVolumesToFsBackup.

@kaovilai
Copy link
Member

When making changes like replacing a field, from Kubernetes best practices, it is best to have a completely new APIVersion signifying a change.

We should follow CRD versioning convention when possible.
https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/

@kaovilai
Copy link
Member

Following k8s convention, we would use https://github.com/kubernetes-sigs/kube-storage-version-migrator for migrating fromDefaultVolumesToRestic to DefaultVolumesToFsBackup

@Lyndon-Li
Copy link
Contributor Author

yes, so we've deferred this, in v1.10, we will not replace it, we leave it there and don't change the CRD version.

@reasonerjt
Copy link
Contributor

Before introducing any breaking change, I believe we need a broader discussion about the versioning strategy in velero.
Some of the decisions need to be made, like, shall we bump v1 CRD to V2alpha1? Will we need to support different versions of CRDs in velero(I hope not)?

In terms of the workflow, AFAIK conversion webhook is the more common approach to handle the migration of CRDs.

Given that v1.11 is a relatively short release, this may need to be pushed out of it so that we have more time to discuss and clarify what should be done.

@stale
Copy link

stale bot commented Mar 14, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the staled label Mar 14, 2023
@kaovilai
Copy link
Member

not-stale :)

@stale
Copy link

stale bot commented May 18, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the staled label May 18, 2023
@stale
Copy link

stale bot commented Jun 10, 2023

Closing the stale issue.

@stale stale bot closed this as completed Jun 10, 2023
@Lyndon-Li Lyndon-Li removed the staled label Jun 11, 2023
@Lyndon-Li Lyndon-Li reopened this Jun 11, 2023
@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. If a Velero team member has requested log or more information, please provide the output of the shared commands.

@github-actions
Copy link

This issue was closed because it has been stalled for 14 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants