Skip to content

Conversation

@shubham-pampattiwar
Copy link
Collaborator

@shubham-pampattiwar shubham-pampattiwar commented May 16, 2025

Thank you for contributing to Velero!

Please add a summary of your change

  • Adds support in GetRelatedItems for grouping PVCs using the VGS label key provided.
  • Related PVCs in the same VGS group are included in the ItemBlock to ensure atomic snapshot coverage.

Does your change fix a particular issue?

Fixes #8935

Related to sub-task of #8865

Depends on merging #8938

Please indicate you've done the following:

@codecov
Copy link

codecov bot commented May 16, 2025

Codecov Report

Attention: Patch coverage is 85.10638% with 7 lines in your changes missing coverage. Please review.

Project coverage is 59.84%. Comparing base (41a6922) to head (97a4d62).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
pkg/itemblock/actions/pvc_action.go 84.44% 5 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8944      +/-   ##
==========================================
+ Coverage   59.81%   59.84%   +0.02%     
==========================================
  Files         375      375              
  Lines       41056    41100      +44     
==========================================
+ Hits        24557    24595      +38     
- Misses      15010    15014       +4     
- Partials     1489     1491       +2     

☔ 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.

@shubham-pampattiwar shubham-pampattiwar force-pushed the vgs-task-2 branch 2 times, most recently from 74b18b8 to f23f3b9 Compare May 30, 2025 15:17
@shubham-pampattiwar shubham-pampattiwar marked this pull request as ready for review May 30, 2025 15:17
@github-actions github-actions bot requested review from sseago and ywk253100 May 30, 2025 15:17
@kaovilai
Copy link
Collaborator

kaovilai commented May 30, 2025

Code Review Summary ✅

Great work on implementing VGS support in the PVC ItemBlockAction plugin! This is a solid implementation that addresses issue #8935 requirements.

Overall Assessment:

  • Clean implementation with proper separation of concerns
  • Excellent test coverage covering edge cases
  • Good error handling and logging
  • Bug fix included (method name correction)
  • Efficient Kubernetes client usage

Key Strengths:

  1. Namespace scoping: Correctly limits PVC search to the same namespace
  2. Label-based grouping: Uses efficient MatchingLabels client filtering
  3. Self-exclusion: Properly excludes the current PVC from grouped results
  4. Graceful degradation: Handles missing VGS configuration cleanly

Minor Enhancement Opportunities (within PR scope):

I've added specific code review comments for:

  • Empty group value validation
  • Enhanced error context for debugging
  • Performance optimizations (early returns, pre-allocated slices)

Test Coverage:

The test suite is comprehensive. Consider adding one more edge case for empty group values to complete the coverage.

Looking forward to seeing this land! 🚀

@shubham-pampattiwar
Copy link
Collaborator Author

@sseago Addressed #8938 (review) in this PR !

kaovilai
kaovilai previously approved these changes Jun 2, 2025
Copy link
Collaborator

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

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

LGTM!

…meGroupSnapshot label

Signed-off-by: Shubham Pampattiwar <[email protected]>

Add changelog file

Signed-off-by: Shubham Pampattiwar <[email protected]>

Update VGS label key and address PR feedback

Signed-off-by: Shubham Pampattiwar <[email protected]>

update log level to debug for edge cases

Signed-off-by: Shubham Pampattiwar <[email protected]>

Change VGS label key constant location

Signed-off-by: Shubham Pampattiwar <[email protected]>

run make update

Signed-off-by: Shubham Pampattiwar <[email protected]>
@shubham-pampattiwar
Copy link
Collaborator Author

@Lyndon-Li @blackpiglet @kaovilai PR review request !

@shubham-pampattiwar shubham-pampattiwar merged commit 0df773b into vmware-tanzu:main Jun 11, 2025
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Changes to the Existing PVC ItemBlockAction plugin

4 participants