Add failure domain lifecycle support: backoff reset, orphan cleanup, …#348
Draft
jcpowermac wants to merge 2 commits into
Draft
Add failure domain lifecycle support: backoff reset, orphan cleanup, …#348jcpowermac wants to merge 2 commits into
jcpowermac wants to merge 2 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jcpowermac The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe controller now tracks per-vCenter backoff and orphan state, adds day-2 orphan cleanup with PV-safety checks, threads feature-gate configuration through vSphere wiring, and expands unit and e2e coverage for topology and failure-domain lifecycle behavior. ChangesStorage policy day-2 cleanup
Dependency updates
Estimated code review effort: 4 (Complex) | ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/operator/storageclasscontroller/storageclasscontroller.go (1)
167-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBackoff reset happens after scheduling, so a successful sync can still wait at the capped delay.
At Line 167-Line 170,
nextCheckis computed before the success check, and only then reset at Line 175. After repeated failures, the first success can still schedule the next run near 30 minutes, instead of restarting from ~1 minute. Move scheduling into success/failure branches and compute the success delay fromdefaultBackoff.Suggested fix
- nextRunDelay := c.backoff.Step() - c.lastCheck = time.Now() - c.nextCheck = c.lastCheck.Add(nextRunDelay) - - if err != nil { - return "", checks.MakeGenericVCenterAPIError(err) - } - // Reset backoff on success — next sync restarts at 1-minute interval - c.backoff = defaultBackoff + c.lastCheck = time.Now() + if err != nil { + nextRunDelay := c.backoff.Step() + c.nextCheck = c.lastCheck.Add(nextRunDelay) + return "", checks.MakeGenericVCenterAPIError(err) + } + // Reset backoff on success and schedule next run from default interval. + c.backoff = defaultBackoff + c.nextCheck = c.lastCheck.Add(wait.Jitter(defaultBackoff.Duration, defaultBackoff.Jitter)) return policyName, checks.MakeClusterCheckResultPass()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/storageclasscontroller/storageclasscontroller.go` around lines 167 - 175, The scheduling (c.lastCheck/c.nextCheck) is computed before checking err, so after failures a subsequent success still uses the large backoff; move the time scheduling into the success/failure branches inside the function (where err is evaluated) so that on error you call c.backoff = c.backoff.Step(), set c.lastCheck = time.Now() and c.nextCheck = c.lastCheck.Add(c.backoff) (and return the MakeGenericVCenterAPIError), while on success you reset c.backoff = defaultBackoff and compute c.lastCheck = time.Now() and c.nextCheck = c.lastCheck.Add(defaultBackoff) so the next sync restarts at the short interval; update locations referencing c.backoff, c.lastCheck, c.nextCheck, defaultBackoff, and checks.MakeGenericVCenterAPIError accordingly.
🧹 Nitpick comments (1)
pkg/operator/storageclasscontroller/vmware.go (1)
626-629: ⚡ Quick winSilently ignoring errors when listing datastores may hide issues.
If
DatastoreListfails for a datacenter (line 626-627), the error is silently swallowed withcontinue. This could mask connectivity issues or permission problems, making debugging harder if a datastore's datacenter cannot be resolved.Consider logging at debug level before continuing:
Suggested fix
dsList, err := dcFinder.DatastoreList(ctx, "*") if err != nil { + klog.V(4).Infof("Could not list datastores in datacenter %s: %v", dc.Name(), err) continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/storageclasscontroller/vmware.go` around lines 626 - 629, The loop silently swallows errors from dcFinder.DatastoreList(ctx, "*"); instead, before the continue log the failure at debug/verbose level including the datacenter identifier and the error (e.g. reference dcFinder or the local datacenter variable and err) so you can still continue on error but capture context; update the block around dcFinder.DatastoreList(ctx, "*") to call the existing logger (or klog) with a descriptive message plus err, then continue as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/storageclasscontroller/vmware.go`:
- Around line 519-527: The orphanedDatastore.HasBoundPVs flag is never set in
findOrphanedTags, so detachOrphanTags' safety check cannot skip datastores with
bound PVs; modify findOrphanedTags to determine whether each orphaned datastore
has any bound PersistentVolumes (e.g., query the Kubernetes API for PVs that
reference the datastore identifier or use any existing helper that maps PVs to
datastores) and set orphanedDatastore.HasBoundPVs accordingly when constructing
each entry; keep the rest of the logic in detachOrphanTags unchanged so its
existing check on HasBoundPVs will correctly skip detaching datastores that have
bound PVs.
- Around line 562-567: The orphan-detection builds currentFDs from
v.infra.Spec.PlatformSpec.VSphere.FailureDomains which includes failure domains
for all vCenters; change the logic (in the method that populates currentFDs,
e.g., findOrphanedTags) to iterate over the already-filtered v.failureDomains
(set in NewStoragePolicyAPI) and insert fd.Topology.Datacenter + "/" +
fd.Topology.Datastore into currentFDs so only failure domains for this
storagePolicyAPI's vCenter (v.connection.Hostname) are considered.
In `@pkg/operator/vspherecontroller/driver_starter_test.go`:
- Around line 131-152: Tests currently duplicate logic by manually appending
topology args using utils.GetTopologyCategories(...) instead of exercising the
topologyHook, so they won't catch regressions; update the test to call
topologyHook (pass the deployment.Spec.Template.Spec.Containers or the container
slice and the clusterCSIDriver/infraObj as required by topologyHook) and then
assert on the returned/modified container args for the "csi-provisioner"
container: for the branch with >=2 topology categories assert it contains
"--feature-gates=Topology=true" and "--strict-topology", and for the 0/1 branch
assert it contains "--feature-gates=Topology=false"; remove the manual append
logic using utils.GetTopologyCategories(...) and drive both scenarios via
topologyHook to validate container selection and correct flag values.
---
Outside diff comments:
In `@pkg/operator/storageclasscontroller/storageclasscontroller.go`:
- Around line 167-175: The scheduling (c.lastCheck/c.nextCheck) is computed
before checking err, so after failures a subsequent success still uses the large
backoff; move the time scheduling into the success/failure branches inside the
function (where err is evaluated) so that on error you call c.backoff =
c.backoff.Step(), set c.lastCheck = time.Now() and c.nextCheck =
c.lastCheck.Add(c.backoff) (and return the MakeGenericVCenterAPIError), while on
success you reset c.backoff = defaultBackoff and compute c.lastCheck =
time.Now() and c.nextCheck = c.lastCheck.Add(defaultBackoff) so the next sync
restarts at the short interval; update locations referencing c.backoff,
c.lastCheck, c.nextCheck, defaultBackoff, and checks.MakeGenericVCenterAPIError
accordingly.
---
Nitpick comments:
In `@pkg/operator/storageclasscontroller/vmware.go`:
- Around line 626-629: The loop silently swallows errors from
dcFinder.DatastoreList(ctx, "*"); instead, before the continue log the failure
at debug/verbose level including the datacenter identifier and the error (e.g.
reference dcFinder or the local datacenter variable and err) so you can still
continue on error but capture context; update the block around
dcFinder.DatastoreList(ctx, "*") to call the existing logger (or klog) with a
descriptive message plus err, then continue as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fbc47b8f-6095-4076-9c5b-e1654313200f
📒 Files selected for processing (5)
pkg/operator/storageclasscontroller/storageclasscontroller.gopkg/operator/storageclasscontroller/storageclasscontroller_test.gopkg/operator/storageclasscontroller/vmware.gopkg/operator/utils/metric.gopkg/operator/vspherecontroller/driver_starter_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/e2e/failure_domain_lifecycle_test.go (4)
196-199: ⚡ Quick winLog PVC deletion error in cleanup.
The Delete error is silently ignored. While cleanup failures shouldn't fail the test, logging them helps diagnose issues when tests leave artifacts behind.
Suggested fix
defer func() { t.Log("Cleaning up test PVC") - kubeClient.CoreV1().PersistentVolumeClaims(ns).Delete(ctx, pvcName, metav1.DeleteOptions{}) + if err := kubeClient.CoreV1().PersistentVolumeClaims(ns).Delete(ctx, pvcName, metav1.DeleteOptions{}); err != nil { + t.Logf("Warning: failed to delete test PVC %s: %v", pvcName, err) + } }()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/failure_domain_lifecycle_test.go` around lines 196 - 199, The deferred cleanup currently ignores errors from the PVC deletion; change the defer func so it captures the returned error from kubeClient.CoreV1().PersistentVolumeClaims(ns).Delete(ctx, pvcName, metav1.DeleteOptions{}) and log it (e.g., via t.Logf or t.Errorf) with context including ns and pvcName; keep the cleanup non-fatal (do not call t.FailNow) but ensure any non-nil err is reported so test artifacts can be diagnosed.Sources: Coding guidelines, Linters/SAST tools
252-270: ⚡ Quick winRemove unused
operatorClientparameter.The
operatorClient *operatorclient.Clientsetparameter is declared but never used within the function body. This appears to be leftover from development.Suggested fix
-func waitForOperatorHealthy(t *testing.T, ctx context.Context, configClient *configclient.Clientset, operatorClient *operatorclient.Clientset) { +func waitForOperatorHealthy(t *testing.T, ctx context.Context, configClient *configclient.Clientset) {Then update all call sites (lines 109, 114, 235, 239) to remove the
ocpOperatorClientargument.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/failure_domain_lifecycle_test.go` around lines 252 - 270, The function signature waitForOperatorHealthy currently declares an unused parameter operatorClient *operatorclient.Clientset; remove that parameter from the function signature and from its parameter list, and update every call site that passes ocpOperatorClient to waitForOperatorHealthy to stop passing that argument so calls match the new signature (keep ctx, t, configClient arguments intact). Ensure imports remain correct after removing the unused parameter.
229-236: ⚡ Quick winHandle errors in deferred FD restore to match the pattern in TestFailureDomainRemovalTagCleanup.
Unlike the first test (lines 96-110) which properly checks and logs errors, this defer silently discards both the
buildFDPatcherror and thePatcherror. This inconsistency makes debugging harder when restores fail.Suggested fix
defer func() { t.Log("Restoring original failure domains") - restoreJSON, _ := buildFDPatch(originalFDs) - configClient.ConfigV1().Infrastructures().Patch( + restoreJSON, err := buildFDPatch(originalFDs) + if err != nil { + t.Errorf("Failed to build restore patch: %v", err) + return + } + _, restoreErr := configClient.ConfigV1().Infrastructures().Patch( ctx, "cluster", types.MergePatchType, restoreJSON, metav1.PatchOptions{}, ) + if restoreErr != nil { + t.Errorf("Failed to restore failure domains: %v", restoreErr) + } waitForOperatorHealthy(t, ctx, configClient, ocpOperatorClient) }()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/failure_domain_lifecycle_test.go` around lines 229 - 236, The deferred restore silently ignores errors; change it to check and log both buildFDPatch and Patch errors like the other test: call buildFDPatch(originalFDs) and if it returns an error log it via t.Logf/t.Errorf and return from the defer, otherwise call configClient.ConfigV1().Infrastructures().Patch(...) and capture its error, logging that error as well; only call waitForOperatorHealthy(t, ctx, configClient, ocpOperatorClient) when the patch succeeds. Reference: buildFDPatch, restoreJSON, configClient.ConfigV1().Infrastructures().Patch, and waitForOperatorHealthy.Sources: Coding guidelines, Linters/SAST tools
283-285: 💤 Low valueLog PVC deletion error.
Consistent with the other cleanup patterns, log the error if deletion fails.
Suggested fix
defer func() { - kubeClient.CoreV1().PersistentVolumeClaims(ns).Delete(ctx, pvcName, metav1.DeleteOptions{}) + if err := kubeClient.CoreV1().PersistentVolumeClaims(ns).Delete(ctx, pvcName, metav1.DeleteOptions{}); err != nil { + t.Logf("Warning: failed to delete test PVC %s: %v", pvcName, err) + } }()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/failure_domain_lifecycle_test.go` around lines 283 - 285, The defer currently drops any error when deleting the PVC; update the defer to call kubeClient.CoreV1().PersistentVolumeClaims(ns).Delete(ctx, pvcName, metav1.DeleteOptions{}) into an error variable and, if non-nil, log it (e.g., using t.Logf or the test logger) with the pvcName, ns and error details so failures are recorded; reference the existing symbols kubeClient.CoreV1().PersistentVolumeClaims, pvcName, ns, ctx and metav1.DeleteOptions{} when making this change.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/e2e/failure_domain_lifecycle_test.go`:
- Around line 196-199: The deferred cleanup currently ignores errors from the
PVC deletion; change the defer func so it captures the returned error from
kubeClient.CoreV1().PersistentVolumeClaims(ns).Delete(ctx, pvcName,
metav1.DeleteOptions{}) and log it (e.g., via t.Logf or t.Errorf) with context
including ns and pvcName; keep the cleanup non-fatal (do not call t.FailNow) but
ensure any non-nil err is reported so test artifacts can be diagnosed.
- Around line 252-270: The function signature waitForOperatorHealthy currently
declares an unused parameter operatorClient *operatorclient.Clientset; remove
that parameter from the function signature and from its parameter list, and
update every call site that passes ocpOperatorClient to waitForOperatorHealthy
to stop passing that argument so calls match the new signature (keep ctx, t,
configClient arguments intact). Ensure imports remain correct after removing the
unused parameter.
- Around line 229-236: The deferred restore silently ignores errors; change it
to check and log both buildFDPatch and Patch errors like the other test: call
buildFDPatch(originalFDs) and if it returns an error log it via t.Logf/t.Errorf
and return from the defer, otherwise call
configClient.ConfigV1().Infrastructures().Patch(...) and capture its error,
logging that error as well; only call waitForOperatorHealthy(t, ctx,
configClient, ocpOperatorClient) when the patch succeeds. Reference:
buildFDPatch, restoreJSON, configClient.ConfigV1().Infrastructures().Patch, and
waitForOperatorHealthy.
- Around line 283-285: The defer currently drops any error when deleting the
PVC; update the defer to call
kubeClient.CoreV1().PersistentVolumeClaims(ns).Delete(ctx, pvcName,
metav1.DeleteOptions{}) into an error variable and, if non-nil, log it (e.g.,
using t.Logf or the test logger) with the pvcName, ns and error details so
failures are recorded; reference the existing symbols
kubeClient.CoreV1().PersistentVolumeClaims, pvcName, ns, ctx and
metav1.DeleteOptions{} when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b24a8bbc-399c-4aa0-a280-33f9fe7235a0
📒 Files selected for processing (5)
pkg/operator/storageclasscontroller/storageclasscontroller.gopkg/operator/storageclasscontroller/storageclasscontroller_test.gopkg/operator/storageclasscontroller/vmware.gopkg/operator/storageclasscontroller/vmware_test.gotest/e2e/failure_domain_lifecycle_test.go
Author
|
@coderabbitai review /test unit |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/vspherecontroller/driver_starter_test.go`:
- Around line 224-225: The call to testlib.AddInitialObjects in
TestTopologyHookSequence currently ignores its error return; capture the
returned error (from testlib.AddInitialObjects(append(initialObjects,
clusterCSIDriver), commonApiClient)) and handle it immediately (e.g., if err !=
nil { t.Fatalf("AddInitialObjects failed: %v", err) } or use your test helper
like require.NoError) before proceeding to testlib.WaitForSync with stopCh so
test setup failures fail fast and don't produce false positives.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 28e5ea56-212b-436a-a091-f8faf31839f8
📒 Files selected for processing (4)
pkg/operator/storageclasscontroller/storageclasscontroller.gopkg/operator/storageclasscontroller/storageclasscontroller_test.gopkg/operator/storageclasscontroller/vmware.gopkg/operator/vspherecontroller/driver_starter_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/operator/storageclasscontroller/storageclasscontroller.go
- pkg/operator/storageclasscontroller/vmware.go
- pkg/operator/storageclasscontroller/storageclasscontroller_test.go
Author
|
/test unit |
Author
|
/test e2e-vsphere-operator-test |
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
test/e2e/failure_domain_lifecycle_test.go (2)
214-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNamespace literal duplicated.
"openshift-cluster-csi-drivers"is hardcoded in two places. Extracting a package-level constant would avoid future drift.♻️ Proposed refactor
+const csiDriverNamespace = "openshift-cluster-csi-drivers" + ... - ns := "openshift-cluster-csi-drivers" + ns := csiDriverNamespaceAlso applies to: 312-312
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/failure_domain_lifecycle_test.go` at line 214, The namespace value is duplicated as a string literal in the failure domain lifecycle tests; extract it into a package-level constant and reuse it everywhere. Update the references in the test helpers/flows around the ns assignment so both occurrences use the same constant, preventing the duplicated literal from drifting.
155-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name implies a transition but no transition is exercised.
TestStorageClassSurvivesTopologyTransitiononly reads the currentthin-csiStorageClass once and checksStoragePolicyNameis non-empty; it never actually mutates failure domains to trigger a topology transition. Consider either renaming to reflect what it checks (a static invariant) or extending it to actually mutate FDs and re-check across a transition, consistent with the other tests in this file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/failure_domain_lifecycle_test.go` around lines 155 - 178, `TestStorageClassSurvivesTopologyTransition` currently only verifies a static `thinCSIStorageClassName` lookup and `StoragePolicyName` value, so either rename the test to reflect that it checks a StorageClass invariant or update it to actually exercise a failure-domain/topology change like the other tests in this file. If you keep the transition behavior, use the existing kube client setup in `TestStorageClassSurvivesTopologyTransition` to mutate FDs and then re-read the StorageClass after the transition; otherwise, adjust the test name and assertions to match the current read-only behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/storageclasscontroller/storageclasscontroller.go`:
- Around line 201-204: The early-return cache check in storageclasscontroller’s
policy lookup is skipping cleanup before force cleanup is considered. Move the
force-cleanup annotation handling in the same flow as the cached-success guard
in the relevant storageclasscontroller method so `forceCleanup` is evaluated
before returning, and only bypass checks when
`len(c.vCenterStoragePolicy[connection.Hostname]) > 0`, there are no pending
orphans, and `forceCleanup` is false.
- Around line 125-129: The stale vCenter cleanup path in storageclasscontroller
should not emit internal hostnames in logs or events. Update the messages in the
code path that removes entries from c.vCenterStoragePolicy, c.backoffStates, and
c.pendingOrphans to use a non-identifying or redacted identifier instead of
host, and apply the same treatment to the StaleVCenterRemoved event so both klog
and c.recorder avoid exposing vCenter hostnames.
- Around line 123-129: The stale-vCenter cleanup in storageclasscontroller.go
only iterates over c.vCenterStoragePolicy, so it can miss hosts that exist only
in c.backoffStates or c.pendingOrphans. Update the cleanup logic around the
host-removal loop to consider all per-vCenter state maps, and when a host is
inactive remove its entries from vCenterStoragePolicy, backoffStates, and
pendingOrphans together so OrphanCleanupPending cannot stay true for a removed
vCenter.
In `@pkg/operator/storageclasscontroller/vmware_test.go`:
- Line 388: The cleanup calls to deleteStoragePolicy in vmware_test are ignoring
returned errors, which violates the error-handling requirement. Update the
affected test cleanup paths to capture and handle the error from
deleteStoragePolicy instead of discarding it, using the apiClient2 and apiClient
references in the test helpers so the cleanup failures are not hidden.
- Around line 357-365: Enable the day-2 cleanup path in this CNS-unavailable
test by setting the storagePolicyAPI’s day2Enabled flag to true in the test
setup so createStoragePolicy exercises orphan cleanup/deletion. Update the test
fixture around apiClient2 and the createStoragePolicy flow to preserve the
existing CNS-unavailable scenario while ensuring the day-2 safety branch is
actually covered.
In `@pkg/operator/storageclasscontroller/vmware.go`:
- Around line 243-249: The orphan-detection path in
storageclasscontroller/vmware.go should block SPBM profile deletion when
`findOrphanedTags` fails. Update the cleanup flow around `findOrphanedTags`,
`detachOrphanTags`, and the deletion branch so an error sets a non-empty
unresolved state or otherwise prevents the delete condition from passing. Make
the same safeguard in the related cleanup block that also uses
`unresolved`/`v.unresolvedOrphans`, ensuring a transient tag lookup error cannot
lead to deleting the policy.
- Around line 264-273: The new logging and events in vmware.go are exposing
internal vCenter/datastore identifiers such as hostnames and
environment-specific names. Update the warning/event paths around
deleteStoragePolicy and the related recorder.Eventf/klog.Warningf calls to
remove or redact those values, using generic wording or placeholder identifiers
instead of real hostnames/datastore names. Apply the same redaction approach
consistently in the other affected log/event blocks referenced by the review so
no internal customer environment details are emitted.
- Around line 266-276: The deletion path in the vmware storage policy cleanup
flow is swallowing failures by always returning an empty name and nil even when
deleteStoragePolicy() errors. Update the logic around deleteStoragePolicy and
the return after the warning so that a failed deletion returns the existing
v.policyName together with the deletion error, while keeping the success path
unchanged. This will let the controller surface the failure and retry with
backoff instead of treating the policy as successfully cleared.
In `@test/e2e/failure_domain_lifecycle_test.go`:
- Around line 403-439: TestVCenterRemovalCleanup is a no-op because it only
detects a multi-vCenter setup and logs, so it never validates stale cleanup
behavior. Update TestVCenterRemovalCleanup to actually mutate the infrastructure
or failure domains and assert the expected cleanup state, reusing the existing
failure-domain/health polling pattern from the other tests (for example, check
for OrphanCleanupPending or operator health after removal). If this scenario
cannot be exercised automatically, change the test to t.Skip with a clear
manual-verification message instead of leaving a passing test with no
assertions.
- Around line 382-401: verifyOrphanConditionPending currently only logs and
never asserts, so the final safety check in TestPVSafetyBlocksCleanup can
silently pass even when the condition never becomes True. Update
verifyOrphanConditionPending to mirror verifyOrphanConditionCleared by treating
a wait.PollUntilContextTimeout timeout as a test failure with t.Errorf or
t.Fatalf, while keeping the success path tied to the
StorageClassControllerOrphanCleanupPending condition lookup in
configClient.ConfigV1().ClusterOperators().Get.
- Around line 211-220: TestPVSafetyBlocksCleanup is creating a plain PVC via
newTestPVC without any consumer, so thin-csi WaitForFirstConsumer may never bind
it and the test can skip instead of exercising orphan cleanup. Update the setup
to create a Pod that mounts/uses the PVC, and if needed add explicit topology
constraints so the claim binds on the intended datastore before waiting. Apply
the same assumption fix in verifyPVCProvisionable so it also provisions
deterministically.
- Around line 359-401: The orphan condition checks in
verifyOrphanConditionCleared and verifyOrphanConditionPending are matching the
wrong condition type string. Update both helpers to use the controller’s actual
condition type, VMwareVSphereDriverStorageClassControllerOrphanCleanupPending,
so the polling logic correctly detects pending and cleared states instead of
silently passing or timing out.
---
Nitpick comments:
In `@test/e2e/failure_domain_lifecycle_test.go`:
- Line 214: The namespace value is duplicated as a string literal in the failure
domain lifecycle tests; extract it into a package-level constant and reuse it
everywhere. Update the references in the test helpers/flows around the ns
assignment so both occurrences use the same constant, preventing the duplicated
literal from drifting.
- Around line 155-178: `TestStorageClassSurvivesTopologyTransition` currently
only verifies a static `thinCSIStorageClassName` lookup and `StoragePolicyName`
value, so either rename the test to reflect that it checks a StorageClass
invariant or update it to actually exercise a failure-domain/topology change
like the other tests in this file. If you keep the transition behavior, use the
existing kube client setup in `TestStorageClassSurvivesTopologyTransition` to
mutate FDs and then re-read the StorageClass after the transition; otherwise,
adjust the test name and assertions to match the current read-only behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+125
to
+129
| klog.V(2).Infof("Removing stale vCenterStoragePolicy entry for %s", host) | ||
| delete(c.vCenterStoragePolicy, host) | ||
| delete(c.backoffStates, host) | ||
| delete(c.pendingOrphans, host) | ||
| c.recorder.Eventf("StaleVCenterRemoved", "Removed stale vCenter entry for %s", host) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Avoid logging vCenter hostnames in logs/events.
These changed log/event messages include internal vCenter hostnames. Prefer a non-identifying message or a stable redacted identifier. As per coding guidelines, “Flag logging that may expose passwords, tokens, API keys, PII … session IDs, internal hostnames, or customer data.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/operator/storageclasscontroller/storageclasscontroller.go` around lines
125 - 129, The stale vCenter cleanup path in storageclasscontroller should not
emit internal hostnames in logs or events. Update the messages in the code path
that removes entries from c.vCenterStoragePolicy, c.backoffStates, and
c.pendingOrphans to use a non-identifying or redacted identifier instead of
host, and apply the same treatment to the StaleVCenterRemoved event so both klog
and c.recorder avoid exposing vCenter hostnames.
Source: Coding guidelines
Comment on lines
+266
to
+276
| if delErr := v.deleteStoragePolicy(ctx); delErr != nil { | ||
| klog.Warningf("Failed to delete orphaned storage policy %s on vCenter %s: %v", | ||
| v.policyName, v.vcenterApiConnection.Hostname, delErr) | ||
| } else { | ||
| v.policyCreated = false | ||
| if v.recorder != nil { | ||
| v.recorder.Eventf("StoragePolicyDeleted", "Deleted orphaned SPBM profile %s from vCenter %s", | ||
| v.policyName, v.vcenterApiConnection.Hostname) | ||
| } | ||
| } | ||
| return "", nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate SPBM deletion failures.
When deleteStoragePolicy fails, this still returns ("", nil), so the controller records an empty policy and won’t retry with error backoff. Return the existing policy name plus an error on delete failure.
Suggested fix
if delErr := v.deleteStoragePolicy(ctx); delErr != nil {
klog.Warningf("Failed to delete orphaned storage policy %s on vCenter %s: %v",
v.policyName, v.vcenterApiConnection.Hostname, delErr)
+ return v.policyName, fmt.Errorf("failed to delete orphaned storage policy %s: %v", v.policyName, delErr)
} else {
v.policyCreated = false
if v.recorder != nil {
v.recorder.Eventf("StoragePolicyDeleted", "Deleted orphaned SPBM profile %s from vCenter %s",
v.policyName, v.vcenterApiConnection.Hostname)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if delErr := v.deleteStoragePolicy(ctx); delErr != nil { | |
| klog.Warningf("Failed to delete orphaned storage policy %s on vCenter %s: %v", | |
| v.policyName, v.vcenterApiConnection.Hostname, delErr) | |
| } else { | |
| v.policyCreated = false | |
| if v.recorder != nil { | |
| v.recorder.Eventf("StoragePolicyDeleted", "Deleted orphaned SPBM profile %s from vCenter %s", | |
| v.policyName, v.vcenterApiConnection.Hostname) | |
| } | |
| } | |
| return "", nil | |
| if delErr := v.deleteStoragePolicy(ctx); delErr != nil { | |
| klog.Warningf("Failed to delete orphaned storage policy %s on vCenter %s: %v", | |
| v.policyName, v.vcenterApiConnection.Hostname, delErr) | |
| return v.policyName, fmt.Errorf("failed to delete orphaned storage policy %s: %v", v.policyName, delErr) | |
| } else { | |
| v.policyCreated = false | |
| if v.recorder != nil { | |
| v.recorder.Eventf("StoragePolicyDeleted", "Deleted orphaned SPBM profile %s from vCenter %s", | |
| v.policyName, v.vcenterApiConnection.Hostname) | |
| } | |
| } | |
| return "", nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/operator/storageclasscontroller/vmware.go` around lines 266 - 276, The
deletion path in the vmware storage policy cleanup flow is swallowing failures
by always returning an empty name and nil even when deleteStoragePolicy()
errors. Update the logic around deleteStoragePolicy and the return after the
warning so that a failed deletion returns the existing v.policyName together
with the deletion error, while keeping the success path unchanged. This will let
the controller surface the failure and retry with backoff instead of treating
the policy as successfully cleared.
Comment on lines
+211
to
+220
| // Create a PVC on the last failure domain's datastore | ||
| t.Log("Creating PVC to block orphan cleanup") | ||
| pvcName := fmt.Sprintf("pv-safety-test-%d", time.Now().Unix()) | ||
| ns := "openshift-cluster-csi-drivers" | ||
| pvc := newTestPVC(pvcName, ns, thinCSIStorageClassName) | ||
|
|
||
| _, err = kubeClient.CoreV1().PersistentVolumeClaims(ns).Create(ctx, pvc, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| t.Fatalf("Failed to create test PVC: %v", err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
What VolumeBindingMode does the vSphere CSI thin-csi StorageClass use in OpenShift, and how does topology-aware provisioning select a failure domain without a consuming pod?
💡 Result:
In OpenShift, the vSphere CSI thin-csi StorageClass uses the WaitForFirstConsumer volume binding mode [1][2][3][4][5]. Regarding your question on how topology-aware provisioning selects a failure domain without a consuming pod: it generally does not [6]. The WaitForFirstConsumer binding mode is specifically designed to delay the creation and binding of a Persistent Volume (PV) until a pod that requires the Persistent Volume Claim (PVC) is scheduled [6][7][8]. This mechanism allows the Kubernetes scheduler to use pod-specific scheduling constraints (such as node affinity or pod affinity) to inform the decision of which failure domain (e.g., zone or region) is most appropriate for provisioning the volume [6][7]. If the volume binding mode is set to Immediate (as opposed to WaitForFirstConsumer), volume provisioning occurs as soon as the PVC is created [6]. In such cases, if topology-aware provisioning is enabled but no pod constraints are available to guide the selection, the system may rely on default behaviors, such as Kubernetes randomly selecting a topology domain from the available options [9]. However, without the context of a pod's requirements, topology-aware provisioning cannot make an "intelligent" choice based on where a workload will actually run [7][8]. If the randomly selected topology domain lacks the necessary resources (such as worker nodes or datastore access), the volume provisioning might fail, or the pod (once created) may remain in a Pending state [9].
Citations:
- 1: https://docs.okd.io/4.17/storage/container_storage_interface/persistent-storage-csi-vsphere.html
- 2: https://docs.okd.io/4.16/storage/container_storage_interface/persistent-storage-csi-vsphere.html
- 3: https://docs.okd.io/4.14/storage/container_storage_interface/persistent-storage-csi-vsphere.html
- 4: https://docs.okd.io/4.12/storage/container_storage_interface/persistent-storage-csi-vsphere.html
- 5: https://docs.okd.io/4.13/storage/container_storage_interface/persistent-storage-csi-vsphere.html
- 6: https://techdocs.broadcom.com/us/en/vmware-cis/vsphere/vsphere/9-0/topology-aware-volume-provisioning.html
- 7: https://techdocs.broadcom.com/us/en/vmware-cis/vsphere/container-storage-plugin/2-0/getting-started-with-vmware-vsphere-container-storage-plug-in-2-0/using-vsphere-container-storage-plug-in/topology-aware-volume-provisioning/deploy-workloads-with-waitforfirstconsumer-mode-in-topology-aware-environment.html
- 8: https://techdocs.broadcom.com/us/en/vmware-cis/vsphere/container-storage-plugin/3-0/getting-started-with-vmware-vsphere-container-storage-plug-in-3-0/using-vsphere-container-storage-plug-in/topology-aware-volume-provisioning/deploy-workloads-with-waitforfirstconsumer-mode-in-topology-aware-environment/deploy-workloads-with-waitforfirstconsumer-mode-in-a-topology-aware-environment-for-block-volumes.html
- 9: https://techdocs.broadcom.com/us/en/vmware-cis/vsphere/container-storage-plugin/3-0/getting-started-with-vmware-vsphere-container-storage-plug-in-3-0/vsphere-container-storage-plug-in-deployment/deploying-vsphere-container-storage-plug-in-with-topology.html
🏁 Script executed:
sed -n '180,260p' test/e2e/failure_domain_lifecycle_test.goRepository: openshift/vmware-vsphere-csi-driver-operator
Length of output: 3037
🏁 Script executed:
sed -n '300,370p' test/e2e/failure_domain_lifecycle_test.goRepository: openshift/vmware-vsphere-csi-driver-operator
Length of output: 2801
Create a consumer Pod before waiting on PVC binding.
newTestPVC() is just a plain PVC, so with thin-csi’s WaitForFirstConsumer binding it won’t bind until something consumes it. That makes TestPVSafetyBlocksCleanup fall into the skip path instead of exercising orphan cleanup, and verifyPVCProvisionable has the same assumption. If the goal is a deterministic datastore, add a Pod and/or explicit topology constraints before waiting on the claim.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/failure_domain_lifecycle_test.go` around lines 211 - 220,
TestPVSafetyBlocksCleanup is creating a plain PVC via newTestPVC without any
consumer, so thin-csi WaitForFirstConsumer may never bind it and the test can
skip instead of exercising orphan cleanup. Update the setup to create a Pod that
mounts/uses the PVC, and if needed add explicit topology constraints so the
claim binds on the intended datastore before waiting. Apply the same assumption
fix in verifyPVCProvisionable so it also provisions deterministically.
jcpowermac
force-pushed
the
multi-vcenter-failure-domain-lifecycle
branch
from
88c3cdd to
6483f9d
Compare
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
jcpowermac
force-pushed
the
multi-vcenter-failure-domain-lifecycle
branch
from
6483f9d to
5f140f5
Compare
…e gate Vendor openshift/api@v0.0.0-20260701110937-6e21043ed12a which adds the VSphereMultiVCenterDay2 feature gate definition. This gate controls day-2 failure domain lifecycle operations (orphan tag cleanup, SPBM profile deletion, per-vCenter backoff) in the storage class controller. Updates both root and openshift-tests go.mod to the same openshift/api version and re-vendors the workspace. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jcpowermac
force-pushed
the
multi-vcenter-failure-domain-lifecycle
branch
from
5f140f5 to
b8cefb6
Compare
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Author
|
/test e2e-vsphere-operator-test |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/storageclasscontroller/storageclasscontroller_test.go`:
- Around line 276-285: Update the backoff expectations in
storageclasscontroller_test around the sync/backoff test cases so successful
reconciliations assert the 1-minute reset contract instead of a 10-minute delay.
Adjust the relevant cases in the table-driven tests for the controller’s sync
behavior so the success path and the fail-then-succeed sequence both use the
same reset interval, keeping the expectations aligned with the intended
lifecycle behavior.
In `@test/e2e/failure_domain_lifecycle_test.go`:
- Around line 230-232: The polling callbacks in the persistent volume claim
lifecycle tests are swallowing real API failures by returning false, nil after
kubeClient.CoreV1().PersistentVolumeClaims(...).Get errors. Update the affected
polling logic in the PVC checks to return the actual error from Get, and only
treat expected NotFound cases as retryable when appropriate; use the existing
polling closures around the PVC lookup code to preserve RBAC/API failures
instead of masking them as timeouts.
- Around line 100-101: The lifecycle e2e logs are exposing vSphere
topology/storage identifiers, so update the failure-domain removal logs to avoid
printing datacenter, datastore, failure-domain, or storage-policy names. In the
relevant test flow around the failure-domain lifecycle logging, replace
identifier-based messages with generic or count/index-based text unless a
specific value is strictly needed for debugging. Apply the same redaction
pattern to all similar log statements in this test so the output no longer
reveals internal infrastructure details.
- Around line 152-178: `TestStorageClassSurvivesTopologyTransition` currently
only reads the existing `thinCSIStorageClassName` and never drives any
failure-domain changes, so it does not test the intended topology lifecycle.
Update this test to actually exercise a 2→1→2→0 transition by using the
failure-domain/topology helpers already available in the test suite, then verify
the `StorageClass` behavior after each change instead of just logging
`StoragePolicyName`. Keep the check anchored around
`TestStorageClassSurvivesTopologyTransition` and the `thinCSIStorageClassName`
lookup so the test still validates the same storage class across the transition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e45ae33f-0f07-41a3-807e-4462e311333f
⛔ Files ignored due to path filters (225)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumopenshift-tests/go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.ci-operator.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Dockerfile.ocpis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apiserver/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiserver/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apps/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/authorization/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/build/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/cloudnetwork/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_operator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_crio_credential_provider_config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_dns.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_image.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha2/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha2/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/console/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/types_console_plugin.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/console/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/envtest-releases.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/types_pacemakercluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/types_pacemakercluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/helm/v1beta1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/helm/v1beta1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/image/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/image/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/image/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/imageregistry/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/imageregistry/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/kubecontrolplane/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/kubecontrolplane/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/legacyconfig/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/legacyconfig/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1beta1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/monitoring/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/monitoring/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/network/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/network/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/networkoperator/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/oauth/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/openshiftcontrolplane/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/openshiftcontrolplane/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_etcd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingresscontroller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_kubeapiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_openshiftapiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operatorcontrolplane/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operatorcontrolplane/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/osin/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/project/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/quota/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/quota/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/route/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/samples/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/samples/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/samples/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/security/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/security/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/security/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/servicecertsigner/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/servicecertsigner/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/sharedresource/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/sharedresource/v1alpha1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/template/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/template/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/template/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/user/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/user/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/user/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/k8s.io/kube-openapi/pkg/builder/openapi.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/builder3/openapi.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/serialization.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/README.mdis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/alias.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/arshal.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/arshal_any.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/arshal_default.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/arshal_funcs.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/arshal_inlined.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/arshal_methods.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/arshal_time.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/decode.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/encode.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/errors.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/fields.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/fold.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/intern.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/internal/jsonflags/flags.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/internal/jsonopts/options.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/internal/jsonwire/decode.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/internal/jsonwire/encode.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/internal/jsonwire/wire.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/jsontext/alias.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/jsontext/decode.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/jsontext/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/jsontext/encode.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/jsontext/errors.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/jsontext/export.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/jsontext/options.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/jsontext/pools.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/jsontext/quote.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/jsontext/state.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/jsontext/token.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/jsontext/value.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/migrate.shis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/options.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/internal/third_party/go-json-experiment/json/value.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/schemaconv/openapi.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/schemaconv/proto_models.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/spec3/encoding.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/spec3/example.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/spec3/external_documentation.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/spec3/header.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/spec3/media_type.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/spec3/operation.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/spec3/parameter.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/spec3/path.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/spec3/request_body.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/spec3/response.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/spec3/security_scheme.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/spec3/server.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/spec3/spec.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/validation/spec/header.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/validation/spec/info.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/validation/spec/items.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/validation/spec/operation.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/validation/spec/parameter.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/validation/spec/path_item.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/validation/spec/paths.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/validation/spec/response.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/validation/spec/responses.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/validation/spec/schema.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/validation/spec/security_scheme.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/validation/spec/swagger.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/kube-openapi/pkg/validation/spec/tag.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (10)
go.modopenshift-tests/go.modpkg/operator/storageclasscontroller/storageclasscontroller.gopkg/operator/storageclasscontroller/storageclasscontroller_test.gopkg/operator/storageclasscontroller/vmware.gopkg/operator/storageclasscontroller/vmware_test.gopkg/operator/utils/metric.gopkg/operator/vspherecontroller/driver_starter_test.gopkg/operator/vspherecontroller/vspherecontroller.gotest/e2e/failure_domain_lifecycle_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- go.mod
- pkg/operator/storageclasscontroller/vmware_test.go
- pkg/operator/utils/metric.go
- pkg/operator/vspherecontroller/vspherecontroller.go
- pkg/operator/vspherecontroller/driver_starter_test.go
- pkg/operator/storageclasscontroller/vmware.go
- pkg/operator/storageclasscontroller/storageclasscontroller.go
Comment on lines
+276
to
+285
| name: "success uses 10-minute interval", | ||
| syncSequence: []bool{true, true, true}, | ||
| // Success always uses successBackoff.Duration (10 min) | ||
| expectedDurations: []time.Duration{10 * time.Minute, 10 * time.Minute, 10 * time.Minute}, | ||
| }, | ||
| { | ||
| name: "fail 3x then succeed resets error backoff", | ||
| syncSequence: []bool{false, false, false, true, false}, | ||
| // fail: 1m, fail: 2m, fail: 4m, success: 10m, fail: 1m (error backoff reset by success) | ||
| expectedDurations: []time.Duration{time.Minute, 2 * time.Minute, 4 * time.Minute, 10 * time.Minute, time.Minute}, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align success-backoff assertions with the 1-minute reset contract.
These tests encode successful sync as a 10-minute delay, but the PR objective says success should reset to a 1-minute interval. As written, the tests will preserve the wrong lifecycle behavior.
Proposed test contract fix
- name: "success uses 10-minute interval",
+ name: "success uses 1-minute interval",
syncSequence: []bool{true, true, true},
- // Success always uses successBackoff.Duration (10 min)
- expectedDurations: []time.Duration{10 * time.Minute, 10 * time.Minute, 10 * time.Minute},
+ // Success resets to the normal 1-minute interval.
+ expectedDurations: []time.Duration{time.Minute, time.Minute, time.Minute},
@@
- // fail: 1m, fail: 2m, fail: 4m, success: 10m, fail: 1m (error backoff reset by success)
- expectedDurations: []time.Duration{time.Minute, 2 * time.Minute, 4 * time.Minute, 10 * time.Minute, time.Minute},
+ // fail: 1m, fail: 2m, fail: 4m, success: 1m, fail: 1m (error backoff reset by success)
+ expectedDurations: []time.Duration{time.Minute, 2 * time.Minute, 4 * time.Minute, time.Minute, time.Minute},
@@
- // vcenter-b should have success interval (10m)
+ // vcenter-b should have success interval (1m)
bDelay := bsB.nextCheck.Sub(beforeB)
- tolerance := float64(successCheckInterval) * 0.05
- if bDelay < successCheckInterval-time.Duration(tolerance) || bDelay > successCheckInterval+time.Duration(tolerance) {
- t.Errorf("vcenter-b: expected success interval ~%v, got %v", successCheckInterval, bDelay)
+ expectedSuccessDelay := time.Minute
+ tolerance := float64(expectedSuccessDelay) * 0.05
+ if bDelay < expectedSuccessDelay-time.Duration(tolerance) || bDelay > expectedSuccessDelay+time.Duration(tolerance) {
+ t.Errorf("vcenter-b: expected success interval ~%v, got %v", expectedSuccessDelay, bDelay)
}Also applies to: 383-387
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/operator/storageclasscontroller/storageclasscontroller_test.go` around
lines 276 - 285, Update the backoff expectations in storageclasscontroller_test
around the sync/backoff test cases so successful reconciliations assert the
1-minute reset contract instead of a 10-minute delay. Adjust the relevant cases
in the table-driven tests for the controller’s sync behavior so the success path
and the fail-then-succeed sequence both use the same reset interval, keeping the
expectations aligned with the intended lifecycle behavior.
Comment on lines
+100
to
+101
| t.Logf("Removing failure domain %q (datacenter=%s, datastore=%s)", | ||
| removedFD.Name, removedFD.Topology.Datacenter, removedFD.Topology.Datastore) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid logging vSphere topology and storage-policy identifiers.
Datacenter, datastore, failure-domain, and storage-policy names can expose customer/internal infrastructure details in e2e logs. Prefer count/index-based logs unless the identifier is required for debugging. As per coding guidelines, “Flag logging that may expose passwords, tokens, API keys, PII ..., internal hostnames, or customer data.”
Proposed log redaction
- t.Logf("Removing failure domain %q (datacenter=%s, datastore=%s)",
- removedFD.Name, removedFD.Topology.Datacenter, removedFD.Topology.Datastore)
+ t.Logf("Removing one failure domain from %d configured failure domains", len(fds))
@@
- t.Logf("StorageClass %s still exists with policy %s", sc.Name, sc.Parameters["StoragePolicyName"])
+ t.Logf("StorageClass %s still exists with a storage policy configured", sc.Name)
@@
- t.Logf("StorageClass thin-csi has StoragePolicyName=%s", policyName)
+ t.Log("StorageClass thin-csi has a StoragePolicyName configured")Also applies to: 142-142, 176-176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/failure_domain_lifecycle_test.go` around lines 100 - 101, The
lifecycle e2e logs are exposing vSphere topology/storage identifiers, so update
the failure-domain removal logs to avoid printing datacenter, datastore,
failure-domain, or storage-policy names. In the relevant test flow around the
failure-domain lifecycle logging, replace identifier-based messages with generic
or count/index-based text unless a specific value is strictly needed for
debugging. Apply the same redaction pattern to all similar log statements in
this test so the output no longer reveals internal infrastructure details.
Source: Coding guidelines
Comment on lines
+152
to
+178
| // TestStorageClassSurvivesTopologyTransition verifies that the thin-csi | ||
| // StorageClass survives transitions between topology-enabled and | ||
| // topology-disabled states when failure domains change. | ||
| func TestStorageClassSurvivesTopologyTransition(t *testing.T) { | ||
| kubeConfig, err := library.NewClientConfigForTest(t) | ||
| if err != nil { | ||
| t.Fatalf("Failed to get kubeconfig: %v", err) | ||
| } | ||
|
|
||
| kubeClient := kubernetes.NewForConfigOrDie(rest.AddUserAgent(kubeConfig, clientName)) | ||
| ctx := context.TODO() | ||
|
|
||
| sc, err := kubeClient.StorageV1().StorageClasses().Get(ctx, thinCSIStorageClassName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| t.Skip("StorageClass thin-csi not found, skipping topology transition test") | ||
| } | ||
| t.Fatalf("Failed to get StorageClass: %v", err) | ||
| } | ||
|
|
||
| policyName := sc.Parameters["StoragePolicyName"] | ||
| if policyName == "" { | ||
| t.Error("StorageClass thin-csi has empty StoragePolicyName") | ||
| } else { | ||
| t.Logf("StorageClass thin-csi has StoragePolicyName=%s", policyName) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make this test exercise an actual topology transition.
TestStorageClassSurvivesTopologyTransition only checks the current thin-csi StorageClass. It never changes failure domains, so it does not verify the PR’s stated 2→1→2→0 topology lifecycle behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/failure_domain_lifecycle_test.go` around lines 152 - 178,
`TestStorageClassSurvivesTopologyTransition` currently only reads the existing
`thinCSIStorageClassName` and never drives any failure-domain changes, so it
does not test the intended topology lifecycle. Update this test to actually
exercise a 2→1→2→0 transition by using the failure-domain/topology helpers
already available in the test suite, then verify the `StorageClass` behavior
after each change instead of just logging `StoragePolicyName`. Keep the check
anchored around `TestStorageClassSurvivesTopologyTransition` and the
`thinCSIStorageClassName` lookup so the test still validates the same storage
class across the transition.
Comment on lines
+230
to
+232
| p, err := kubeClient.CoreV1().PersistentVolumeClaims(ns).Get(pollCtx, pvcName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return false, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not swallow API errors inside polling callbacks.
Returning (false, nil) on a non-nil Get error masks RBAC/API failures as generic timeouts. Return the error, or only suppress expected NotFound cases. As per path instructions, “Never ignore error returns.”
Proposed polling error handling
p, err := kubeClient.CoreV1().PersistentVolumeClaims(ns).Get(pollCtx, pvcName, metav1.GetOptions{})
if err != nil {
- return false, nil
+ if apierrors.IsNotFound(err) {
+ return false, nil
+ }
+ return false, err
}
@@
co, err := configClient.ConfigV1().ClusterOperators().Get(pollCtx, storageCOName, metav1.GetOptions{})
if err != nil {
- return false, nil
+ return false, err
}
@@
p, err := kubeClient.CoreV1().PersistentVolumeClaims(ns).Get(pollCtx, pvcName, metav1.GetOptions{})
if err != nil {
- return false, nil
+ if apierrors.IsNotFound(err) {
+ return false, nil
+ }
+ return false, err
}
@@
co, err := configClient.ConfigV1().ClusterOperators().Get(pollCtx, storageCOName, metav1.GetOptions{})
if err != nil {
- return false, nil
+ return false, err
}
@@
co, err := configClient.ConfigV1().ClusterOperators().Get(pollCtx, storageCOName, metav1.GetOptions{})
if err != nil {
- return false, nil
+ return false, err
}Also applies to: 293-295, 327-329, 363-365, 385-387
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 232-232: error is not nil (line 230) but it returns nil
(nilerr)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/failure_domain_lifecycle_test.go` around lines 230 - 232, The
polling callbacks in the persistent volume claim lifecycle tests are swallowing
real API failures by returning false, nil after
kubeClient.CoreV1().PersistentVolumeClaims(...).Get errors. Update the affected
polling logic in the PVC checks to return the actual error from Get, and only
treat expected NotFound cases as retryable when appropriate; use the existing
polling closures around the PVC lookup code to preserve RBAC/API failures
instead of masking them as timeouts.
Sources: Path instructions, Linters/SAST tools
…Day2 Adds day-2 failure domain removal awareness to the storage class controller, gated behind the VSphereMultiVCenterDay2 TechPreview feature gate. When a failure domain is removed from the Infrastructure spec, the operator detects orphaned tags, safely detaches them (blocking on CNS PV presence), and deletes the SPBM profile when the vCenter has zero remaining failure domains. Key changes: - Gate all day-2 lifecycle paths (orphan detection, tag detach, SPBM deletion, stale host cleanup) on VSphereMultiVCenterDay2 feature gate - Per-vCenter exponential backoff with independent state tracking, success resets error backoff to default interval - Orphan tag detection compares tagged datastores against current failure domains using (datacenter, datastore) tuples to prevent cross-DC false matches - CNS PV safety: query CNS volumes per datastore before detaching tags, conservative fallback (skip detach) when CNS API is unavailable - Force-cleanup annotation on ClusterCSIDriver to bypass PV safety check - OrphanCleanupPending informational condition on operator status with pending orphan counts, cleared when resolved - Stale vCenter cleanup iterates union of all per-vCenter maps to prevent leaked state - Early-return cache bypassed when forceCleanup is set or pendingOrphans exist, preventing stale condition state - findOrphanedTags errors block SPBM deletion to prevent unsafe cleanup on transient API failures - SPBM delete failure retains known policy name instead of returning empty string - Prometheus metrics for tag operations and orphan detection - E2e tests for FD removal tag cleanup, PV safety blocking, topology transition, and operator health verification - Unit tests for per-vCenter backoff, gate-off behavior, pending orphan resync bypass, and stale map cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jcpowermac
force-pushed
the
multi-vcenter-failure-domain-lifecycle
branch
from
b8cefb6 to
acb68c3
Compare
Author
|
/test e2e-vsphere-operator-test |
Author
|
/test e2e-vsphere-operator-test |
Contributor
|
@jcpowermac: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…topology tests
Phase 1: Fix StorageClassController backoff — reset to 1-minute interval on success instead of permanently staying at the 30-minute cap after exponential backoff.
Phase 3A: Orphan tag cleanup — detect datastores tagged with the cluster tag that are no longer referenced by any failure domain, detach orphan tags with PV safety check (skip detach when bound PVs exist), add tag operation and orphan detection Prometheus metrics, integrate cleanup into createStoragePolicy() to handle zonal-to-non-zonal transitions (2+ FDs to 0 FDs). Support force-cleanup via Infrastructure annotation.
Add topology mode transition verification tests confirming the hook correctly handles FD count transitions (2->1->2->0).
Summary by CodeRabbit