✨ Add in-place update hooks to API #12343
Conversation
k8s-ci-robot
added
area/runtime-sdk
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/L
Signed-off-by: Alexandr Demicev <alexandr.demicev@suse.com>
alexander-demicev
force-pushed
the
inplacehooks
branch
from
a180b63 to
9310ebd
Compare
There was a problem hiding this comment.
Thanks for this PR
TBH, It is a little bit complex to think about all the implications without a deeper understanding of when/how those hooks are being called. e.g.
- is []string the best way to represent changes (e.g. how this will work when there changes to items in an array)
- do we need more than []string? e.g. reference to KCP/MD, corresponding templates, may be list or reference to controlled machines impacted by the change
- do we need something to link a set of proposed/accepted changes to in-place updates of the corresponding machines? (e.g. what will happen if the spec changes again in the meantime)
Considering I don't have a good answer to those questions yet, my gut feeling is that we should first look into where/how hooks are going to be called and then use learning to finalize & implement hooks, also because without the first part, hooks are not usable.
But no strong opinions, I'm also ok in merging a first release and then iterate, but this will probably require us to make exceptions e.g. if breaking changes will be required (probably not a blocker in this case, but I just want to bring this up).
| // machineRef is a reference to the machine object the in-place update hook corresponds to. | ||
| // Updaters should fetch the latest machine state using this reference. | ||
| // +required | ||
| MachineRef corev1.ObjectReference `json:"machineRef"` |
There was a problem hiding this comment.
Let's please not use corev1.ObjectReference, because it has many fields we do not care about.
I would suggest to create an new type ObjectReference with only the fields we care about
|
|
||
| // updateStatus indicates the current status of the update operation. | ||
| // +required | ||
| UpdateStatus UpdateMachineStatus `json:"updateStatus"` |
There was a problem hiding this comment.
q: do we need this or can we infer the same info from CommonRetryResponse:
- status=Success + retryAfterSeconds > 0 -> in progress
- status=Success + retryAfterSeconds = 0 -> completed
- status=Failure -> failed
| // until it reports Done or Failed status. | ||
| func UpdateMachine(*UpdateMachineRequest, *UpdateMachineResponse) {} | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
We probably need to start working to a book page with this documentation, but considering there are still details to be figured out, I'm also ok to defer
What this PR does / why we need it:
This PR introduces the runtime hooks infrastructure for in-place updates as defined in the In-place updates proposal
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
/area runtime-sdk