-
Notifications
You must be signed in to change notification settings - Fork 350
refact/lb/sg: isolate security group deletion fragments from EnsureLoadBalancerDeleted #1159
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
Conversation
Hi @mtulio. 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. |
probably won't get to this today, but i will review early next week. |
I am able to run the tests locally: $ make test
....
--- PASS: TestGetNodeTopology/Should_return_unhandled_errors (0.00s)
PASS
ok k8s.io/cloud-provider-aws/pkg/resourcemanagers 1.025s
? k8s.io/cloud-provider-aws/pkg/services [no test files]
$ ./e2e.test --ginkgo.v
....
-----------------------------
Summarizing 1 Failure:
[FAIL] [cloud-provider-aws-e2e] ecr [It] should start pod using public ecr image
/home/mtulio/go/pkg/mod/k8s.io/[email protected]/test/e2e/framework/pod/pod_client.go:107
Ran 6 of 6 Specs in 710.314 seconds
FAIL! -- 5 Passed | 1 Failed | 0 Pending | 0 Skipped
--- FAIL: TestE2E (710.31s)
suite_test.go:46: ReportDir:
FAIL Looks like the failed one would be related with my local setup, needs label ok-to-test to validate it in controlled environment. |
/ok-to-test |
5ac5356
to
2785cbb
Compare
2785cbb
to
55d583b
Compare
I am observing a permanent failure on CI when launching the cluster trying to use an image that is no longer available:
This is also happening in other PRs I am watching. Is this comes from the test framework or is it possible to use a valid image in CCM repo? |
/test pull-cloud-provider-aws-e2e |
An issue has been opened to track the CI problem: #1167 |
Hi @yue9944882 @kmala , feedback addressed. Would you mind taking a look? I am still waiting/looking for CI to fix e2e job (#1167), but this is ready for review. |
/test pull-cloud-provider-aws-e2e |
Looks like e2e is now working, and tests are green awaiting for feedback! Thanks! |
55d583b
to
82f61bc
Compare
Hey @kmala , feedback addressed. PTAL? Thanks! |
/triage accepted |
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 makes sense to me, ++ on the code docs too.
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kmala 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 |
Isolating security group deletion fragments from EnsureLoadBalancerDeleted to buildSecurityGroupsToDelete and deleteSecurityGroupsWithBackoff, so the envaluation criteria and backof deletion can be reused in future implementations, i.e. NLB with Security Groups.
82f61bc
to
e454240
Compare
/lgtm |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Isolating security group deletion fragments from
EnsureLoadBalancerDeleted
tobuildSecurityGroupsToDelete
anddeleteSecurityGroupsWithBackoff
, so the evaluation criteria and backoff deletion can be reused in future implementations, i.e. NLB with Security Groups.This change contributes to decrease the change scope of #1158.
Only the backoff logic has been changed to add exponencial check, preventing
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: