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
apiserver: use the correct error when logging errors updating managedFields #113711
apiserver: use the correct error when logging errors updating managedFields #113711
Conversation
@@ -202,8 +202,8 @@ func (f *FieldManager) UpdateNoErrors(liveObj, newObj runtime.Object, manager st | |||
if err != nil { | |||
atMostEverySecond.Do(func() { | |||
ns, name := "unknown", "unknown" | |||
accessor, err := meta.Accessor(newObj) | |||
if err == nil { | |||
accessor, accessorErr := meta.Accessor(newObj) |
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.
Otherwise error log on line 211 uses a nil err
/assign @apelisse |
/lgtm
/retest |
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.
/assign @lavalamp
Also generally curious if we should be concerned that this is logging
accessor, accessorErr := meta.Accessor(newObj) | ||
if accessorErr == nil { | ||
ns = accessor.GetNamespace() | ||
name = accessor.GetName() | ||
} |
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.
Wouldn't it be cuter to do:
accessor, accessorErr := meta.Accessor(newObj) | |
if accessorErr == nil { | |
ns = accessor.GetNamespace() | |
name = accessor.GetName() | |
} | |
if accessor, err := meta.Accessor(newObj); err == nil { | |
ns = accessor.GetNamespace() | |
name = accessor.GetName() | |
} |
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.
+1
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.
Updated, thanks
I guess we'll only be able to tell once we see the error :-) |
/triage accepted |
…Fields Signed-off-by: Andrew Sy Kim <andrewsy@google.com>
fb0e4ce
to
efdd067
Compare
/lgtm |
1 similar comment
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim, apelisse, lavalamp 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: Andrew Sy Kim andrewsy@google.com
What type of PR is this?
/kind bug
What this PR does / why we need it:
I noticed that
[SHOULD NOT HAPPEN] failed to update managedFields
was being logged but there was no error logged with it. After looking at the code, it's because the err passed intoklog.ErrorS
is the nil error frommeta.Accessor(newObj)
when it should be the err from trying to updatemanagedFields
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: