Add Design Proposal To Extend Resource Policy With Field Selector Filter #5842

ihcsim · 2023-02-07T21:52:39Z

Please add a summary of your change

This design proposal extends the new ResourcePolicies resource with field selector properties so that users can define filter criteria based on resource properties like resource name.

Does your change fix a particular issue?

Fixes #5152
Fixes #5118

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.

design/resource-filter-field-selectors-design.md

ihcsim · 2023-09-23T22:36:56Z

@qiuming-best Apologies for the delay. I have updated the proposal to use the ResourcePolicies API. PTAL - thanks.

changelogs/unreleased/5841-isim

qiuming-best · 2023-10-18T10:49:25Z

design/resource-filter-field-selectors-design.md

+```
+
+### Code Design
+


@ihcsim
The API above looks good, and could you please give more details in code design, how and where to filter resources when doing backup in Velero

Sure thing - I will take a stab at it. I am still unfamiliar with Velero code base, might take slightly longer.

qiuming-best · 2023-10-18T10:54:39Z

design/resource-filter-field-selectors-design.md

+
+## Compatibility
+
+The field selector mechanism only works with the new `ResourcePolicies` API.


Please add the conflict resolution about the FieldSelectorPolicy with current existing rules.

such as if the FieldSelectorPolicy selected one resource and Action is Skip but the user used --include-resources when backup

Is it necessary for field selector policy to support action? At the moment, action seems to be specific to volume policy. Also, like labels, the K8s field selector API only works with = and !=. It doesn't support the equivalence of Skip.

kaovilai · 2023-10-18T16:19:14Z

design/resource-filter-field-selectors-design.md

+## Background
+
+The current mechanism to include cluster-scoped resources in backups via the `IncludeResources`, `IncludeClusterResources` and `IncludeClusterScopedResources` API filters the resources by their [`ResourceGroup`][3].
+This is insufficent for operator backup use cases, where specific Custom Resource Definition (CRD) must be included in order for the restored operator to work.


This is awesome. Restore of operators have been requested rather frequently.

shawn-hurley · 2023-10-18T18:40:21Z

design/resource-filter-field-selectors-design.md

+velero backup create --include-cluster-scoped-resources="customresourcedefinitions" --resource-policies-configmap=backup-policy-nginx-crd
+```
+
+Although the supported field selectors vary across Kubernetes resource types, all resource types support the `metadata.name` and `metadata.namespace` fields.


Are we suggesting that we only allow these two in our API?

If we allow others that a given resource type supports if there is an error, do we error the backup at that point? Is there someway to validate the request during backup resource creation vs during runtime?

The goal is to let K8s tells us which field selectors are supported for a particular resource group, by e.g., specifying the field selectors in the metav1.ListOptions. Velero shouldn't need to maintain any knowledge of which field selectors are supported server-side.

As for error handling, not sure if it's worth doing more upfront client-side validation, since K8s already has a well-defined API for field selections. I think we can just let Kubernetes handle it, like how kubectl get works. If we got an error from K8s, Velero should fail the backup.

I am not super worried about the syntax, either. I am worried about someone attempting to use a FiieldSelector that is not valid, a backup or schedule is created, and until the code makes that specific call, it is not known that it is broken. For instance, kubectl get will tell you right away if the field selector you are using is invalid.

I was hoping that a list with/ the ListOption, when the resource is requested, or some other type of admission thing would give the user much quicker feedback on the invalid FieldSelector.

Maybe this is too much work for this one enhancement, but I do start to wonder if we are getting to the point when the admission plugin/webhook is going to help with the overall user experience.

Or some form of dry run option for the overall resource policy may help too? Personally, I am not a fan of admission webhooks because they can be ~~disabled~~ ignored from the webhook configuration, they can go down, they don't load-balance correctly in HA etc.

dry run to validate valid spec sounds good to me.

I think that a dry-run command could take the CR and validate the FieldSelectors against a given cluster, would make sense as an option, and would give you a similar feel to the kubectl get.

At some point, I do wonder if the project is going to have to consider a webhook, but for this, I think the proposed idea of a dry run makes sense and would alleviate my concerns

codecov · 2023-10-26T07:38:51Z

Codecov Report

Merging #5842 (682181f) into main (23921e5) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 682181f differs from pull request most recent head 10663ca. Consider uploading reports for the commit 10663ca to get more accurate results

@@            Coverage Diff             @@
##             main    #5842      +/-   ##
==========================================
+ Coverage   61.00%   61.03%   +0.03%     
==========================================
  Files         252      251       -1     
  Lines       26898    26837      -61     
==========================================
- Hits        16409    16381      -28     
+ Misses       9326     9303      -23     
+ Partials     1163     1153      -10

see 16 files with indirect coverage changes

…Policies configuration Signed-off-by: Ivan Sim <[email protected]>

Signed-off-by: Ivan Sim <[email protected]>

Fix typo Re-add changelog Signed-off-by: Ivan Sim <[email protected]>

Signed-off-by: Ivan Sim <[email protected]>

reasonerjt · 2023-10-30T02:40:44Z

Is there expectation to merge it in v1.13 timeframe, please be reminded this was not on our radar during v1.13 planning and we've passed v1.13 Feature Freeze milestone. Unless we decide this is very important, this will be left in backlog.

Also there are other work planned for v1.13 around the resource policy..

ihcsim · 2023-10-30T03:08:47Z

@reasonerjt No, this is not needed for v1.13, unless others feel otherwise.

ihcsim · 2024-02-03T20:10:00Z

Closing this for now, as I am no longer sure if this is the right approach to backup an operator. Ideally, we want a way to identify and backup all resources required by an operator. The proposed approach is a bit too granular.

github-actions bot assigned ihcsim Feb 7, 2023

github-actions bot requested review from Lyndon-Li and reasonerjt February 7, 2023 21:52

github-actions bot added Area/Design Design Documents has-changelog labels Feb 7, 2023

reasonerjt requested a review from qiuming-best February 13, 2023 06:35

reasonerjt self-assigned this Feb 13, 2023

ihcsim mentioned this pull request Feb 15, 2023

Design for Handling backup of volumes by resources filters #5773

Merged

3 tasks

OrlinVasilev requested a review from sseago February 21, 2023 15:04

reasonerjt added the 1.12-candidate label Apr 11, 2023

qiuming-best reviewed Apr 18, 2023

View reviewed changes

design/resource-filter-field-selectors-design.md Outdated Show resolved Hide resolved

ihcsim force-pushed the design_resource_filter_field_selectors branch from 3b94981 to 1f59bed Compare September 23, 2023 22:35

ihcsim force-pushed the design_resource_filter_field_selectors branch 2 times, most recently from c62ec87 to 7d81a4a Compare September 23, 2023 22:44

kaovilai reviewed Oct 12, 2023

View reviewed changes

changelogs/unreleased/5841-isim Show resolved Hide resolved

github-actions bot removed the has-changelog label Oct 17, 2023

qiuming-best reviewed Oct 18, 2023

View reviewed changes

kaovilai reviewed Oct 18, 2023

View reviewed changes

shawn-hurley reviewed Oct 18, 2023

View reviewed changes

ihcsim changed the title ~~Add Design Proposal To Extend Resource Filters With Field Selectors~~ Add Design Proposal To Extend Resource Policy With Field Selector Filter Oct 18, 2023

qiuming-best added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Oct 26, 2023

github-actions bot added the has-changelog label Oct 28, 2023

ihcsim and others added 3 commits October 28, 2023 11:12

Add design proposal to extend ResourcePolicies API with FieldSelector…

0b71c4b

…Policies configuration Signed-off-by: Ivan Sim <[email protected]>

Removed unneeded changelog

1c4f942

Signed-off-by: Ivan Sim <[email protected]>

Address CI failures

2c40c25

Fix typo Re-add changelog Signed-off-by: Ivan Sim <[email protected]>

ihcsim force-pushed the design_resource_filter_field_selectors branch from 6627712 to 2c40c25 Compare October 28, 2023 18:12

Define compatibility between resource filers and field selector policy

10663ca

Signed-off-by: Ivan Sim <[email protected]>

ihcsim closed this Feb 3, 2024

yuanqijing mentioned this pull request May 23, 2024

Request for Custom Configuration to Enhance Resource Filtering in Velero #7815

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Design Proposal To Extend Resource Policy With Field Selector Filter #5842

Add Design Proposal To Extend Resource Policy With Field Selector Filter #5842

ihcsim commented Feb 7, 2023 •

edited

Loading

ihcsim commented Sep 23, 2023

qiuming-best Oct 18, 2023

ihcsim Oct 18, 2023

qiuming-best Oct 18, 2023

ihcsim Oct 28, 2023

kaovilai Oct 18, 2023

shawn-hurley Oct 18, 2023

ihcsim Oct 18, 2023

shawn-hurley Oct 19, 2023

ihcsim Oct 19, 2023 •

edited

Loading

kaovilai Oct 19, 2023

shawn-hurley Oct 23, 2023

codecov bot commented Oct 26, 2023 •

edited

Loading

reasonerjt commented Oct 30, 2023

ihcsim commented Oct 30, 2023

ihcsim commented Feb 3, 2024


		## Compatibility

		The field selector mechanism only works with the new `ResourcePolicies` API.

		```

		### Code Design

Add Design Proposal To Extend Resource Policy With Field Selector Filter #5842

Add Design Proposal To Extend Resource Policy With Field Selector Filter #5842

Conversation

ihcsim commented Feb 7, 2023 • edited Loading

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

ihcsim commented Sep 23, 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

ihcsim Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 26, 2023 • edited Loading

Codecov Report

reasonerjt commented Oct 30, 2023

ihcsim commented Oct 30, 2023

ihcsim commented Feb 3, 2024

ihcsim commented Feb 7, 2023 •

edited

Loading

ihcsim Oct 19, 2023 •

edited

Loading

codecov bot commented Oct 26, 2023 •

edited

Loading