Replace HandleCrash and HandleError calls to use context-aware alternatives #134182
Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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-sigs/prow repository. |
|
Hi @mayank-agrwl. Thanks for your PR. I'm waiting for a 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-sigs/prow repository. |
| if err != nil { | ||
| defer utilruntime.HandleError(err) | ||
|
|
||
| defer utilruntime.HandleErrorWithContext(ctx, err, "Error creating pod for job") |
There was a problem hiding this comment.
Removed this as we are already checking for err != nil in the if block above for the same err
|
/cc @pohly |
|
/ok-to-test |
pkg/controller/job/job_controller.go
Outdated
| jobKey, err := controller.KeyFunc(job) | ||
| if err != nil { | ||
| utilruntime.HandleError(fmt.Errorf("Couldn't get key for job %#v: %v", job, err)) | ||
| utilruntime.HandleErrorWithContext(ctx, err, "Couldn't get key for job", "job", job) |
There was a problem hiding this comment.
What is the effect of using utilruntime.HandleErrorWithContext with key-value pair "job", job? Is it logging just the namespace / name, or entire object?
I'm probably ok with both, but it seems a bit inconsistent currently, seeing:
- https://github.com/kubernetes/kubernetes/pull/134182/files#diff-a7577a7dede7864ff38c631319033714fdd0bed91108d976282e1099507e6ff0R609
- https://github.com/kubernetes/kubernetes/pull/134182/files#diff-a7577a7dede7864ff38c631319033714fdd0bed91108d976282e1099507e6ff0R566
I'm leaning to log the namespace / name using klog.KObj(job) in all cases. Only that matters for controller.KeyFunc(job). For debugging the state of the entire Job admins or on-callers should rather use audit logs.
There was a problem hiding this comment.
@mimowo Thanks for your review. I didn't want to change functional behavior by much, so I essentially logged what I saw. For example, here I saw the job object and in some places I saw namespace/name combination using string formatting. I support the idea of using klog.KObj(job). Based on what @pohly says, I'll change it to be consistent across all changes in this PR.
There was a problem hiding this comment.
I also prefer klog.KObj. Dumping entire objects can get large and risks leaking sensitive information. This should be reserved to higher debug levels.
There was a problem hiding this comment.
Following up on this, for cases like:
https://github.com/kubernetes/kubernetes/pull/134182/files#diff-a7577a7dede7864ff38c631319033714fdd0bed91108d976282e1099507e6ff0R609
We don't really have an option I think? The type of object is interface{}, so it can be anything and won't satisfy the interface for kObj.
There was a problem hiding this comment.
That's a pity, but you are right. This should be dead code anyway (KeyFunc shouldn't fail), so let's not worry further and keep it equivalent to what was done before (dump struct).
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mayank-agrwl 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 |
1e7d40d to
b360361
Compare
|
@pohly Based on your feedback, moving all changes here. |
We can slice the work by function (focusing on one aspect of the problem) or by component (focusing on one reviewer/approver), but slicing it by function and component goes a bit too far. Doing several components together in batches is a good middle ground.
Thanks! Let's use this PR as an example and then continue. |
b360361 to
f5c5f6a
Compare
| // GetServicesToUpdateOnPodChange returns a set of Service keys for Services | ||
| // that have potentially been affected by a change to this pod. | ||
| func GetServicesToUpdateOnPodChange(serviceLister v1listers.ServiceLister, old, cur interface{}) sets.String { | ||
| func GetServicesToUpdateOnPodChange(logger klog.Logger, serviceLister v1listers.ServiceLister, old, cur interface{}) sets.Set[string] { |
There was a problem hiding this comment.
Modified this as I changed the signature to accept logger and then the linter was complaining about sets.String on CI checks.
There was a problem hiding this comment.
It's okay to ignore linter hints. Use common sense to decide whether it's a) worth fixing and b) belongs into the PR as a drive-by-change.
There was a problem hiding this comment.
I think it's okay to include here.
|
Updated this PR to use |
| tombstone, ok := obj.(cache.DeletedFinalStateUnknown) | ||
| if !ok { | ||
| utilruntime.HandleError(fmt.Errorf("Couldn't get object from tombstone %#v", obj)) | ||
| utilruntime.HandleErrorWithLogger(logger, fmt.Errorf("couldn't get object from tombstone %#v", obj), "Couldn't get object from tombstone") |
There was a problem hiding this comment.
| utilruntime.HandleErrorWithLogger(logger, fmt.Errorf("couldn't get object from tombstone %#v", obj), "Couldn't get object from tombstone") | |
| utilruntime.HandleErrorWithLogger(logger, nil, "Couldn't get object from tombstone", "obj", obj) |
// In contrast to HandleError, passing nil for the error is still going to
// trigger a log entry. Don't construct a new error or wrap an error
// with fmt.Errorf. Instead, add additional information via the mssage
// and key/value pairs.
| endpointSlice, ok := tombstone.Obj.(*discovery.EndpointSlice) | ||
| if !ok { | ||
| utilruntime.HandleError(fmt.Errorf("Tombstone contained object that is not a EndpointSlice: %#v", obj)) | ||
| utilruntime.HandleErrorWithLogger(logger, fmt.Errorf("tombstone contained object that is not a EndpointSlice: %#v", obj), "Tombstone contained an object that is not a Pod.") |
| logger.V(2).Info("synced garbage collector") | ||
| } else { | ||
| utilruntime.HandleError(fmt.Errorf("timed out waiting for dependency graph builder sync during GC sync")) | ||
| utilruntime.HandleErrorWithContext(ctx, fmt.Errorf("timed out waiting for dependency graph builder sync during GC sync"), "Failed to sync garbage collector") |
| n, ok := item.(*node) | ||
| if !ok { | ||
| utilruntime.HandleError(fmt.Errorf("expect *node, got %#v", item)) | ||
| utilruntime.HandleErrorWithContext(ctx, fmt.Errorf("expect *node, got %#v", item), "Invalid item in attemptToDeleteWorker") |
| owner, ok := item.(*node) | ||
| if !ok { | ||
| utilruntime.HandleError(fmt.Errorf("expect *node, got %#v", item)) | ||
| utilruntime.HandleErrorWithLogger(logger, fmt.Errorf("expect *node, got %#v", item), "Invalid item in attemptToOrphanWorker") |
There was a problem hiding this comment.
And here.
Usually %T would be used to show the actual type, not the variable content. I think it's okay to update this when touching the line.
| pod, ok = tombstone.Obj.(*v1.Pod) | ||
| if !ok { | ||
| utilruntime.HandleError(fmt.Errorf("tombstone contained object that is not a pod %#v", obj)) | ||
| utilruntime.HandleErrorWithLogger(logger, fmt.Errorf("tombstone contained object that is not a pod %#v", obj), "Unexpected object type in tombstone") |
| rsKey, err := controller.KeyFunc(rs) | ||
| if err != nil { | ||
| utilruntime.HandleError(fmt.Errorf("couldn't get key for %v %#v: %v", rsc.Kind, rs, err)) | ||
| utilruntime.HandleErrorWithContext(ctx, err, "Couldn't get key for replicaset", "repliset_kind", rsc.Kind, "replicaset", klog.KObj(rs)) |
| // Periodic resync will send update events for all known pods. | ||
| // Two different versions of the same pod will always have different RVs | ||
| return sets.String{} | ||
| return sets.Set[string]{} |
There was a problem hiding this comment.
I think it is safe to return nil here: an empty map can be read from. Depends on whether the caller then uses the set only for reading or also for writing.
| tombstone, ok := obj.(cache.DeletedFinalStateUnknown) | ||
| if !ok { | ||
| utilruntime.HandleError(fmt.Errorf("couldn't get object from tombstone %#v", obj)) | ||
| utilruntime.HandleErrorWithLogger(logger, fmt.Errorf("couldn't get object from tombstone %#v", obj), "Couldn't get object from tombstone") |
| pod, ok := tombstone.Obj.(*v1.Pod) | ||
| if !ok { | ||
| utilruntime.HandleError(fmt.Errorf("tombstone contained object that is not a Pod: %#v", obj)) | ||
| utilruntime.HandleErrorWithLogger(logger, fmt.Errorf("tombstone contained object that is not a Pod: %#v", obj), "Tombstone contained an object that is not a Pod.") |
82b82d7 to
b2ef4bf
Compare
|
@pohly updated all call-sites to remove fmt.Errorf in HandleError calls. |
|
/retest |
| go func() { | ||
| defer utilruntime.HandleCrash() | ||
| e.checkLeftoverEndpoints() | ||
| defer utilruntime.HandleCrashWithContext(ctx) |
There was a problem hiding this comment.
Nit: HandleCrashWithLogger would be more consistent with next line.
| endpointSlice := obj.(*discovery.EndpointSlice) | ||
| if endpointSlice == nil { | ||
| utilruntime.HandleError(fmt.Errorf("Invalid EndpointSlice provided to onEndpointSliceAdd()")) | ||
| utilruntime.HandleErrorWithLogger(logger, nil, "unable to extract endpointSlice from the provided object", "object", obj) |
There was a problem hiding this comment.
| utilruntime.HandleErrorWithLogger(logger, nil, "unable to extract endpointSlice from the provided object", "object", obj) | |
| utilruntime.HandleErrorWithLogger(logger, nil, "Unable to extract endpointSlice from the provided object", "object", obj) |
| endpointSlice := obj.(*discovery.EndpointSlice) | ||
| if endpointSlice == nil || prevEndpointSlice == nil { | ||
| utilruntime.HandleError(fmt.Errorf("Invalid EndpointSlice provided to onEndpointSliceUpdate()")) | ||
| utilruntime.HandleErrorWithLogger(logger, nil, "unable to extract endpointSlice from the provided object", "prevObject", prevObj, "object", obj) |
There was a problem hiding this comment.
| utilruntime.HandleErrorWithLogger(logger, nil, "unable to extract endpointSlice from the provided object", "prevObject", prevObj, "object", obj) | |
| utilruntime.HandleErrorWithLogger(logger, nil, "Unable to extract endpointSlice from the provided object", "prevObject", prevObj, "object", obj) |
| endpointSlice, ok := tombstone.Obj.(*discovery.EndpointSlice) | ||
| if !ok { | ||
| utilruntime.HandleError(fmt.Errorf("Tombstone contained object that is not a EndpointSlice: %#v", obj)) | ||
| utilruntime.HandleErrorWithLogger(logger, nil, "Tombstone contained object that is not a EndpointSlice", "actual", fmt.Sprintf("%T", obj)) |
There was a problem hiding this comment.
| utilruntime.HandleErrorWithLogger(logger, nil, "Tombstone contained object that is not a EndpointSlice", "actual", fmt.Sprintf("%T", obj)) | |
| utilruntime.HandleErrorWithLogger(logger, nil, "Tombstone contained object that is not a EndpointSlice", "type", fmt.Sprintf("%T", obj)) |
There was a problem hiding this comment.
"actualType" would also work, but "actual" isn't really meaningful by itself.
| n, ok := item.(*node) | ||
| if !ok { | ||
| utilruntime.HandleError(fmt.Errorf("expect *node, got %#v", item)) | ||
| utilruntime.HandleErrorWithContext(ctx, nil, "Expected *node", "actual", fmt.Sprintf("%T", item)) |
There was a problem hiding this comment.
| utilruntime.HandleErrorWithContext(ctx, nil, "Expected *node", "actual", fmt.Sprintf("%T", item)) | |
| utilruntime.HandleErrorWithContext(ctx, nil, "Expected *node", "type", fmt.Sprintf("%T", item)) |
| err = gc.removeFinalizer(logger, owner, metav1.FinalizerOrphanDependents) | ||
| if err != nil { | ||
| utilruntime.HandleError(fmt.Errorf("removeOrphanFinalizer for %s failed with %v", owner.identity, err)) | ||
| utilruntime.HandleErrorWithLogger(logger, err, "RemoveOrphanFinalizer for owner failed", "owner", owner.identity) |
There was a problem hiding this comment.
| utilruntime.HandleErrorWithLogger(logger, err, "RemoveOrphanFinalizer for owner failed", "owner", owner.identity) | |
| utilruntime.HandleErrorWithLogger(logger, err, "Removing orphan finalizer for owner failed", "owner", owner.identity) |
Log messages should be ideally be plain English.
| pod, ok = tombstone.Obj.(*v1.Pod) | ||
| if !ok { | ||
| utilruntime.HandleError(fmt.Errorf("tombstone contained object that is not a pod %#v", obj)) | ||
| utilruntime.HandleErrorWithLogger(logger, nil, "Tombstone contained object that is not a pod", "actual", fmt.Sprintf("%T", obj)) |
There was a problem hiding this comment.
| utilruntime.HandleErrorWithLogger(logger, nil, "Tombstone contained object that is not a pod", "actual", fmt.Sprintf("%T", obj)) | |
| utilruntime.HandleErrorWithLogger(logger, nil, "Tombstone contained object that is not a pod", "type", fmt.Sprintf("%T", obj)) |
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "github.com/onsi/gomega" |
| // GetServicesToUpdateOnPodChange returns a set of Service keys for Services | ||
| // that have potentially been affected by a change to this pod. | ||
| func GetServicesToUpdateOnPodChange(serviceLister v1listers.ServiceLister, old, cur interface{}) sets.String { | ||
| func GetServicesToUpdateOnPodChange(logger klog.Logger, serviceLister v1listers.ServiceLister, old, cur interface{}) sets.Set[string] { |
There was a problem hiding this comment.
I think it's okay to include here.
b2ef4bf to
2553dc2
Compare
|
@pohly Can you please take another look when you get a chance. Thanks! |
| if err != nil { | ||
| return nil, err | ||
| // Return an empty set instead of nil on error to match the return type. | ||
| return sets.Set[string]{}, err |
There was a problem hiding this comment.
I don't understand the "to match the return type".
nil works. It's an untyped constant that gets converted to sets.Set[string](nil) automatically.
I think it is more idiomatic this way:
diff --git a/staging/src/k8s.io/endpointslice/util/controller_utils.go b/staging/src/k8s.io/endpointslice/util/controller_utils.go
index 1949f559b07..87c345a57d9 100644
--- a/staging/src/k8s.io/endpointslice/util/controller_utils.go
+++ b/staging/src/k8s.io/endpointslice/util/controller_utils.go
@@ -51,13 +51,12 @@ var semanticIgnoreResourceVersion = conversion.EqualitiesOrDie(
// GetPodServiceMemberships returns a set of Service keys for Services that have
// a selector matching the given pod.
func GetPodServiceMemberships(serviceLister v1listers.ServiceLister, pod *v1.Pod) (sets.Set[string], error) {
- // Initialize the set using the new generic type.
- set := sets.Set[string]{}
services, err := serviceLister.Services(pod.Namespace).List(labels.Everything())
if err != nil {
- return set, err
+ return nil, err
}
+ set := sets.New[string]()
for _, service := range services {
if service.Spec.Selector == nil {
// If the service has a nil selector this means selectors match nothing, not everything.
@@ -67,8 +66,7 @@ func GetPodServiceMemberships(serviceLister v1listers.ServiceLister, pod *v1.Pod
if labels.ValidatedSetSelector(service.Spec.Selector).Matches(labels.Set(pod.Labels)) {
key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(service)
if err != nil {
- // Return an empty set instead of nil on error to match the return type.
- return sets.Set[string]{}, err
+ return nil, err
}
set.Insert(key)
}| utilruntime.HandleError(fmt.Errorf("unable to get pod %s/%s's service memberships: %v", newPod.Namespace, newPod.Name, err)) | ||
| return sets.String{} | ||
| utilruntime.HandleErrorWithLogger(logger, err, "Unable to get pod's service memberships", "pod", klog.KObj(newPod)) | ||
| return sets.Set[string]{} |
There was a problem hiding this comment.
| return sets.Set[string]{} | |
| return nil |
| retval := determineNeededServiceUpdates(testCase.a, testCase.b, false) | ||
| if !retval.Equal(testCase.xor) { | ||
| t.Errorf("%s (with podChanged=false): expected: %v got: %v", testCase.name, testCase.xor.List(), retval.List()) | ||
| t.Errorf("%s (with podChanged=false): expected: %v got: %v", testCase.name, testCase.xor.UnsortedList(), retval.UnsortedList()) |
There was a problem hiding this comment.
| t.Errorf("%s (with podChanged=false): expected: %v got: %v", testCase.name, testCase.xor.UnsortedList(), retval.UnsortedList()) | |
| t.Errorf("%s (with podChanged=false): expected: %v got: %v", testCase.name, sets.List(testCase.xor), sets.List(retval)) |
The error message is easier to read if entries are sorted.
| retval = determineNeededServiceUpdates(testCase.a, testCase.b, true) | ||
| if !retval.Equal(testCase.union) { | ||
| t.Errorf("%s (with podChanged=true): expected: %v got: %v", testCase.name, testCase.union.List(), retval.List()) | ||
| t.Errorf("%s (with podChanged=true): expected: %v got: %v", testCase.name, testCase.union.UnsortedList(), retval.UnsortedList()) |
There was a problem hiding this comment.
The tests can be simplified by relying on "nil set is a valid set" and automatic inference of the template type:
diff --git a/staging/src/k8s.io/endpointslice/util/controller_utils_test.go b/staging/src/k8s.io/endpointslice/util/controller_utils_test.go
index b5f06a270b6..63e448e8064 100644
--- a/staging/src/k8s.io/endpointslice/util/controller_utils_test.go
+++ b/staging/src/k8s.io/endpointslice/util/controller_utils_test.go
@@ -42,45 +42,45 @@ func TestDetermineNeededServiceUpdates(t *testing.T) {
}{
{
name: "no services changed",
- a: sets.New[string]("a", "b", "c"),
- b: sets.New[string]("a", "b", "c"),
- xor: sets.New[string](),
- union: sets.New[string]("a", "b", "c"),
+ a: sets.New("a", "b", "c"),
+ b: sets.New("a", "b", "c"),
+ xor: nil,
+ union: sets.New("a", "b", "c"),
},
{
name: "all old services removed, new services added",
- a: sets.New[string]("a", "b", "c"),
- b: sets.New[string]("d", "e", "f"),
- xor: sets.New[string]("a", "b", "c", "d", "e", "f"),
- union: sets.New[string]("a", "b", "c", "d", "e", "f"),
+ a: sets.New("a", "b", "c"),
+ b: sets.New("d", "e", "f"),
+ xor: sets.New("a", "b", "c", "d", "e", "f"),
+ union: sets.New("a", "b", "c", "d", "e", "f"),
},
{
name: "all old services removed, no new services added",
- a: sets.New[string]("a", "b", "c"),
- b: sets.New[string](),
- xor: sets.New[string]("a", "b", "c"),
- union: sets.New[string]("a", "b", "c"),
+ a: sets.New("a", "b", "c"),
+ b: nil,
+ xor: sets.New("a", "b", "c"),
+ union: sets.New("a", "b", "c"),
},
{
name: "no old services, but new services added",
- a: sets.New[string](),
- b: sets.New[string]("a", "b", "c"),
- xor: sets.New[string]("a", "b", "c"),
- union: sets.New[string]("a", "b", "c"),
+ a: nil,
+ b: sets.New("a", "b", "c"),
+ xor: sets.New("a", "b", "c"),
+ union: sets.New("a", "b", "c"),
},
{
name: "one service removed, one service added, two unchanged",
- a: sets.New[string]("a", "b", "c"),
- b: sets.New[string]("b", "c", "d"),
- xor: sets.New[string]("a", "d"),
- union: sets.New[string]("a", "b", "c", "d"),
+ a: sets.New("a", "b", "c"),
+ b: sets.New("b", "c", "d"),
+ xor: sets.New("a", "d"),
+ union: sets.New("a", "b", "c", "d"),
},
{
name: "no services",
- a: sets.New[string](),
- b: sets.New[string](),
- xor: sets.New[string](),
- union: sets.New[string](),
+ a: nil,
+ b: nil,
+ xor: nil,
+ union: nil,
},
}
@@ -384,12 +384,12 @@ func TestGetPodServiceMemberships(t *testing.T) {
{
name: "get servicesMemberships for pod-3",
pod: pods[3],
- expect: sets.New[string](),
+ expect: nil,
},
{
name: "get servicesMemberships for pod-4",
pod: pods[4],
- expect: sets.New[string](),
+ expect: nil,
},
}
for _, test := range tests {
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Replace HandleError and HandleCrash with their context-aware alternatives in the following controllers:
Which issue(s) this PR is related to:
Contributes to #126379
Special notes for your reviewer:
Does this PR introduce a user-facing change?