Design for Handling backup of volumes by resources filters #5773

qiuming-best · 2023-01-17T07:17:01Z

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)
#5035

Please indicate you've done the following:

Accepted the DCO. Commits without the DCO will delay acceptance.
Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
Updated the corresponding documentation in site/content/docs/main.

Signed-off-by: Ming <[email protected]>

codecov-commenter · 2023-01-17T07:27:39Z

Codecov Report

Merging #5773 (516be60) into main (598333d) will increase coverage by 0.23%.
The diff coverage is n/a.

❗ Current head 516be60 differs from pull request most recent head 7759424. Consider uploading reports for the commit 7759424 to get more accurate results

@@            Coverage Diff             @@
##             main    #5773      +/-   ##
==========================================
+ Coverage   40.67%   40.90%   +0.23%     
==========================================
  Files         243      248       +5     
  Lines       21035    21554     +519     
==========================================
+ Hits         8555     8817     +262     
- Misses      11857    12100     +243     
- Partials      623      637      +14

Impacted Files	Coverage Δ
pkg/util/kube/predicate.go	`49.09% <0.00%> (-16.77%)`	⬇️
pkg/restore/change_pvc_node_selector.go	`62.50% <0.00%> (-4.97%)`	⬇️
pkg/persistence/object_store.go	`53.09% <0.00%> (-3.10%)`	⬇️
pkg/podvolume/restorer.go	`9.09% <0.00%> (-2.48%)`	⬇️
internal/storage/storagelocation.go	`78.78% <0.00%> (-1.77%)`	⬇️
pkg/install/resources.go	`51.13% <0.00%> (-1.19%)`	⬇️
pkg/plugin/framework/action_resolver.go	`5.00% <0.00%> (-0.76%)`	⬇️
pkg/controller/pod_volume_backup_controller.go	`38.74% <0.00%> (-0.41%)`	⬇️
pkg/controller/pod_volume_restore_controller.go	`25.00% <0.00%> (-0.40%)`	⬇️
pkg/controller/backup_controller.go	`55.64% <0.00%> (-0.20%)`	⬇️
... and 28 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

qiuming-best · 2023-01-18T01:12:44Z

hi @pradeepkchaturvedi, Here is one draft design pr for handling backup volume by resources filters. If you have any opinion, please comment on it.

design/handle-backup-of-volumes-by-resources-filters.md

reasonerjt · 2023-01-18T10:33:06Z

design/handle-backup-of-volumes-by-resources-filters.md

+- option to skip folders
+
+### B. Accurately select target volume to backup
+Some volumes are only used for logging while others volumes are for DBs, and only need to backup DBs data. users need `one specific resource seletor` to accurately select target volumes to backup.


How do we support that if these two PVs have the same storage class?

For those PVs has the same attributes and are hard to distinguish, we could use GVRN ways to accurately select PV to backup or skip. Which would be a group of combinations of groupResource + namespacedNames

design/handle-backup-of-volumes-by-resources-filters.md

gsadhani · 2023-02-15T09:45:28Z

design/handle-backup-of-volumes-by-resources-filters.md

+#### Versioning
+Here we introduced the version field in the YAML config to contain break changes in case of some compatibility problems:
+```yaml
+version: v1


What was the reason to choose to ConfigMap over a new CRD for resource policy? CRD would allow API versioning of the policy resources.

I've added more details to the design now.

the reason we choose Configmap is:

CRD is more like one kind of resource with status, Kubernates API Server handles the lifecycle of a CR and handles it in different statuses. Compared to CRD, Configmap is more focused to store data. And we only want store policies, so Configmap is more suitable.

CRD would allow API versioning also needs our Velero codes to explain it, so it may be no different to use Configmap, we don't change the version of Configmap, but maintain the version in data part of Configmap.

We did think about adding CR, but favor CM over CR for 2 reasons:

We don't want to introduce the full set of subcommands immediately to manage the lifecycle of these CRs, we may make it a CR in the future if this approach is verified.

There's also debate that, this piece of data is purely for reference, we don't want to introduce a controller to reconcile the status if we make it a CR.

The status subresource is optional. Policy engines like OPA and Kyverno use CRD to represent policies. With CRD, at least users are familiar with the corresponding K8s versioning scheme.

design/handle-backup-of-volumes-by-resources-filters.md

rnarenpujari · 2023-02-15T10:13:05Z

design/handle-backup-of-volumes-by-resources-filters.md

+- conditions:
+    csi:
+      driver: aws.efs.csi.driver


I assume there will be a catch-all condition that we can use at the end? For example if user wants to skip all PVs except storage class abc.

whether should we have a catch-all condition is depend on user configuration. If users configured it, velero would support it.

The current situation would be the ignored volumes will not be backed up.
suppose we have volumes A, B, C, and D, through all kinds of resources filters, A and B are defined to include, D is excluded, Velero will backup volumes A and B, and D won't be backed up. the ignored C will not be backed up by default.

Yes, I think that's a good enhancement, thanks @rnarenpujari

cc @qiuming-best

The current situation would be the ignored volumes will not be backed up.
suppose we have volumes A, B, C, and D, through all kinds of resources filters, A and B are defined to include, D is excluded, Velero will backup volumes A and B, and D won't be backed up. the ignored C will not be backed up by default.

OK skip was not a good example. If we want to snapshot storage class abc but use restic for everything else, then a catch-all/wildcard condition at the end would be nice to have I think.

I assume in that case, you just defined 2 conditions; one for storage class abc with volume-snapshot, and the wildcard condition with restic.

@qiuming-best how would one represent a policy with wildcard conditions?

design/handle-backup-of-volumes-by-resources-filters.md

ihcsim · 2023-02-15T19:51:33Z

@reasonerjt @qiuming-best I am not convinced that we need to special-case the filtering of persistent volumes. The introduction of a non-K8s API and corresponding versioning scheme to represent volume policies feels a bit much. Feels like all the conditions described in the proposal can be represented using the new include/exclude resource filters in #5838, plus the field selector proposal at #5842. With the exception of the proposed non-skip action types, the rest of the volume policies are really just about filtering resources, right? Why not try to integrate with the new filtering mechanism in #5838?

Can you elaborate a bit more on the file-system-backup and volume-snapshot action type and their parameters? Feels like this is something that can be expressed using the existing CRD, independent of the filtering mechanism.

The field selector mechanism proposed in #5842 uses the fields.Fields to represent the filter attributes. It is possible to provide our implementation of fields.Selector to satisfy the conditions example in this proposal. Then the Backup resource can look something like:

apiVersion: velero.io/v1
kind: Backup
metadata:
  name: my-backup
  namespace: velero
spec:
  includedClusterScopeResources:
  - groupResource:
    group: "v1"
    resource: "persistentvolumes"
    selectors:
      capacity: "0,100Gi"
      csi:
        driver: aws.ebs.csi.driver
        fsType: ext4
      storageClass:
      - gp2
      - ebs-sc
  excludedClusterScopeResources:
  - groupResource:
      group: "v1"
      resource: "persistentvolumes"
      selectors:
        csi:
          driver: aws.efs.csi.driver
  - groupResource:
      group: "v1"
      resource: "persistentvolumes"
      selectors:
        nfs: {}

Building on top of #5838, I believe this approach can significantly reduce the implementation and maintenance scope, where we just need to focus on the handling and conversion of the field selectors. (K8s only supports a handful of field selectors on the server-side.)

Even if we think the proposed volume policy is necessary, I don't think #5842 contradicts this proposal. It addresses issues like #5152 and #5118 which this proposal doesn't. Based on our discussion on the community call, I would like to get a sense of how this policy can be extended to include other cluster-scoped resources like CRDs, APIService, ClusterRole, filtering by their names. I suspect we might end up with supporting different sets of conditions for different resources (unlike field selectors where K8s provide a sensible default).

reasonerjt · 2023-02-16T18:57:22Z

@ihcsim
Thanks for the comment.

The reason there's the action and parameters fields is that we wanna make sure there're flexibilities so that the user can choose to do more actions against them in addition to include/exclude. For example in one backup, for volumes provisioned via storageClass A, with certain capacities, the user may wanna choose to use file-system-back approach (restic/kopia) to backup the data, and for some volumes that meet certain criteria, the user wanna choose to take snapshots, and some certain volumes will be skipped, etc.

The fieldSelector in #5842 may be sufficient for filtering resources based on names, but as for PV there are quite a few fields, and nested structures in the spec. In our internal discussion, we found matching the values of the fields does not satisfy the use case, for example, capacity is a range in the policy data, and to apply the policy against a PV we do not simply check if the value in the selector is the same as in the spec, therefore we introduce the condition which does not have to be a 1:1 mapping to each field in the spec.

I agree the policy proposed in the PR does not contradict #5842. I was thinking we may extend the policy to add resourcePolicies which support more general conditions like type: "customresourcedefinitions", name: "name1, name2". I slightly prefer not to add too many fields into the spec of backup CR, but I'm open for discussion in this part.

ihcsim · 2023-02-17T17:40:55Z

design/handle-backup-of-volumes-by-resources-filters.md

+// VolumePolicy defined policy to conditions to match Volumes and related action to handle matched Volumes
+type VolumePolicy struct {
+	// Conditions defined list of conditions to match Volumes
+	Conditions map[string]interface{} `yaml:"conditions"`


In our internal discussion, we found matching the values of the fields does not satisfy the use case, for example, capacity is a range in the policy data, and to apply the policy against a PV we do not simply check if the value in the selector is the same as in the spec, therefore we introduce the condition which does not have to be a 1:1 mapping to each field in the spec.

@reasonerjt As proposed, the field selector approach can be extended (client-side) to work like this. AIUI, there aren't ways to do server-side filtering for fields like capacity, right? We will have to retrieve all the PVs and then apply the filters on the client-side. If that's true, the underlying implementation will be the same regardless of whether it's fields.Fields, field.Selector, or Conditions in the API. (Fields like name, namespace and status will work without additional implementation because K8s know how to filter by those fields.)

Are you suggesting we should introduce a 1:1 mapping to the PV, or put this chunk of data into backup CR?

IMO, 1:1 mapping may be sufficient but a more flexible condition map can be easier to create and maintain, I don't see a very strong reason we must keep this 1:1 mapping to the fileds of PV resource.

design/handle-backup-of-volumes-by-resources-filters.md

ihcsim · 2023-02-17T17:55:51Z

design/handle-backup-of-volumes-by-resources-filters.md

+- conditions:
+    csi:
+      driver: aws.efs.csi.driver


I assume in that case, you just defined 2 conditions; one for storage class abc with volume-snapshot, and the wildcard condition with restic.

@qiuming-best how would one represent a policy with wildcard conditions?

ihcsim · 2023-02-17T18:06:14Z

design/handle-backup-of-volumes-by-resources-filters.md

+#### Versioning
+Here we introduced the version field in the YAML config to contain break changes in case of some compatibility problems:
+```yaml
+version: v1


The status subresource is optional. Policy engines like OPA and Kyverno use CRD to represent policies. With CRD, at least users are familiar with the corresponding K8s versioning scheme.

ihcsim · 2023-02-17T18:23:57Z

design/handle-backup-of-volumes-by-resources-filters.md

+
+Volume policies are a list and if the target volumes match the first policy, the latter will be ignored, which would reduce the complexity of matching volumes especially when there are multiple complex volumes policies.
+
+#### Action


The reason there's the action and parameters fields is that we wanna make sure there're flexibilities so that the user can choose to do more actions against them in addition to include/exclude.

@reasonerjt Seems to me there is potentially a "filtering" and a "grouping" requirements. If grouping is the main use case where we want to apply certain actions to a group of volumes, I wonder if a volume group CRD might be clearer and more structured than a data in a ConfigMap. The Backup CR can reference the volume groups, instead of the ConfigMap.

If grouping and filtering can be tackled separately, the Backup CRD spec already has filter-related fields. We can change it from []string to a struct to fulfill the Conditions requirement. We should keep #5842, which goes with #5838, separated from this proposal.

It's not about grouping, it's about variety of actions, we wanna support actions other than "include/exclude".

As for putting the filter in or out of Backup CR, we wanna put it out of the CR for two reasons:

Control the size of backup CR. We've been continuously adding new fields to the CRD to add new features, despite it's already v1, I hope we can reduce such changes, and will open another issue to discuss the v1 -> v2 transformation of velero CRDs.

Make the policy re-usable across backups. There has been some requirement from other adopters that they wanna introduce a policy CR and use them with different backups, this is a step towards that direction.

"include/exclude" could not further policy distinct when one backup both have volume-snapshot and file-system-backup, so grouping does not work, we introduced actions way

Policy re-use sounds like a good use case to me. If the ultimate goal is a policy CR, why not start with a CR right from the start? As for not adding new fields to backup, I think you will have to add something to it anyway, even if it's just a ref (or a list of refs), right?

Regardless, can we keep and review #5842, and possibly add it to #5838, as I feel like it's different from the policy concept in this proposal? Thanks.

reasonerjt

LGTM, thanks @qiuming-best for putting this together.

shubham-pampattiwar

added a few comments

shubham-pampattiwar · 2023-03-02T02:54:17Z

design/handle-backup-of-volumes-by-resources-filters.md

