-
Notifications
You must be signed in to change notification settings - Fork 1.4k
✨ Add in-place update hooks to API #12343
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
base: main
Are you sure you want to change the base?
✨ Add in-place update hooks to API #12343
Conversation
[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 |
a180b63
to
9310ebd
Compare
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 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).
// until it reports Done or Failed status. | ||
func UpdateMachine(*UpdateMachineRequest, *UpdateMachineResponse) {} | ||
|
||
func init() { |
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.
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
9310ebd
to
d9e88e3
Compare
@fabriziopandini Thank you for the great feedback, all your points are valid. We need to start somewhere with the implementation, and my intention is to split it into smaller PRs to make the review process much easier. The in-place update feature will be hidden behind a feature gate and won’t be announced as alpha until we all agree on it. I’ll start implementing a reference updater as soon as possible, and at the same time, we’re going to begin building an experimental updater in Rancher to battle-test the idea. Hopefully, this will help clarify some of the open questions as we go. Regarding the book, I completely agree that we need to create a dedicated page. I can open an issue to track it, but I’d prefer to wait a bit until we have some functional code to document. |
Signed-off-by: Alexandr Demicev <[email protected]>
d9e88e3
to
526da98
Compare
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