Cloud-provider: add event filter method for cloud provider implementations in service controller #108914

MartinForReal · 2022-03-23T09:29:28Z

Signed-off-by: MartinForReal fanshangxiang@gmail.com

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add LoadBalancerClass filter in service controller of cloud-provifer

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

MartinForReal · 2022-03-23T09:30:40Z

/triage accepted

MartinForReal · 2022-03-23T09:43:40Z

/assign @andrewsykim

MartinForReal · 2022-03-23T10:24:48Z

/assign @thockin

andrewsykim

/cc @XudongLiuHarold

andrewsykim · 2022-03-23T13:27:37Z

staging/src/k8s.io/cloud-provider/controllers/service/controller.go

@@ -108,6 +111,7 @@ func New(
 	nodeInformer coreinformers.NodeInformer,
 	clusterName string,
 	featureGate featuregate.FeatureGate,
+	loadBalancerClasses ...*string,


IF we wanted to do this, I think we want to add an interface method to the LoadBalancers interface for the allowed class names as opposed to adding an additional parameter to the controller constructor.

For most of the existing cloud providers, they handle all of svc change events. Do you think that we need to enforce them to accept this break change?

Here we add an optional parameter to enable filtering on different LoadBalancer Classes. This parameter is not required for existing implementations.

If we need to do this by introducing a new interface. Do you think following interface would be enough ?

type LoadBalancerClassSupported interface { GetWatchedLoadBalancerClassList() []*string }

For most of the existing cloud providers, they handle all of svc change events.

I'm not sure what you mean by this, we currently filter for Type=LoadBalancer and when the class is NOT set:

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/cloud-provider/controllers/service/controller.go#L835

^ adding the filter for when the class is not set was to preserve existing behavior

Yes. For most of the existing cloud providers, they will watch for changes in services whose type is LoadBalancer
I think we are one the same page

Adding a new method here https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/cloud-provider/cloud.go#L133 might make the most sense, but I need to give it more thought

@XudongLiuHarold , any advice is appreciated! Thanks!

XudongLiuHarold · 2022-03-24T18:40:29Z

staging/src/k8s.io/cloud-provider/controllers/service/controller.go

+	if service.Spec.Type == v1.ServiceTypeLoadBalancer {
+		if service.Spec.LoadBalancerClass == nil {
+			// Default provider should accept svc, and other provider should ignore
+			return len(allowedLoadbalancerClass) == 0


Shouldn't this just return true?

If service.Spec.LoadBalancerClass is nil, which means users want to use the cloud provider's load balancer implementation, no matter there is allowedLoadbalancerClass or not, right?

Imagine there are a few cloud providers available. the default provider will handle all svc whose type is ServiceTypeLoadBalancer and LoadBalancerClass is nil. allowedLoadbalancerClass is empty if this informer is created by default provider
other providers only handle svc whose LoadBalancerClass is not nil. allowedLoadbalancerClass is not empty, the provider here will only handle request whose LoadBalancerClass falls in allowedLoadbalancerClass

IF LoadBalancerClass is nil, This svc should be ignored by other provider and handled by the default provider

XudongLiuHarold · 2022-03-24T18:48:11Z

staging/src/k8s.io/cloud-provider/controllers/service/controller.go

+
+		// if LoadBalancerClass is set, the user does not want the default cloud-provider Load Balancer
+		for _, className := range allowedLoadbalancerClass {
+			if strings.EqualFold(*className, *service.Spec.LoadBalancerClass) {


If we found the supported LoadBalancerClass, shouldn't we return false for cloud providers? Since users indicate that they don't want the cloud providers' load balancer implementation and we also found that the implementation they want to use is also supported.

wantloadbalancer return true only when svc is accpeted by current load balancer implementation. The svc is accepted only when svc meets LoadBalancerClass requirement set by loadbalancer implementation.
If this controller is launched by non-default cloud provider, here it should return true.

andrewsykim · 2022-03-25T14:41:20Z

staging/src/k8s.io/cloud-provider/controllers/service/controller.go

@@ -97,6 +98,8 @@ type Controller struct {
 	nodeSyncCh chan interface{}
 	// needFullSync indicates if the nodeSyncInternal will do a full node sync on all LB services.
 	needFullSync bool
+	// loadBalancerClasses indicates list of accepted LoadbalancerClasses. if empty, the provider accepts all.
+	loadBalancerClasses []*string


If we're going to allow the provider implementations to allow-list the class names they should watch, it should be done through the LoadBalancer interface instead of a parameter into the controllers constructor.

Btw, I also added this topic in the next SIG meeting in case other folks have thoughts/opinons on this.

MartinForReal · 2022-03-31T02:14:52Z

I attended the SIG Meeting(but didn't say anything) and we would like to add an extra function in cloudprovider interface. wouldn't we? @andrewsykim

Signed-off-by: MartinForReal <fanshangxiang@gmail.com>

k8s-ci-robot · 2022-03-31T05:14:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MartinForReal
To complete the pull request process, please ask for approval from andrewsykim after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

staging/src/k8s.io/cloud-provider/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

MartinForReal · 2022-03-31T05:20:47Z

Hi @andrewsykim, @XudongLiuHarold ,

I added a method in cloud provider and the implementation would decide whether service object should be put into worker queue.

Any advice is appreciated. Thanks!

k8s-ci-robot · 2022-03-31T05:58:24Z

@MartinForReal: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-integration	`46fa449`	link	true	`/test pull-kubernetes-integration`
pull-kubernetes-node-e2e-containerd	`46fa449`	link	true	`/test pull-kubernetes-node-e2e-containerd`
pull-kubernetes-conformance-kind-ga-only-parallel	`46fa449`	link	true	`/test pull-kubernetes-conformance-kind-ga-only-parallel`
pull-kubernetes-e2e-gce-ubuntu-containerd	`46fa449`	link	true	`/test pull-kubernetes-e2e-gce-ubuntu-containerd`
pull-kubernetes-e2e-kind-ipv6	`46fa449`	link	true	`/test pull-kubernetes-e2e-kind-ipv6`
pull-kubernetes-typecheck	`46fa449`	link	true	`/test pull-kubernetes-typecheck`
pull-kubernetes-verify-govet-levee	`46fa449`	link	true	`/test pull-kubernetes-verify-govet-levee`
pull-kubernetes-e2e-gce-100-performance	`46fa449`	link	true	`/test pull-kubernetes-e2e-gce-100-performance`
pull-kubernetes-e2e-kind	`46fa449`	link	true	`/test pull-kubernetes-e2e-kind`
pull-kubernetes-unit	`46fa449`	link	true	`/test pull-kubernetes-unit`
pull-kubernetes-verify	`46fa449`	link	true	`/test pull-kubernetes-verify`

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.

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.

MartinForReal · 2022-05-24T02:34:20Z

1.24 has been released and I received an email about calling for kep. Should I wirte a kep first?

k8s-ci-robot · 2022-08-02T14:34:14Z

@MartinForReal: PR needs rebase.

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.

k8s-triage-robot · 2022-10-31T14:47:05Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

MartinForReal · 2022-11-01T01:44:57Z

/remove-lifecycle stale

k8s-ci-robot requested review from andrewsykim and cheftako March 23, 2022 09:30

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 23, 2022

k8s-ci-robot assigned andrewsykim Mar 23, 2022

MartinForReal changed the title ~~Feature: add loadbalancerclass filter for service controller~~ Cloud-provider: add loadbalancerclass filter for service controller Mar 23, 2022

andrewsykim reviewed Mar 23, 2022

View reviewed changes

k8s-ci-robot requested a review from XudongLiuHarold March 23, 2022 13:27

XudongLiuHarold reviewed Mar 24, 2022

View reviewed changes

andrewsykim reviewed Mar 25, 2022

View reviewed changes

feature: add loadbalancerclass filter for service controller

46fa449

Signed-off-by: MartinForReal <fanshangxiang@gmail.com>

MartinForReal force-pushed the feature/cloud_provider_add_loadbalancerclass_filer branch from 7814d88 to 46fa449 Compare March 31, 2022 05:13

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 31, 2022

MartinForReal changed the title ~~Cloud-provider: add loadbalancerclass filter for service controller~~ Cloud-provider: add event filter method for cloud provider implementations in service controller Mar 31, 2022

MartinForReal mentioned this pull request Mar 31, 2022

Feature: add support for loadBalancerClass kubernetes-sigs/cloud-provider-azure#917

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 31, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 1, 2022

MartinForReal closed this Dec 9, 2022

MartinForReal deleted the feature/cloud_provider_add_loadbalancerclass_filer branch December 9, 2022 02:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cloud-provider: add event filter method for cloud provider implementations in service controller #108914

Cloud-provider: add event filter method for cloud provider implementations in service controller #108914

MartinForReal commented Mar 23, 2022

MartinForReal commented Mar 23, 2022

MartinForReal commented Mar 23, 2022

MartinForReal commented Mar 23, 2022

andrewsykim left a comment

andrewsykim Mar 23, 2022

MartinForReal Mar 23, 2022

andrewsykim Mar 23, 2022

andrewsykim Mar 23, 2022

MartinForReal Mar 23, 2022

andrewsykim Mar 23, 2022

MartinForReal Mar 24, 2022

XudongLiuHarold Mar 24, 2022 •

edited

MartinForReal Mar 25, 2022

XudongLiuHarold Mar 24, 2022

MartinForReal Mar 25, 2022

andrewsykim Mar 25, 2022

MartinForReal commented Mar 31, 2022

k8s-ci-robot commented Mar 31, 2022

MartinForReal commented Mar 31, 2022

k8s-ci-robot commented Mar 31, 2022

MartinForReal commented May 24, 2022

k8s-ci-robot commented Aug 2, 2022

k8s-triage-robot commented Oct 31, 2022

MartinForReal commented Nov 1, 2022

Cloud-provider: add event filter method for cloud provider implementations in service controller #108914

Cloud-provider: add event filter method for cloud provider implementations in service controller #108914

Conversation

MartinForReal commented Mar 23, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

MartinForReal commented Mar 23, 2022

MartinForReal commented Mar 23, 2022

MartinForReal commented Mar 23, 2022

andrewsykim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XudongLiuHarold Mar 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinForReal commented Mar 31, 2022

k8s-ci-robot commented Mar 31, 2022

MartinForReal commented Mar 31, 2022

k8s-ci-robot commented Mar 31, 2022

MartinForReal commented May 24, 2022

k8s-ci-robot commented Aug 2, 2022

k8s-triage-robot commented Oct 31, 2022

MartinForReal commented Nov 1, 2022

XudongLiuHarold Mar 24, 2022 •

edited