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
Promote service load balancer finalizer to GA #85023
Conversation
/sig network |
@@ -307,8 +307,6 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) { | |||
|
|||
for _, tc := range testCases { | |||
t.Run(tc.desc, func(t *testing.T) { | |||
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, serviceLoadBalancerFinalizerFeature, tc.enableFeatureGate)() |
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.
Can we remove serviceLoadBalancerFinalizerFeature
or add a TODO to remove in a follow-up PR?
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.
Thanks Andrew. Totally, removed serviceLoadBalancerFinalizerFeature
as that is no longer needed.
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.
minor comment, lgtm otherwise
pkg/controller/service/controller.go
Outdated
if err := s.addFinalizer(service); err != nil { | ||
return op, fmt.Errorf("failed to add load balancer cleanup finalizer: %v", err) | ||
} | ||
// Always try to add finalizer prior to load balancer creation. |
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.
Wondering if this comment should be updated now. Maybe just state the intent now that it's the default behavior.
// Always add a finalizer prior to creating load balancers, this ensures Services can't be deleted
// until all corresponding load balancer resources are also deleted.
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.
same for removeFinalizer above
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.
Yes that make more sense. Done.
kubeClient clientset.Interface | ||
clusterName string | ||
balancer cloudprovider.LoadBalancer | ||
// TODO(#85155): Stop relying on this and remove the cache completely. |
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.
BTW created #85155 for cleaning up the cache layer eventually.
/lgtm |
/assign @thockin |
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.
Thanks!
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MrHohn, thockin 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Ref kubernetes/enhancements#980 and kubernetes/cloud-provider#16. This PR promotes service load balancer finalizer to GA. There is no behavioral change.
Which issue(s) this PR fixes:
Fixes #NONE
Special notes for your reviewer:
/assign @andrewsykim @bowei @thockin
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: