Skip to content

Conversation

@kaovilai
Copy link
Collaborator

@kaovilai kaovilai commented Sep 3, 2025

This design proposal addresses issue #7492 by modifying Velero's backup
behavior to include all resources from namespaces selected by labelSelector
or orLabelSelectors, equivalent to explicitly listing them in
includedNamespaces.

Key design decisions:

  • Modify existing LabelSelector behavior (breaking change)
  • Precedence: (includedNamespaces ∪ labelSelector ∪ orLabelSelectors) - excludedNamespaces
  • Support full Kubernetes label selector syntax
  • Enhanced logging for namespace selection transparency

🤖 Generated with Claude Code

Co-Authored-By: Claude [email protected]

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Design for #7492

Please indicate you've done the following:

@github-actions github-actions bot requested a review from blackpiglet September 3, 2025 18:37
@kaovilai kaovilai changed the title Add design document for namespace selection by label selector Design: namespace selection by label selector Sep 3, 2025
@github-actions github-actions bot added the Area/Design Design Documents label Sep 3, 2025
@codecov
Copy link

codecov bot commented Sep 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.19%. Comparing base (c594026) to head (04ebf07).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9223   +/-   ##
=======================================
  Coverage   60.19%   60.19%           
=======================================
  Files         386      386           
  Lines       35925    35925           
=======================================
  Hits        21624    21624           
  Misses      12720    12720           
  Partials     1581     1581           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

kaovilai and others added 2 commits September 10, 2025 14:20
This design proposal addresses issue vmware-tanzu#7492 by modifying Velero's backup
behavior to include all resources from namespaces selected by labelSelector
or orLabelSelectors, equivalent to explicitly listing them in
includedNamespaces.

Key design decisions:
- Modify existing LabelSelector behavior (breaking change)
- Precedence: (includedNamespaces ∪ labelSelector ∪ orLabelSelectors) - excludedNamespaces
- Support full Kubernetes label selector syntax
- Enhanced logging for namespace selection transparency

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Remove multi-phase approach as requested - combine all implementation
work into a single comprehensive plan with 6-week timeline.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Clean up markdown formatting issues detected by linter including:
- Add proper blank lines around lists and headings
- Add trailing newline at end of file
- Improve code block formatting consistency

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Copilot AI review requested due to automatic review settings December 10, 2025 06:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a design proposal to modify Velero's backup behavior for namespace selection using label selectors. The design addresses issue #7492, proposing that namespaces matching LabelSelector or OrLabelSelectors should be treated as if explicitly listed in includedNamespaces, causing all resources within those namespaces to be backed up (rather than only individually labeled resources).

Key changes:

  • Proposes modifying nsTracker behavior in pkg/backup/item_collector.go to treat label-selected namespaces as fully included
  • Defines precedence formula: (includedNamespaces ∪ labelSelector ∪ orLabelSelectors) - excludedNamespaces
  • Documents breaking change impact and migration considerations

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
design/namespace-label-selector-backup_design.md Complete design document proposing namespace selection by label selector functionality with implementation details, alternatives, security considerations, and compatibility analysis
changelogs/unreleased/9223-kaovilai Changelog entry documenting the design proposal

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +180 to +190
Backup status will include selected namespace information:

```yaml
status:
phase: Completed
namespaces:
included: ["ns1", "ns3", "ns4", "ns5", "ns6"]
explicitlyIncluded: ["ns1"]
selectedByLabels: ["ns3", "ns4", "ns5", "ns6"]
excluded: ["ns2", "temp-ns"]
```
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

The proposed status fields (status.namespaces.included, status.namespaces.explicitlyIncluded, status.namespaces.selectedByLabels, status.namespaces.excluded) would require adding new fields to the BackupStatus struct. However, the "API Schema" section at lines 107-117 states "No Changes Required", which is contradictory. This design should explicitly document the new status fields as an API addition, similar to how the wildcard namespace design documents status field additions (see design/wildcard-namespace-support-design.md:46-69).

Copilot uses AI. Check for mistakes.
// Namespace matches selector -> track AND include all resources in namespace
if nt.singleLabelSelector != nil && nt.singleLabelSelector.Matches(labels.Set(namespace.GetLabels())) {
nt.track(namespace.GetName())
// This namespace now behaves like it's in includedNamespaces
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

The "New behavior" code example is identical to the "Current behavior" except for the comment. This doesn't demonstrate what actual code changes are needed. The example should show concrete implementation changes, such as how the tracked namespace will be treated as if it's in includedNamespaces, or reference the specific mechanism that will be modified. Consider showing actual before/after code that illustrates the behavioral change.

Suggested change
// This namespace now behaves like it's in includedNamespaces
nt.includeNamespace(namespace.GetName()) // NEW: treat as if in includedNamespaces

Copilot uses AI. Check for mistakes.
Comment on lines +30 to +32
When a namespace matches `spec.labelSelector` or any `spec.orLabelSelectors`, Velero will treat that namespace as if it were explicitly listed in `spec.includedNamespaces`.
All resources within matching namespaces will be included in the backup, regardless of their individual labels.
The final namespace selection follows the precedence: `(includedNamespaces ∪ labelSelector_matches ∪ orLabelSelector_matches) - excludedNamespaces`.
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

The design describes labelSelector and orLabelSelectors working together in the union formula, and line 197 mentions a warning for "Using both labelSelector and orLabelSelectors". However, the current API documentation in pkg/apis/velero/v1/backup_types.go:98-99 explicitly states: "LabelSelector as well as OrLabelSelectors cannot co-exist in backup request, only one of them can be used." This design must explicitly address whether this mutual exclusivity constraint is being removed as part of this change, or if the formula should be adjusted to reflect the OR relationship between these fields.

Copilot uses AI. Check for mistakes.

### Issue 2: Complex Selector User Education

Users may create overly complex selectors that are difficult to understand or maintain.
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

Issue 2 is incomplete - it identifies a concern but doesn't include a "Potential Solution" like Issue 1 does. Consider adding potential mitigation strategies such as: providing clear documentation with examples, implementing validation warnings for overly complex selectors, or offering CLI commands to preview namespace selection before execution.

Suggested change
Users may create overly complex selectors that are difficult to understand or maintain.
Users may create overly complex selectors that are difficult to understand or maintain.
**Potential Solution:** Provide clear documentation with practical examples of label selectors, implement validation warnings or linter tools to alert users about overly complex or ambiguous selectors, and offer CLI commands or UI features to preview which namespaces would be selected by a given selector before executing a backup.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant