-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
✨ Set Kubernetes version in machinepool machine Status.Version #10849
✨ Set Kubernetes version in machinepool machine Status.Version #10849
Conversation
Skipping CI for Draft Pull Request. |
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.
Just a few minor findings
39a1377
to
857318a
Compare
/test pull-cluster-api-e2e-main |
// Get the nodeRefsMap from the cluster. | ||
s.nodeRefMap, err = r.getNodeRefMap(ctx, clusterClient) | ||
if err != nil { | ||
r.recorder.Event(mp, corev1.EventTypeWarning, "FailedGetNodeRefMap", err.Error()) |
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.
I would prefer to return an error here and not send an event. Usually we're bubbling up errors and not emitting events when we hit them.
I think this could produce a lot of events when a workload cluster is not reachable
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.
gotcha -- right right i think i took this approach because if we return an error at this point, then we stop short of reconcileMachines
which would a be change in behavior (previously reconcileMachines
was called without any node references as we didn't set the status).
can make the update to return at this point if you think that's best? or maybe emit a Warn
log indicating status won't be set on the machines and let execution continue?
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.
Maybe let's store this error here e.g. in getNodeRefsErr
and then check in l.300+ if either this error is !=nil or the one returned from reconcileMachines
and then return if at least one of the errors is != nil + aggregate them.
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.
nice yes sounds good -- just pushed up this change
Reviewed everything apart from "machinepool_controller_phases_test.go" (will do that on the next review) |
857318a
to
7fc92cf
Compare
7fc92cf
to
12d8855
Compare
i also found this open issue: #8856 since this PR is already touching this code i could also tie in this improvement if it would make sense? i think since the |
Let's do this in a separate PR. Mostly because this PR is ready for merge /lgtm |
LGTM label has been added. Git tree hash: c2e459acde1c02a617fc24543e6dd61655ec7859
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
/test pull-cluster-api-e2e-main |
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 #9017
/area machinepool