New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dra: refactoring overall flow of prepare/unprepare resources #120099
Conversation
/area kubelet |
/retest |
First draft of refactoring the overall code of the dra plugin client. Implemented detecting version in lazy way (on first call). I also removed having two types While waiting for your feedback guys, I'm willing to look at e2e node tests and e2e tests with kind and to potentially add new tests and try to cover as many cases as possible. I wonder if you prefer separate PRs or updating this one tho . |
@TommyStarK if new e2e tests depend on these changes, it's better to add them to this PR. If not - a separate PR would be better. |
/triage accepted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass. Didn't thoroughly review the tests yet.
c313976
to
0053f2a
Compare
/retest-required |
/lgtm |
LGTM label has been added. Git tree hash: 9404dcb012cf034d6df77abe4dd25c346bc4c898
|
@klueska friendly ping |
pkg/kubelet/cm/dra/plugin/client.go
Outdated
p.Lock() | ||
p.version = v1alpha2Version | ||
p.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add SetVersion()
and GetVersion()
calls to avoid needing to make the lock calls here (and in similar places) directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing, done in 55e3662. I made setVersion
to accept a parameter just in case of supporting future api versions.
Signed-off-by: TommyStarK <thomasmilox@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience and your quick turnaround after the reviews.
/approve
/lgtm
LGTM label has been added. Git tree hash: 6d91a41b055d201791370176bc46626690c1262c
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: klueska, TommyStarK The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
If applied this commit aims to refactor the overall flow of the node client when it has to prepare/unprepare resources.
Which issue(s) this PR fixes:
part of #119469
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: