Deprecate the use of pkg/errors #2868
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci
bot
requested review from
cheesesashimi and
kikisdeliveryservice
|
/test e2e-aws-op |
|
@raisaat: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
|
/test e2e-gcp-op |
|
/test e2e-aws |
raisaat
changed the title
openshift-ci
bot
added
the
needs-rebase
raisaat
force-pushed
the
error-wrapping-golang
branch
from
87fda23
to
ebfe1f1
Compare
openshift-ci
bot
removed
the
needs-rebase
raisaat
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/test e2e-gcp-op |
|
/test e2e-aws |
|
You don't need to run test again. When you push any new changes, all tests will automatically rerun |
There was a problem hiding this comment.
Overall, this looks great! I just have a couple of minor comments / suggestions. The biggest one is the fix for the unit tests / linter issues.
Also, I think we may be missing a few cases:
$ rg -g "\!vendor" -g "*.go" --files-with-matches -F "github.com/pkg/errors"
pkg/server/api.go
pkg/daemon/on_disk_validation.goOnce those are addressed, we should be able to completely remove github.com/pkg/errors, though the actual removal should be done in a separate commit in this PR for easier review since it's vendored.
pkg/daemon/pivot/utils/run.go
Outdated
| } | ||
| return "", errors.Wrap(err, "failed to run command %s (%d tries): %v") | ||
| return "", fmt.Errorf("failed to run command %s (%d tries): %v: %w", err) |
There was a problem hiding this comment.
This is where the unit test failure is occurring. Basically, this fmt.Errorf requires four formatting arguments, but only one was passed in.
This should also fix the lint failure as well.
test/e2e-single-node/sno_mcd_test.go
Outdated
| } else { | ||
| return errors.Wrapf(err, "unkown error occured %v", lastErr) | ||
| return fmt.Errorf("unkown error occured %v: %w", lastErr, err) |
There was a problem hiding this comment.
| return fmt.Errorf("unkown error occured %v: %w", lastErr, err) | |
| return fmt.Errorf("unknown error occurred %v: %w", lastErr, err) |
|
/retest-required |
| @@ -183,14 +182,14 @@ func (ctrl *Controller) checkMasterNodesOnDelete(obj interface{}) { | |||
| } | |||
| currentMasters, err := ctrl.getCurrentMasters() | |||
| if err != nil { | |||
| goerrs.Wrap(err, "Reconciling to make master nodes schedulable/unschedulable failed") | |||
| fmt.Errorf("Reconciling to make master nodes schedulable/unschedulable failed: %w", err) | |||
There was a problem hiding this comment.
golangci-lint caught this. The reason why this was never caught before is most likely because the linter was not looking for the goerrs.Wrap() function. When this was changed to use fmt.Errorf(), the linter picked up on the pattern and alerted to an unused result since fmt.Errorf() is likely a more well-known signature.
That explanation aside, it's interesting that this error is never bubbled out of this function. This could be because this function is passed into a ResourceEventHandlerFuncs struct whose type does not accept a function with any return arguments. The same pattern can be seen throughout ctrl.checkMasterNodesOnDelete, ctrl.reconcileMaster and ctrl.reconcileMasters (all within this file, pkg/controller/node/node_controller.go).
As for a proposed remedy, I offer two suggestions:
- We replace these specific calls to
fmt.Errorf()with calls toglog.Errorf()so the errors are logged. This remedy would not introduce any unknown behavior and may help gain insight into how often these particular errors occur. - Instead of just logging the error, we handle it using the
utilruntime.HandleError()function which will try again using therudimentaryErrorBackoffmechanism within that package. This function appears to be used throughout this file. However, this could introduce unknown behavior.
There was a problem hiding this comment.
Interesting. I think paths like this one are very rarely considered in the first place, since we don't generally consider node deletion operations. In fact I am not sure whether we have any testing for this case, and whether we'd be able to auto recover, if at all.
I think right now I am leaning towards option 1, since I think it's fine to increase error logging in general in case these situations do happen. For 2, I don't know how we are handling the errors at all, so I am not sure what behaviour the HandleError actually exhibits for our use case (again, they should very rarely fire)
There was a problem hiding this comment.
Should we leave option 2 for now? I have made a commit for option 1. I can go ahead and squash it with the previous commits (after a rebase since there are merge conflicts).
pkg/daemon/on_disk_validation.go
Outdated
| @@ -156,7 +155,7 @@ func checkV3Files(files []ign3types.File) error { | |||
| var err error | |||
| contents, err = dataurl.DecodeString(*f.Contents.Source) | |||
| if err != nil { | |||
| return errors.Wrapf(err, "couldn't parse file %q", f.Path) | |||
| return fmt.Errorf("couldn't parse file %q: %w", f.Path, err) | |||
There was a problem hiding this comment.
Just an FYI: This may cause a merge conflict depending upon whether #2874 is merged before this is.
openshift-ci
bot
added
the
needs-rebase
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
raisaat
force-pushed
the
error-wrapping-golang
branch
from
e36a8eb
to
f5fc192
Compare
openshift-ci
bot
removed
the
needs-rebase
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
|
/test e2e-gcp-op |
|
/retest-required |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
hmm so the failures seem related to this PR /hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
Ah apologies, I duplicate some old code for that PR, and forgot to update error references. |
raisaat
force-pushed
the
error-wrapping-golang
branch
from
1f114c1
to
081b84b
Compare
raisaat
force-pushed
the
error-wrapping-golang
branch
from
081b84b
to
3a6fb1a
Compare
|
@kikisdeliveryservice Thanks! Fixed them. @yuqi-zhang not a problem :) |
openshift-ci
bot
added
the
needs-rebase
Signed-off-by: Raisaat Rashid <rarashid@redhat.com>
raisaat
force-pushed
the
error-wrapping-golang
branch
from
3a6fb1a
to
aba9ca0
Compare
openshift-ci
bot
removed
the
needs-rebase
|
Thanks @raisaat /lgtm |
openshift-ci
bot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi, kikisdeliveryservice, raisaat 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 |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@raisaat: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
Overriding the agnostic-upgrade job which keeps failing due to unrelated bugs and to prevent other PRs from merging which will require this to be updated again. /override ci/prow/e2e-agnostic-upgrade |
|
@kikisdeliveryservice: Overrode contexts on behalf of kikisdeliveryservice: ci/prow/e2e-agnostic-upgrade In response to this:
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. |
Deprecate the use of pkg/errors in MCO to use built-in Golang error-wrapping
Signed-off-by: Raisaat Rashid rarashid@redhat.com
- What I did
Changed all the code instances in machine-config-operator using pkg/errors to use built-in Golang error-wrapping instead.
- How to verify it
Make sure all the e2e and unit tests run properly.
- Description for the changelog
Deprecate the use of pkg/errors in machine-config-operator to use built-in Golang error-wrapping