Skip to content

add finaliser to GPUCluster CR using PATCH - #2676

Merged
tariq1890 merged 1 commit into
mainfrom
ensure-finaliser
Jul 27, 2026
Merged

tariq1890 merged 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>

@rajathagasthya rajathagasthya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

@tariq1890
tariq1890 merged commit 3fad672 into main Jul 27, 2026
36 of 39 checks passed
@tariq1890
tariq1890 deleted the ensure-finaliser branch July 27, 2026 05:37
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.

4 participants