Skip to content

add finaliser to GPUCluster CR using PATCH#2676

Open
tariq1890 wants to merge 1 commit into
mainfrom
ensure-finaliser
Open

add finaliser to GPUCluster CR using PATCH#2676
tariq1890 wants to merge 1 commit into
mainfrom
ensure-finaliser

Conversation

@tariq1890

Copy link
Copy Markdown
Contributor

This PR adds a new util method to add a finaliser to the GPUCluster custom resource. It also uses PATCH to add the finaliser as it's safer and more robuse to race conditions around object updates.

h/t to @rajathagasthya for pointing me to the method in CAPI that inspired this change

@coveralls

coveralls commented Jul 24, 2026

Copy link
Copy Markdown

Coverage Status

coverage: 33.505% (-0.04%) from 33.544% — ensure-finaliser into main

Comment thread internal/utils/utils.go Outdated
@tariq1890

Copy link
Copy Markdown
Contributor Author

/ok to test a4248cf

@tariq1890 tariq1890 self-assigned this Jul 25, 2026
@tariq1890

Copy link
Copy Markdown
Contributor Author

Thanks for the review @rahulait. I updated the PR to address a review comment from Claude and also added unit tests

 Missing optimistic lock (internal/utils/utils.go:179): client.MergeFrom generates a patch with no resourceVersion, so the API server applies it without conflict detection. Two concurrent reconcilers can silently clobber each other's finalizers. The old r.Update included resourceVersion and would have 409'd. Fix: replace client.MergeFrom(original) with client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{}).

Signed-off-by: Tariq Ibrahim <tibrahim@nvidia.com>
Co-authored-by: Rajath Agasthya <ragasthya@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants