-
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
🐛 Update Machine Status with Error Reason and Error Message if infra object is deleted #1458
🐛 Update Machine Status with Error Reason and Error Message if infra object is deleted #1458
Conversation
Hi @kashifest. Thanks for your PR. I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign @ncdc |
@@ -78,6 +78,13 @@ func (r *MachineReconciler) reconcileExternal(ctx context.Context, m *clusterv1. | |||
obj, err := external.Get(r.Client, ref, m.Namespace) | |||
if err != nil { | |||
if apierrors.IsNotFound(err) { | |||
if m.Status.InfrastructureReady && m.Status.GetTypedPhase() == clusterv1.MachinePhaseRunning { |
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 shouldn't have logic comparing against the Machine "Phase", it is meant to only be a user friendly display value.
This check should also probably be pushed up to where reconcileExternal is called for the InfrastructureRef, since a missing Bootstrap.ConfigRef should not result in an 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 checked the machine phase to make sure this block of code is executed only once. If it is not recommended, I can remove that
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.
Once we get it sorted where this logic should live, could you please make sure to add test case(s) for it? We have e.g. TestReconcileInfrastructure
and TestReconcileBootstrap
.
@@ -78,6 +78,13 @@ func (r *MachineReconciler) reconcileExternal(ctx context.Context, m *clusterv1. | |||
obj, err := external.Get(r.Client, ref, m.Namespace) | |||
if err != nil { | |||
if apierrors.IsNotFound(err) { | |||
if m.Status.InfrastructureReady && m.Status.GetTypedPhase() == clusterv1.MachinePhaseRunning { | |||
// Infra object went missing after the machine was up and running | |||
klog.Infof("Infrastructure object %v %q went missing. Updating Machine Status", ref.GroupVersionKind(), ref.Name) |
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 don't want to add any new raw klog
calls. Please use r.Log
instead.
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.
ok
@@ -78,6 +78,13 @@ func (r *MachineReconciler) reconcileExternal(ctx context.Context, m *clusterv1. | |||
obj, err := external.Get(r.Client, ref, m.Namespace) | |||
if err != nil { | |||
if apierrors.IsNotFound(err) { | |||
if m.Status.InfrastructureReady && m.Status.GetTypedPhase() == clusterv1.MachinePhaseRunning { |
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 should not check the phase - that is for human consumption. Checking InfrastructureReady is sufficient.
If we place this logic here, it applies to all external references - both the bootstrap ref and the infrastructure ref. @vincepri do you this is appropriate?
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 checked the machine phase to make sure this block of code is executed only once. If it is not recommended, I can remove that
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.
If we are not currently short-circuiting machine reconciliation if the error reason/error message are set, we need to be doing that.
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.
@ncdc I am not sure if I understood correctly, this check was allowing to setting the error reason/message and logging only once, but it did not short-circuit reconciliation
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.
In what situation would it be executing this more than once?
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.
In each call to reconcileExternal in my current version of the code?
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.
Ok, once you move this to reconcileInfrastructure, I think we also need to add code toward the top of Reconcile that checks machine.status.errorReason/errorMessage, and if either is set, it stops. This would need to be before the call to patch.NewHelper. Do you agree @vincepri?
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.
Yes, the short-circuit behavior is definitely missing. Let's follow-up with a different PR marked with
if m.Status.InfrastructureReady && m.Status.GetTypedPhase() == clusterv1.MachinePhaseRunning { | ||
// Infra object went missing after the machine was up and running | ||
klog.Infof("Infrastructure object %v %q went missing. Updating Machine Status", ref.GroupVersionKind(), ref.Name) | ||
machineStatusError := capierrors.MachineStatusError("Invalid Configuration") |
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.
@vincepri do we have any guidance on the content and format for error reasons?
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 should avoid creating new ErrorReason(s) dynamically. We can add new ones to the constants defined in the package. Let's focus on having a good error message in ErrorMessage
though
klog.Infof("Infrastructure object %v %q went missing. Updating Machine Status", ref.GroupVersionKind(), ref.Name) | ||
machineStatusError := capierrors.MachineStatusError("Invalid Configuration") | ||
m.Status.ErrorReason = &machineStatusError | ||
m.Status.ErrorMessage = pointer.StringPtr("Invalid Configuration") |
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.
This should be descriptive and include the GVK (GroupVersionKind) and name of the resource that went missing.
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.
ok.
@ncdc I can add the test cases also. Now I am waiting to know where should I put the check in |
+1 to |
Before merging, let's find a PR title that communicates better the changes we're making. PR titles go in release notes for users to read |
/ok-to-test |
29f3724
to
166cc55
Compare
b779cfb
to
0a59225
Compare
@@ -197,6 +197,14 @@ func (r *MachineReconciler) reconcileInfrastructure(ctx context.Context, m *clus | |||
if infraConfig == nil && err == nil { | |||
return nil | |||
} else if err != nil { | |||
if m.Status.InfrastructureReady { |
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.
This should probably also test apierrors.IsNotFound(errors.Cause(err))
, to ensure that the error is related to the resource being missing rather than another type of 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 tried that. It seems reconcileExternal is consuming that error and only returning the requeue 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.
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 could either move the requeue error up to the calling functions, or (less preferred, but quicker) string match "not found" on the incoming 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.
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'm good with this as a short term solution as long as a tracking issue is created to clean up later.
0a59225
to
160b664
Compare
// Infra object went missing after the machine was up and running | ||
r.Log.Error(err, "Machine infrastructure reference has been deleted after being ready, setting failure state") | ||
machineStatusError := capierrors.InvalidConfigurationMachineError | ||
m.Status.ErrorReason = &machineStatusError |
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.
m.Status.ErrorReason = &machineStatusError | |
m.Status.ErrorReason = capierrors.ClusterStatusErrorPtr(capierrors.InvalidConfigurationMachineError) |
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.
@vincepri Thanks. Didn't know this exist. Shouldn't it be MachineStatusErrorPtr instead of ClusterStatusErrorPtr
160b664
to
c62129c
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.
/approve
/assign @detiber
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kashifest, 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 |
/lgtm |
What this PR does / why we need it:
Updates the machine status with error reason and error message when machine is in running phase and the infrastructure object is deleted
Which issue(s) this PR fixes (optional, in fixes #(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1398