🐛 e2e: Serialize default KMS provider across concurrent jobs - #1797
Open
aakashchan wants to merge 7 commits into
Open
🐛 e2e: Serialize default KMS provider across concurrent jobs#1797aakashchan wants to merge 7 commits into
aakashchan wants to merge 7 commits into
Conversation
The four Get-or-Create*StoragePolicy helpers in test/e2e/infrastructure/vsphere/vcenter/storage.go each check pbmClient.ProfileIDByName before calling CreateProfile, but had no fallback if CreateProfile lost a create race. Two e2e shards (e.g. Core and Experimental) sharing one testbed can both reach SynchronizedBeforeSuite around the same time and race to create the same fixed-name PBM storage profile (e.g. "worker-storagepolicy"); the loser got a PbmDuplicateName SOAP fault and failed its entire suite with 0 specs run. Confirmed via UTS runs 1115673, 1108771, and 1108764, where two sibling shards created their content libraries 60ms apart against the same vCenter.
isPbmDuplicateNameFault manually unwrapped the SOAP fault via soap.IsSoapFault/soap.ToSoapFault(...).VimFault(), which duplicates logic govmomi already provides. Use fault.As against types.PbmDuplicateName and types.PbmDuplicateNameFault instead, matching the existing pattern in pkg/providers/vsphere/vcenter/getvm.go. Addresses review comment on PR vmware-tanzu#1788.
vCenter's default KMS provider (CryptoManagerKmip.SetDefaultKmsCluster / GetDefaultKmsCluster called with entity=nil) is a single, VC-wide setting, not scoped per-namespace or per-test. The vm_encryption.go specs save and restore it in BeforeEach/AfterEach as if each test owned it exclusively. When two E2E jobs run encryption specs concurrently against the same vCenter appliance, one job's cleanup can clear or overwrite the default the other job just set and is relying on, producing a spurious VirtualMachineEncryptionSynced=NoDefaultKeyProvider failure. Add a distributed lock backed by a Lease in the target supervisor cluster's vmware-system-vmop namespace and hold it for the full read-mutate-restore window (BeforeEach's read through AfterEach's restore, via DeferCleanup) so only one job at a time can mutate the default KMS provider for a given vCenter. Changes: - Add AcquireDefaultKMSProviderLock in test/e2e/vmservice/utils/kmslock.go, an optimistic-concurrency create-or-take-over Lease lock with a bounded TTL so a crashed holder cannot permanently block other jobs. - Wire the lock into vm_encryption.go's BeforeEach, released after AfterEach restores the original default provider. - Register coordination/v1 in the shared e2e scheme (vmservice/common/scheme.go) so controller-runtime clients can read/write Lease objects; this was previously missing and caused the lock to fail with "no kind is registered for the type v1.Lease".
aakashchan
marked this pull request as ready for review
August 5, 2026 11:48
aakashchan
changed the title
test/e2e/infrastructure/vsphere/vcenter/storage.go's PBM duplicate-profile-name handling is unrelated to the default-KMS-provider lock work in this PR; drop it back to main so the PR only contains the KMS-lock fix.
Keep only the non-obvious rationale (VC-wide setting, why the lock lives in the supervisor cluster instead of vCenter) and drop restatements of what the code already says, plus the ticket reference so the comment stands on its own.
faisalabujabal
added
the
pro-ai-review-requested
faisalabujabal
approved these changes
Aug 5, 2026
faisalabujabal
left a comment
faisalabujabal
left a comment
Contributor
There was a problem hiding this comment.
post a message on the channel as we discussed that we are going with this solution. lets give the team a chance to object if anyone wants to
|
📝 CL Summary: This PR introduces a distributed lock backed by a Kubernetes |
faisalabujabal
removed
the
pro-ai-review-requested
bryanv
reviewed
Aug 7, 2026
Derives the Lease's lease-duration from the Ginkgo suite's own --timeout (the same value hack/e2e/run-e2e.sh sets via GINKGO_TIMEOUT) minus a buffer, instead of a separately maintained 15m constant, per faisalabujabal's review comment to source it from the suite timeout. Also preconditions releaseKMSLease's Delete on the Lease's UID and resourceVersion captured at Get time, so a take-over landing between the Get and Delete fails the delete with a conflict instead of deleting a Lease this process no longer holds, per bryanv's review comment.
The Lease's LeaseDurationSeconds was derived from the Ginkgo suite's --timeout minus a buffer, so a holder could legitimately keep it for up to that whole window (e.g. ~115m with --ginkgo.timeout=2h) while waiters in AcquireDefaultKMSProviderLock only ever waited a fixed 10*time.Minute. Any time a holder's real BeforeEach+It+AfterEach work ran long - observed swinging 4.6m to 7.5m per spec on live testbeds, and up to ~12m just for namespace realization under load - a waiter could exhaust its 10m budget and fail with "timed out waiting to acquire the default KMS provider lock", even though the holder was still alive and correctly serialized (UTS builds 1179655/1179879). Shrinking the fixed lease duration instead of deriving it from the suite timeout doesn't fix this: a static TTL either has to bound worst-case runtime (which the observed variance shows is unsafe - a holder judged "expired" while still alive causes two jobs to mutate vCenter's global default KMS provider concurrently, the exact bug this lock exists to prevent) or stays large enough that waiters keep timing out under real contention. Replace the fixed/derived TTL with a renewal loop: the lease duration is now a short, fixed window (90s) that only bounds how fast a dead holder (crash, panic) is reclaimed, and a background goroutine renews RenewTime every 30s for as long as the holder is alive via DeferCleanup, decoupling "how long the work takes" from "how long the lock may be held". Waiters' timeout is bumped from 10m to 25m and re-scoped to mean "how long to wait for queued jobs ahead of us", sized off the several-minute-per-spec hold times actually observed, since it's no longer coupled to lease TTL semantics at all. Changes: - kmslock.go: replace defaultKMSProviderLockLeaseDuration() (derived from GinkgoConfiguration().Timeout) with a fixed 90s constant, add renewKMSLeaseUntilStopped/renewKMSLease, and start/stop the renewal goroutine around the existing acquire/release in AcquireDefaultKMSProviderLock. - vm_encryption.go: bump the lock-acquire wait timeout from 10m to 25m and update the comment to reflect the new, decoupled meaning. Test changes: - No new test file; test/e2e/vmservice/utils has no unit-test convention today - this lock is exercised live by the vm_encryption.go specs it serializes, same as before this change.
Minimum allowed line rate is |
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
4 participants
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.
What does this PR do, and why is it needed?
vCenter's default KMS provider (
CryptoManagerKmip.SetDefaultKmsCluster/
GetDefaultKmsClustercalled withentity=nil) is a single, VC-widesetting — it is not scoped per-namespace, per-session, or per-test. The
vm_encryption.goE2E specs save and restore this value inBeforeEach/AfterEachas if each test owned it exclusively. When twoE2E jobs run encryption specs concurrently against the same vCenter
appliance, one job's cleanup can clear or overwrite the default the
other job just set and is relying on, producing a spurious
VirtualMachineEncryptionSynced=NoDefaultKeyProviderfailure andtimeout partway through VM creation.
This was root-caused from two UTS builds (1132205 and 1132208) that
ran encryption specs concurrently against the same shared vCenter: the
first build's
AfterEachcleared the global default KMS provider 3seconds before the second build's vTPM spec tried to create its VM,
which then failed because vm-operator correctly saw no default
provider configured.
This PR adds a distributed lock backed by a Kubernetes
Leasein thetarget supervisor cluster's
vmware-system-vmopnamespace, held forthe full read-mutate-restore window (from
BeforeEach's read of thecurrent default through
AfterEach's restore, viaDeferCleanup), soonly one job at a time can mutate the default KMS provider for a given
vCenter appliance.
Changes:
AcquireDefaultKMSProviderLockintest/e2e/vmservice/utils/kmslock.go: an optimistic-concurrencycreate-or-take-over
Leaselock with a bounded TTL so a crashedholder cannot permanently block other jobs.
vm_encryption.go'sBeforeEach, released afterAfterEachrestores the original default provider.coordination/v1in the shared e2e scheme(
vmservice/common/scheme.go) so controller-runtime clients canread/write
Leaseobjects — this was previously missing and causedthe lock to fail with
no kind is registered for the type v1.Lease.Which issue(s) is/are addressed by this PR? (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Are there any special notes for your reviewer:
UTS precheckin succeeded for all environment. Ran the encryption tests.
This is a test-infrastructure-only change (
test/e2e/**); no product(
pkg/,api/) code is touched. Verified withgo build ./...,go vet, andgofmton the e2e module; also validated live against areal WCP testbed running the
vm_encryption.gospecs (surfaced andfixed the missing
coordination/v1scheme registration in theprocess).
Please add a release note if necessary: