pkg/destroy/aws: Fix fail-slow error handling in deleteEC2SubnetByVPC #2497
Conversation
openshift-ci-robot
added
size/S
pkg/destroy/aws/aws.go
Outdated
There was a problem hiding this comment.
i'm not sure what this assignment wants to achieve, err is from DescribeSubnets call
There was a problem hiding this comment.
We only enter this loop when err from DescribeSubnets was nill. This commit just recycles that variable for the first deleteEC2Subnet error.
There was a problem hiding this comment.
Aaand I need to return err at tge end ;).
There was a problem hiding this comment.
Aaand I need to return err at [the] end ;).
Done with cdb8e0cdd->013ff7a70.
There was a problem hiding this comment.
So this captures first failure, eats it and returns that , while all following errors are only logged at debug.
This is a weird behavior.. do we have a problem if we just collect them and report all at the end?
There was a problem hiding this comment.
This is a weird behavior.. do we have a problem if we just collect them and report all at the end?
Sure, but we're just going to log it and throw the error out later.
There was a problem hiding this comment.
What I have now is very similar to the existing lastError log-and-forget pattern we have here, etc.. How extensively do you want me to rewrite this package?
There was a problem hiding this comment.
What I have now is very similar to the existing lastError log-and-forget pattern we have here, etc
Yeah, I was looking at other functions. So let's keep this behavior consistent at least with existing for now.
But maybe can we use the lastError, err like others compared to this err shadowing, err2 to be more readable.
There was a problem hiding this comment.
But maybe can we use the lastError...
Rerolled to use lastError with 013ff7a70 -> 58524d5.
There was a problem hiding this comment.
This eats the first error all together ;) maybe that's fine as that's what other places do
wking
force-pushed
the
fail-slow-error-logging
branch
from
cdb8e0c to
013ff7a
Compare
Fix a bug from 37a7f49 (pkg/destroy/aws: Delete subnets by VPC, 2019-08-13, openshift#2214), where failFast=false lead to all deleteEC2Subnet errors being ignored. Luckily we have 'failFast: true' for them, so no exposed change, but fixing this bug protects us from exposing it if we toggle failFast in the future.
wking
force-pushed
the
fail-slow-error-logging
branch
from
013ff7a to
58524d5
Compare
openshift-ci-robot
added
size/XS
|
@wking: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Fix a bug from 37a7f49 (#2214), where
failFast=falselead to all deleteEC2Subnet errors being ignored. Luckily we havefailFast: truefor them, so no exposed change, but fixing this bug protects us from exposing it if we toggle failFast in the future.