Align node filtering with kubernetes service controller #370

lbernail · 2018-06-27T12:56:41Z

Addresses #292

Use the same node selection logic as the kubernetes service controller:

filter out master nodes
filter out non-ready node
filter out nodes with label alpha.service-controller.kubernetes.io/exclude-balancer

In the kubernetes service controller, the service node exclusion is a feature gate but there is no feature gate on this controller, so I removed the feature gate test (it seems unlikely that this label would be applied to a node that doesn't require exclusion). If you think it's not a good call I can revisit this.

I have a custom build available on dockerhub for testing: lbernail/glbc:v0.2

rramkumar1 · 2018-06-27T17:43:16Z

/assign @bowei

rramkumar1 · 2018-06-27T18:09:37Z

/ok-to-test

rramkumar1 · 2018-07-25T18:49:34Z

@lbernail Sorry for the long delay. This looks fine to me, once you have rebased.

lbernail · 2018-07-26T10:41:38Z

@rramkumar1 : no problem. I just did the rebase

rramkumar1 · 2018-07-26T15:43:08Z

pkg/utils/utils.go

@@ -54,6 +56,10 @@ const (
 	AddInstances
 	// RemoveInstances used to record a call to RemoveInstances.
 	RemoveInstances
+	// Label for Master nodes
+	LabelNodeRoleMaster = "node-role.kubernetes.io/master"


One thing I didn't notice before: These constant are copied from the service controller code. Given that we don't vendor that code in here, how can we keep these values up-to-date?

Ref: https://github.com/kubernetes/kubernetes/blob/c97b2e0d1e663c94710664f73bf9faa878f014a5/pkg/controller/service/service_controller.go

I tried to find a better solution but no obvious came to mind
The master label is unlikely to change because it's used in many tools (and the alpha annotation is just alpha and I don't think it's worth implementing feature gates for this use case)

Happy to revise this if you have a suggestion

Seems fine as that is unlikely to change. Kubeadm also hardcoded it: https://github.com/kubernetes/kubernetes/blob/6ca14282562597709c909e9f7f3abdb600afc193/cmd/kubeadm/app/constants/constants.go#L181-L183

Could you update the comment to reference service controller?

// LabelNodeRoleMaster specifies that a node is a master // This is a duplicate definition of the constant in: // kuberentes/kubernetes/pkg/controller/service/service_controller.go

rramkumar1 · 2018-07-27T15:20:53Z

/lgtm
/assign @MrHohn
Do you have any thoughts?

MrHohn · 2018-07-27T17:22:03Z

pkg/utils/utils.go

@@ -54,6 +56,10 @@ const (
 	AddInstances
 	// RemoveInstances used to record a call to RemoveInstances.
 	RemoveInstances
+	// Label for Master nodes
+	LabelNodeRoleMaster = "node-role.kubernetes.io/master"


Seems fine as that is unlikely to change. Kubeadm also hardcoded it: https://github.com/kubernetes/kubernetes/blob/6ca14282562597709c909e9f7f3abdb600afc193/cmd/kubeadm/app/constants/constants.go#L181-L183

Could you update the comment to reference service controller?

// LabelNodeRoleMaster specifies that a node is a master // This is a duplicate definition of the constant in: // kuberentes/kubernetes/pkg/controller/service/service_controller.go

MrHohn · 2018-07-27T17:25:28Z

pkg/utils/utils.go

@@ -54,6 +56,10 @@ const (
 	AddInstances
 	// RemoveInstances used to record a call to RemoveInstances.
 	RemoveInstances
+	// Label for Master nodes
+	LabelNodeRoleMaster = "node-role.kubernetes.io/master"
+	// Label for nodes excluded from load-balancing


Can we use the same comment and reference service controller?

// LabelNodeRoleExcludeBalancer specifies that the node should be // exclude from load balancers created by a cloud provider. // This is a duplicate definition of the constant in: // kuberentes/kubernetes/pkg/controller/service/service_controller.go

MrHohn · 2018-07-27T18:21:39Z

pkg/utils/utils.go

@@ -339,7 +345,7 @@ IngressLoop:
 // TODO(rramkumar): Add a test for this.
 func GetReadyNodeNames(lister listers.NodeLister) ([]string, error) {
 	var nodeNames []string
-	nodes, err := lister.ListWithPredicate(NodeIsReady)


I saw the same logic is used in https://github.com/kubernetes/ingress-gce/blob/master/pkg/controller/translator/translator.go, is that relevant?

I think we should update that too yes
Not updating it did not break any of my tests but it makes a lot of sense to not include zones without nodes considered for load-balancing

Yeah that makes more sense, thanks for checking.

MrHohn · 2018-07-27T18:29:00Z

pkg/utils/utils.go

@@ -339,7 +345,7 @@ IngressLoop:
 // TODO(rramkumar): Add a test for this.
 func GetReadyNodeNames(lister listers.NodeLister) ([]string, error) {


Can we update the comment to reflect what it is doing now?

k8s-ci-robot · 2018-07-28T10:27:59Z

New changes are detected. LGTM label has been removed.

lbernail · 2018-07-28T10:31:27Z

I updated the comments and will add the same logic in translator.go

lbernail · 2018-07-31T10:49:30Z

@MrHohn I updated the PR to create a GetReadyNodes function which returns Nodes and not strings so it can be used in translator.go. I kept the GetReadyNodeNames function to avoid refactoring too many things but it now calls the GetReadyNodes function.
What do you think?

I'll do more testing on a cluster later this week

MrHohn · 2018-08-01T03:32:24Z

pkg/utils/utils.go

+	// This is a duplicate definition of the constant in:
+	// kubernetes/kubernetes/pkg/controller/service/service_controller.go
+	LabelNodeRoleMaster = "node-role.kubernetes.io/master"
+	// LabelNodeRoleMaster specifies that a node is a master


Should comment about LabelNodeRoleExcludeBalancer here instead.

MrHohn · 2018-08-01T03:40:57Z

pkg/utils/utils.go

@@ -335,19 +347,68 @@ IngressLoop:
 	return
 }

-// GetReadyNodeNames returns names of schedulable, ready nodes from the node lister.
+// GetReadyNodeNames returns names of ready nodes, excluding masters and ones exclued from load-balancing


nit: we should still include the schedulable bit in comment, as a node can be both ready and non-schedulable.

MrHohn · 2018-08-01T03:41:59Z

pkg/utils/utils.go

+	nodes, err := lister.ListWithPredicate(getNodeConditionPredicate())
+	if err != nil {
+		return readyNodes, err
+	}
 	for _, n := range nodes {
 		if n.Spec.Unschedulable {


This is duplicate with getNodeConditionPredicate(), which already checks node.Spec.Unschedulable?

Definitely, I missed that
I will export GetNodeConditionPredicate and simplify accordingly

MrHohn · 2018-08-01T03:43:49Z

I updated the PR to create a GetReadyNodes function which returns Nodes and not strings so it can be used in translator.go. I kept the GetReadyNodeNames function to avoid refactoring too many things but it now calls the GetReadyNodes function.

Thanks, that makes sense to me, left some minor comments.

MrHohn · 2018-08-04T01:22:38Z

@lbernail Thanks for the works, this LGTM. Could you squash the fix-up commits?

lbernail · 2018-08-06T21:19:54Z

Sure, I will take care of it in the next few days

lbernail · 2018-08-07T12:07:49Z

@MrHohn I squashed the PR into 2 commits, let me know if that works for you
In addition I did some testing and the new changes looks ok too. I tested:

master filtering
exclude-balancer filtering
nodes included in load-balancing in some zones only (instance groups were not created in the other ones)
nodes included in load-balancing in all zones

MrHohn · 2018-08-07T16:59:55Z

@lbernail LGTM. Thanks for verifying the change.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 27, 2018

k8s-ci-robot assigned bowei Jun 27, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 27, 2018

rramkumar1 reviewed Jul 26, 2018

View reviewed changes

k8s-ci-robot assigned rramkumar1 and MrHohn Jul 27, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2018

MrHohn reviewed Jul 27, 2018

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2018

MrHohn reviewed Aug 1, 2018

View reviewed changes

lbernail added 2 commits August 7, 2018 10:26

Align node filtering with kubernetes service controller

911d755

Align filtering logic for GetZoneForNode and ListZones

4580d9f

MrHohn merged commit d94a5a7 into kubernetes:master Aug 7, 2018

rramkumar1 mentioned this pull request Aug 10, 2018

Align node filtering with service controller #292

Closed

freehan mentioned this pull request Dec 20, 2018

Instance is Not Removed from IG when node is marked as unschedulable #591

Closed

lbernail mentioned this pull request May 13, 2019

REQUEST: New membership for lbernail kubernetes/org#811

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align node filtering with kubernetes service controller #370

Align node filtering with kubernetes service controller #370

lbernail commented Jun 27, 2018 •

edited

Loading

rramkumar1 commented Jun 27, 2018

rramkumar1 commented Jun 27, 2018

rramkumar1 commented Jul 25, 2018

lbernail commented Jul 26, 2018

rramkumar1 Jul 26, 2018 •

edited

Loading

lbernail Jul 27, 2018

MrHohn Jul 27, 2018

rramkumar1 commented Jul 27, 2018

MrHohn Jul 27, 2018

MrHohn Jul 27, 2018

MrHohn Jul 27, 2018

lbernail Jul 28, 2018

MrHohn Jul 30, 2018

MrHohn Jul 27, 2018

k8s-ci-robot commented Jul 28, 2018

lbernail commented Jul 28, 2018

lbernail commented Jul 31, 2018

MrHohn Aug 1, 2018

MrHohn Aug 1, 2018

MrHohn Aug 1, 2018

lbernail Aug 1, 2018 •

edited

Loading

MrHohn commented Aug 1, 2018

MrHohn commented Aug 4, 2018

lbernail commented Aug 6, 2018

lbernail commented Aug 7, 2018

MrHohn commented Aug 7, 2018

		@@ -339,7 +345,7 @@ IngressLoop:
		// TODO(rramkumar): Add a test for this.
		func GetReadyNodeNames(lister listers.NodeLister) ([]string, error) {

Align node filtering with kubernetes service controller #370

Align node filtering with kubernetes service controller #370

Conversation

lbernail commented Jun 27, 2018 • edited Loading

rramkumar1 commented Jun 27, 2018

rramkumar1 commented Jun 27, 2018

rramkumar1 commented Jul 25, 2018

lbernail commented Jul 26, 2018

rramkumar1 Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rramkumar1 commented Jul 27, 2018

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

k8s-ci-robot commented Jul 28, 2018

lbernail commented Jul 28, 2018

lbernail commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbernail Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

MrHohn commented Aug 1, 2018

MrHohn commented Aug 4, 2018

lbernail commented Aug 6, 2018

lbernail commented Aug 7, 2018

MrHohn commented Aug 7, 2018

lbernail commented Jun 27, 2018 •

edited

Loading

rramkumar1 Jul 26, 2018 •

edited

Loading

lbernail Aug 1, 2018 •

edited

Loading