Skip to content

Conversation

@deepakkinni
Copy link
Collaborator

@deepakkinni deepakkinni commented Nov 17, 2025

What this PR does / why we need it:
Configures snapshot limits via ConfigMap. The snapshot limit enforcement reads configuration from a ConfigMap named cns-csi-limits in each namespace with key csi.vsphere.max-snapshots-per-volume.

Design decisions:

  • ConfigMap location: Per-namespace (allows tenant-level customization)
  • Default behavior: If ConfigMap not found, uses default limit of 4
  • Validation: If ConfigMap exists but key is missing, request fails to prevent misconfiguration
  • Clamping: Values exceeding absolute max (32) are clamped with a warning
  • Invalid values (negative or non-numeric) fail the request immediately
  • Per-volume locking ensures serialized snapshot operations

Which issue this PR fixes: fixes #

Testing done:

Precheckins:
WCP(PASS): https://jenkins-vcf-csifvt.devops.broadcom.net/job/wcp-instapp-e2e-pre-checkin/748/
VKS(PASS): https://jenkins-vcf-csifvt.devops.broadcom.net/view/instapp/job/vks-instapp-e2e-pre-checkin/

=== RUN   TestGetSnapshotLimitForNamespace
=== RUN   TestGetSnapshotLimitForNamespace/WhenConfigMapExists_ValidValue
=== RUN   TestGetSnapshotLimitForNamespace/WhenConfigMapExists_ValueEqualsMax
=== RUN   TestGetSnapshotLimitForNamespace/WhenConfigMapExists_ValueExceedsMax
=== RUN   TestGetSnapshotLimitForNamespace/WhenConfigMapExists_ValueIsZero
=== RUN   TestGetSnapshotLimitForNamespace/WhenConfigMapExists_ValueIsNegative
=== RUN   TestGetSnapshotLimitForNamespace/WhenConfigMapExists_InvalidFormat
=== RUN   TestGetSnapshotLimitForNamespace/WhenConfigMapExists_MissingKey
=== RUN   TestGetSnapshotLimitForNamespace/WhenConfigMapNotFound
=== RUN   TestGetSnapshotLimitForNamespace/WhenK8sClientCreationFails
--- PASS: TestGetSnapshotLimitForNamespace (0.00s)
    --- PASS: TestGetSnapshotLimitForNamespace/WhenConfigMapExists_ValidValue (0.00s)
    --- PASS: TestGetSnapshotLimitForNamespace/WhenConfigMapExists_ValueEqualsMax (0.00s)
    --- PASS: TestGetSnapshotLimitForNamespace/WhenConfigMapExists_ValueExceedsMax (0.00s)
    --- PASS: TestGetSnapshotLimitForNamespace/WhenConfigMapExists_ValueIsZero (0.00s)
    --- PASS: TestGetSnapshotLimitForNamespace/WhenConfigMapExists_ValueIsNegative (0.00s)
    --- PASS: TestGetSnapshotLimitForNamespace/WhenConfigMapExists_InvalidFormat (0.00s)
    --- PASS: TestGetSnapshotLimitForNamespace/WhenConfigMapExists_MissingKey (0.00s)
    --- PASS: TestGetSnapshotLimitForNamespace/WhenConfigMapNotFound (0.00s)
    --- PASS: TestGetSnapshotLimitForNamespace/WhenK8sClientCreationFails (0.00s)
PASS
ok  	sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/wcp	0.976s


Ran Tests on real setup:
Test #1: Under Limit - ConfigMap=5, create 3 snapshots → ✅ PASSED
Test #2: At Limit Minus One - ConfigMap=5, existing=3, create 4th → ✅ PASSED
Test #3: At Limit - ConfigMap=5, existing=5 READY, try create 6th → ✅ PASSED (correctly denied)
Test #4: Over Limit - ConfigMap=3, existing=4 READY, try create 5th → ✅ PASSED (correctly denied)
Test #5: No ConfigMap - Use Default - No ConfigMap, create 3 snapshots → ✅ PASSED
Test #6: Valid ConfigMap Value - ConfigMap=3, create 2 snapshots → ✅ PASSED
Test #7: Absolute Max Enforcement - ConfigMap=100, webhook caps at 32, create 32, 33rd denied → ✅ PASSED
Test #8: Count Multiple Snapshots - ConfigMap=10, create 9 snapshots, verify all counted → ✅ PASSED
Test #9: Different PVCs Same Namespace - ConfigMap=3, PVC1 has 2, PVC2 has 2 → ✅ PASSED
Test #10: Different PVCs Independent Limits - ConfigMap=3, PVC1 has 3, PVC2 create 2 → ✅ PASSED
Test #11: Empty Snapshot List - ConfigMap=3, no existing snapshots, create first → ✅ PASSED
Test #12: Delete and Recreate - ConfigMap=2, create 2, delete 1, create another → ✅ PASSED
Test #13: Rapid Creation - ConfigMap=5, rapidly create 5 snapshots sequentially → ✅ PASSED
Test #14: Error Message Content - ConfigMap=2, existing=2, verify error message format → ✅ PASSED

Special notes for your reviewer:

  • No RBAC changes required (controller already has ConfigMap read permissions)
  • If ConfigMap not found, defaults to limit of 4
  • If ConfigMap exists but key is missing, request fails (intentional)

Release note:

Snapshot limits are now configured via ConfigMap cns-csi-limits

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 17, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deepakkinni

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 17, 2025
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2025
@deepakkinni deepakkinni force-pushed the topic/dk016388/snap_limit_csi_v1 branch from 389d8fa to 952d1d4 Compare December 10, 2025 09:00
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 10, 2025
@deepakkinni deepakkinni force-pushed the topic/dk016388/snap_limit_csi_v1 branch from 952d1d4 to ec351d4 Compare December 10, 2025 09:25
@deepakkinni
Copy link
Collaborator Author

/retest

@deepakkinni deepakkinni force-pushed the topic/dk016388/snap_limit_csi_v1 branch from ec351d4 to 4cd7768 Compare December 11, 2025 19:33
@xing-yang
Copy link
Contributor

xing-yang commented Dec 12, 2025

In Testing Results, please add pre-check pipelines and manual testing results.
In PR description, please clarify this is for WCP only.
Release Note section needs to be in this format:
Screenshot 2025-12-12 at 9 45 14 AM

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants