-
Notifications
You must be signed in to change notification settings - Fork 333
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
base: master
Are you sure you want to change the base?
Conversation
This issue is currently awaiting triage. If cloud-provider-aws contributors determine 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 @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/kubernetes@v1.26.0/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
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
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! |
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.
55d583b
to
82f61bc
Compare
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?: