-
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
🌱 Log reason for MachineDeployment rollouts / MachineSet creations #10688
🌱 Log reason for MachineDeployment rollouts / MachineSet creations #10688
Conversation
Signed-off-by: Stefan Büringer [email protected]
/assign @fabriziopandini @chrischdi @vincepri |
return false | ||
|
||
// If RolloutAfter is not set, pick the first matching MachineSet. | ||
if deployment.Spec.RolloutAfter == nil { |
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.
to clarify, for this three cases (451, 456, 464) we could return early within the if equal { check if we wanted?
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.
Yup
if ms == nil { | ||
return false | ||
if len(matchingMachineSets) == 0 { | ||
return nil, fmt.Sprintf("couldn't find MachineSet matching MachineDeployment spec template: %s", strings.Join(diffs, ",")), nil |
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.
why did you choose to pass this around instead of logging right here?
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.
Because this func is used in multiple places. I wanted to make sure we log this decision/reason only at the point where we actually create a new MachineSet
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.
Considering this one: #10688 (review)
Logging it only when we create a new MachineSet means that we can do it on log level 0.
If we would log it every time we run through this code we have to move it somewhere towards trace level, because it would be very very verbose otherwise. But of course on log levels >=3 it's a lot less useful because nobody will see it per default.
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 see, the reasoning and outcome makes sense to me. The implementation seems to me like we have some debt tangling in these functions. May be have this be an error type that can be checked, that wouldn't help much though...
Anyways we can always follow up and iterate to keep untangling. Thanks!
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.
Agree overall. I think an error might be not the best fit, because it's not really an error, it's the reasoning why we made a certain decision.
/test pull-cluster-api-e2e-conformance-ci-latest-main |
/test pull-cluster-api-e2e-main |
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.
/lgtm
The only concern is that in case of diffs we log the difference between current and all the existing MS, which can become verbose.
But considering we keep revision history to 1 by default, the trade-off between improved observability and verbosity makes sense to me.
LGTM label has been added. Git tree hash: 44bc72a6743fb71125f06e7d338051f876f44c31
|
Yup that's a good point. I was considering only logging "n" MachineSets with the highest revisions. But then I thought:
|
/lgtm |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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 |
Signed-off-by: Stefan Büringer [email protected]
What this PR does / why we need it:
Follow-up of this PR #10628 for Machinedeployments
Initial rollout:
Rollout because existing MachineSets don't match
Message properly formatted: (only \n replaced with new line)
Rollout because of RolloutAfter
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Related to #8186