+// Action defined as one action for a specific way of backup
+type Action struct {
+	// Type defined specific type of action, it could be 'file-system-backup', 'volume-snapshot', or 'skip' currently
+	Type string `yaml:"type"`


@qiuming-best @reasonerjt If we support only a handful type of actions shouldn't the Action.Type be of some struct instead of string ? like:

type VolumeActionType string const Skip VolumeActionType = "skip" const VolumeSnapshot VolumeActionType = "volume-snapshot" etc type Action struct { Type VolumeActionType }

ok, I'll modify it

shubham-pampattiwar · 2023-03-02T03:43:17Z

design/handle-backup-of-volumes-by-resources-filters.md

+## Implementation
+This implementation should be included in Velero v1.11.0
+
+Currently, in Velero v1.11.0 we only support `Action`


Should we also add backup level Validation where in we check whether the specified action in the policy is supported by velero install instance or not. For example, If the user specified fs-backup but the velero install does not have restic or kopia then in that case we could fail the backup even before we start processing it ? WYGT ?

users could config many kinds of Action Type, they can config both fs-backup and volume-snapshot types of Action. When doing the backup for one specific volume, we will first check which types of Action the volume matched, if all policies are validated and returned it could use fs-backup and volume-snapshot both ok, and the current backup is through CSI plugin to take a snapshot, it will take a snapshot for volume-snapshot matched even though users have configured anther fs-backup for it.

If we add lots of validation related to the installation or environments it will make our validation complicated, and some redundant configurations by users are also acceptable.

shubham-pampattiwar · 2023-03-02T03:44:44Z

design/handle-backup-of-volumes-by-resources-filters.md

+- backup.velero.io/must-include-additional-items
+
+So it should be careful with the combination of volumes resources policies and the above resources filters.
+- When volume resource policies conflict with the above resource filters, we should respect the above resource filters. For example, if the user used the opt-out approach to `backup.velero.io/backup-volumes-excludes` annotation on the pod and also defined include volume in volumes resources filters configuration, we should respect the opt-out approach to skip backup of the volume.


++ Also, please be sure to add this in the velero documentation

Signed-off-by: Ming <[email protected]>

shubham-pampattiwar

LGTM @qiuming-best

qiuming-best added 2 commits January 17, 2023 07:12

Design for Handle backup of volumes by resources filters

eb284fd

Signed-off-by: Ming <[email protected]>

Merge branch 'main' of https://github.com/qiuming-best/velero into main

5899287

github-actions bot assigned qiuming-best Jan 17, 2023

github-actions bot added the Area/Design Design Documents label Jan 17, 2023

github-actions bot requested review from reasonerjt and shubham-pampattiwar January 17, 2023 07:17

qiuming-best changed the title ~~Design for Handle backup of volumes by resources filters~~ Design for Handling backup of volumes by resources filters Jan 17, 2023

qiuming-best marked this pull request as draft January 17, 2023 07:18

github-actions bot added the has-changelog label Jan 17, 2023

qiuming-best force-pushed the volumes-filter-design branch 2 times, most recently from 063b507 to a388d6c Compare January 17, 2023 07:35

qiuming-best requested review from Lyndon-Li, ywk253100, sseago and blackpiglet January 17, 2023 07:36

qiuming-best force-pushed the volumes-filter-design branch 2 times, most recently from 943f15c to 7f44611 Compare January 17, 2023 13:27

Lyndon-Li reviewed Jan 18, 2023

View reviewed changes

design/handle-backup-of-volumes-by-resources-filters.md Show resolved Hide resolved

design/handle-backup-of-volumes-by-resources-filters.md Show resolved Hide resolved

design/handle-backup-of-volumes-by-resources-filters.md Outdated Show resolved Hide resolved

qiuming-best force-pushed the volumes-filter-design branch from 7f44611 to 7833ea5 Compare January 18, 2023 03:29

reasonerjt reviewed Jan 18, 2023

View reviewed changes

pradeepkchaturvedi reviewed Jan 19, 2023

View reviewed changes

design/handle-backup-of-volumes-by-resources-filters.md Show resolved Hide resolved

design/handle-backup-of-volumes-by-resources-filters.md Outdated Show resolved Hide resolved

reasonerjt mentioned this pull request Jan 30, 2023

Velero should provide a mechanism to support finer-grained granular restore #5803

Open

reasonerjt reviewed Jan 30, 2023

View reviewed changes

design/handle-backup-of-volumes-by-resources-filters.md Outdated Show resolved Hide resolved

blackpiglet mentioned this pull request Jan 30, 2023

Design to add support for Multiple VolumeSnapshotClasses in CSI Plugin #5774

Merged

3 tasks

reasonerjt reviewed Jan 30, 2023

View reviewed changes

design/handle-backup-of-volumes-by-resources-filters.md Outdated Show resolved Hide resolved

reasonerjt reviewed Jan 30, 2023

View reviewed changes

design/handle-backup-of-volumes-by-resources-filters.md Outdated Show resolved Hide resolved

reasonerjt reviewed Jan 30, 2023

View reviewed changes

design/handle-backup-of-volumes-by-resources-filters.md Outdated Show resolved Hide resolved

reasonerjt reviewed Jan 30, 2023

View reviewed changes

design/handle-backup-of-volumes-by-resources-filters.md Outdated Show resolved Hide resolved

qiuming-best force-pushed the volumes-filter-design branch 9 times, most recently from dfe780e to 641b48e Compare February 15, 2023 07:43

gsadhani reviewed Feb 15, 2023

View reviewed changes

rnarenpujari reviewed Feb 15, 2023

View reviewed changes

qiuming-best force-pushed the volumes-filter-design branch 5 times, most recently from cfcf707 to bcb501b Compare February 16, 2023 08:55

ihcsim reviewed Feb 17, 2023

View reviewed changes

qiuming-best force-pushed the volumes-filter-design branch 2 times, most recently from 7f9ec68 to b11b839 Compare March 1, 2023 03:27

reasonerjt previously approved these changes Mar 1, 2023

View reviewed changes

shubham-pampattiwar reviewed Mar 2, 2023

View reviewed changes

Add API Design

d72e88a

Signed-off-by: Ming <[email protected]>

qiuming-best dismissed reasonerjt’s stale review via d72e88a March 2, 2023 07:53

qiuming-best force-pushed the volumes-filter-design branch from b11b839 to d72e88a Compare March 2, 2023 07:53

qiuming-best mentioned this pull request Mar 7, 2023

Handle backup of volume by resource policies #5901

Merged

3 tasks

sseago approved these changes Mar 9, 2023

View reviewed changes

shubham-pampattiwar approved these changes Mar 9, 2023

View reviewed changes

qiuming-best merged commit 54042c3 into vmware-tanzu:main Mar 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Design for Handling backup of volumes by resources filters #5773

Design for Handling backup of volumes by resources filters #5773

qiuming-best commented Jan 17, 2023

codecov-commenter commented Jan 17, 2023 •

edited

Loading

qiuming-best commented Jan 18, 2023

reasonerjt Jan 18, 2023

qiuming-best Jan 30, 2023

gsadhani Feb 15, 2023

qiuming-best Feb 16, 2023

reasonerjt Feb 16, 2023

ihcsim Feb 17, 2023

rnarenpujari Feb 15, 2023

qiuming-best Feb 16, 2023

reasonerjt Feb 16, 2023

rnarenpujari Feb 17, 2023

ihcsim Feb 17, 2023

ihcsim commented Feb 15, 2023 •

edited

Loading

reasonerjt commented Feb 16, 2023

ihcsim Feb 17, 2023

reasonerjt Feb 19, 2023

ihcsim Feb 17, 2023

ihcsim Feb 17, 2023

ihcsim Feb 17, 2023

reasonerjt Feb 19, 2023

qiuming-best Feb 20, 2023 •

edited

Loading

ihcsim Feb 20, 2023 •

edited

Loading

reasonerjt left a comment

shubham-pampattiwar left a comment

shubham-pampattiwar Mar 2, 2023

qiuming-best Mar 2, 2023

shubham-pampattiwar Mar 2, 2023

qiuming-best Mar 2, 2023 •

edited

Loading

shubham-pampattiwar Mar 2, 2023

qiuming-best Mar 2, 2023

shubham-pampattiwar left a comment


		Volume policies are a list and if the target volumes match the first policy, the latter will be ignored, which would reduce the complexity of matching volumes especially when there are multiple complex volumes policies.

		#### Action

Design for Handling backup of volumes by resources filters #5773

Design for Handling backup of volumes by resources filters #5773

Conversation

qiuming-best commented Jan 17, 2023

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

codecov-commenter commented Jan 17, 2023 • edited Loading

Codecov Report

qiuming-best commented Jan 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihcsim commented Feb 15, 2023 • edited Loading

reasonerjt commented Feb 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiuming-best Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

ihcsim Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

reasonerjt left a comment

Choose a reason for hiding this comment

shubham-pampattiwar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiuming-best Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham-pampattiwar left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 17, 2023 •

edited

Loading

ihcsim commented Feb 15, 2023 •

edited

Loading

qiuming-best Feb 20, 2023 •

edited

Loading

ihcsim Feb 20, 2023 •

edited

Loading

qiuming-best Mar 2, 2023 •

edited

Loading