Skip to content

Commit

Permalink
KCCM: add providerID predicate to service controller
Browse files Browse the repository at this point in the history
  • Loading branch information
@alexanderConstantinescu
alexanderConstantinescu committed Apr 18, 2023
1 parent 67c667c commit d09792e
Show file tree
Hide file tree
Showing 2 changed files with 149 additions and 27 deletions.
28 changes: 19 additions & 9 deletions staging/src/k8s.io/cloud-provider/controllers/service/controller.go
View file
Expand Up @@ -659,18 +659,22 @@ func nodeNames(nodes []*v1.Node) sets.String {

func shouldSyncUpdatedNode(oldNode, newNode *v1.Node) bool {
// Evaluate the individual node exclusion predicate before evaluating the
// compounded result of all predicates. We don't sync ETP=local services
// for changes on the readiness condition, hence if a node remains NotReady
// and a user adds the exclusion label we will need to sync as to make sure
// this change is reflected correctly on ETP=local services. The sync
// function compares lastSyncedNodes with the new (existing) set of nodes
// for each service, so services which are synced with the same set of nodes
// should be skipped internally in the sync function. This is needed as to
// trigger a global sync for all services and make sure no service gets
// skipped due to a changing node predicate.
// compounded result of all predicates. We don't sync changes on the
// readiness condition for eTP:Local services is enabled, hence if a node
// remains NotReady and a user adds the exclusion label we will need to sync
// as to make sure this change is reflected correctly on ETP=local services.
// The sync function compares lastSyncedNodes with the new (existing) set of
// nodes for each service, so services which are synced with the same set of
// nodes should be skipped internally in the sync function. This is needed
// as to trigger a global sync for all services and make sure no service
// gets skipped due to a changing node predicate.
if respectsPredicates(oldNode, nodeIncludedPredicate) != respectsPredicates(newNode, nodeIncludedPredicate) {
return true
}
// For the same reason as above, also check for changes to the providerID
if respectsPredicates(oldNode, nodeHasProviderIDPredicate) != respectsPredicates(newNode, nodeHasProviderIDPredicate) {
return true
}
return respectsPredicates(oldNode, allNodePredicates...) != respectsPredicates(newNode, allNodePredicates...)
}

Expand Down Expand Up @@ -927,10 +931,12 @@ var (
nodeIncludedPredicate,
nodeUnTaintedPredicate,
nodeReadyPredicate,
nodeHasProviderIDPredicate,
}
etpLocalNodePredicates []NodeConditionPredicate = []NodeConditionPredicate{
nodeIncludedPredicate,
nodeUnTaintedPredicate,
nodeHasProviderIDPredicate,
}
)

Expand All @@ -947,6 +953,10 @@ func nodeIncludedPredicate(node *v1.Node) bool {
return !hasExcludeBalancerLabel
}

func nodeHasProviderIDPredicate(node *v1.Node) bool {
return node.Spec.ProviderID != ""
}

// We consider the node for load balancing only when its not tainted for deletion by the cluster autoscaler.
func nodeUnTaintedPredicate(node *v1.Node) bool {
for _, taint := range node.Spec.Taints {
Expand Down
148 changes: 130 additions & 18 deletions staging/src/k8s.io/cloud-provider/controllers/service/controller_test.go
View file
Expand Up @@ -467,9 +467,9 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) {
// TODO: Finish converting and update comments
func TestUpdateNodesInExternalLoadBalancer(t *testing.T) {
nodes := []*v1.Node{
{ObjectMeta: metav1.ObjectMeta{Name: "node0"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}},
{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}},
{ObjectMeta: metav1.ObjectMeta{Name: "node73"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}},
{ObjectMeta: metav1.ObjectMeta{Name: "node0"}, Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}},
{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}},
{ObjectMeta: metav1.ObjectMeta{Name: "node73"}, Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}},
}
table := []struct {
desc string
Expand Down Expand Up @@ -573,13 +573,14 @@ func TestUpdateNodesInExternalLoadBalancer(t *testing.T) {
}

func TestNodeChangesForExternalTrafficPolicyLocalServices(t *testing.T) {
node1 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node0"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node2 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node2NotReady := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionFalse}}}}
node2Tainted := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Spec: v1.NodeSpec{Taints: []v1.Taint{{Key: ToBeDeletedTaint}}}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionFalse}}}}
node2SpuriousChange := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: v1.NodeStatus{Phase: v1.NodeTerminated, Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node2Exclude := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1", Labels: map[string]string{v1.LabelNodeExcludeBalancers: ""}}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node3 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node73"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node1 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node0"}, Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node2 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node2NotReady := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionFalse}}}}
node2Tainted := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Spec: v1.NodeSpec{ProviderID: providerID, Taints: []v1.Taint{{Key: ToBeDeletedTaint}}}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionFalse}}}}
node2SpuriousChange := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Phase: v1.NodeTerminated, Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node2Exclude := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1", Labels: map[string]string{v1.LabelNodeExcludeBalancers: ""}}, Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node2ProviderIDUnset := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1", Labels: map[string]string{v1.LabelNodeExcludeBalancers: ""}}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node3 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node73"}, Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}

type stateChanges struct {
nodes []*v1.Node
Expand Down Expand Up @@ -666,6 +667,20 @@ func TestNodeChangesForExternalTrafficPolicyLocalServices(t *testing.T) {
{Service: service3, Hosts: []*v1.Node{node1, node3}},
},
},
{
desc: "1 node gets providerID set",
initialState: []*v1.Node{node1, node2ProviderIDUnset, node3},
stateChanges: []stateChanges{
{
nodes: []*v1.Node{node1, node2, node3},
},
},
expectedUpdateCalls: []fakecloud.UpdateBalancerCall{
{Service: etpLocalservice1, Hosts: []*v1.Node{node1, node2, node3}},
{Service: etpLocalservice2, Hosts: []*v1.Node{node1, node2, node3}},
{Service: service3, Hosts: []*v1.Node{node1, node2, node3}},
},
},
{
desc: "1 node goes Ready",
initialState: []*v1.Node{node1, node2NotReady, node3},
Expand Down Expand Up @@ -745,10 +760,10 @@ func TestNodeChangesForExternalTrafficPolicyLocalServices(t *testing.T) {
}

func TestNodeChangesInExternalLoadBalancer(t *testing.T) {
node1 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node0"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node2 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node3 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node73"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node4 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node4"}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node1 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node0"}, Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node2 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node3 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node73"}, Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}
node4 := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node4"}, Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}

services := []*v1.Service{
newService("s0", "777", v1.ServiceTypeLoadBalancer),
Expand Down Expand Up @@ -1804,9 +1819,10 @@ func Test_respectsPredicates(t *testing.T) {
want bool
}{
{want: false, input: &v1.Node{}},
{want: true, input: &v1.Node{Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}},
{want: true, input: &v1.Node{Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}},
{want: false, input: &v1.Node{Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}}},
{want: false, input: &v1.Node{Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionFalse}}}}},
{want: true, input: &v1.Node{Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}, ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{}}}},
{want: true, input: &v1.Node{Spec: v1.NodeSpec{ProviderID: providerID}, Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}, ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{}}}},
{want: false, input: &v1.Node{Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}}, ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{v1.LabelNodeExcludeBalancers: ""}}}},

{want: false, input: &v1.Node{Status: v1.NodeStatus{Conditions: []v1.NodeCondition{{Type: v1.NodeReady, Status: v1.ConditionTrue}}},
Expand Down Expand Up @@ -1885,6 +1901,8 @@ func TestListWithPredicate(t *testing.T) {
}
}

var providerID = "providerID"

func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
testcases := []struct {
name string
Expand All @@ -1899,7 +1917,8 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
Name: "node",
},
Spec: v1.NodeSpec{
Taints: []v1.Taint{},
Taints: []v1.Taint{},
ProviderID: providerID,
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{
Expand All @@ -1920,6 +1939,7 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
Key: ToBeDeletedTaint,
},
},
ProviderID: providerID,
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{
Expand All @@ -1944,6 +1964,7 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
Key: ToBeDeletedTaint,
},
},
ProviderID: providerID,
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{
Expand All @@ -1959,7 +1980,8 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
Name: "node",
},
Spec: v1.NodeSpec{
Taints: []v1.Taint{},
Taints: []v1.Taint{},
ProviderID: providerID,
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{
Expand Down Expand Up @@ -2342,6 +2364,9 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "node",
},
Spec: v1.NodeSpec{
ProviderID: providerID,
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{
{
Expand All @@ -2355,6 +2380,9 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "node",
},
Spec: v1.NodeSpec{
ProviderID: providerID,
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{
{
Expand All @@ -2372,6 +2400,9 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "node",
},
Spec: v1.NodeSpec{
ProviderID: providerID,
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{
{
Expand All @@ -2385,6 +2416,9 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "node",
},
Spec: v1.NodeSpec{
ProviderID: providerID,
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{
{
Expand Down Expand Up @@ -2487,6 +2521,9 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "node",
},
Spec: v1.NodeSpec{
ProviderID: providerID,
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{
{
Expand All @@ -2500,6 +2537,9 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "node",
},
Spec: v1.NodeSpec{
ProviderID: providerID,
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{},
},
Expand Down Expand Up @@ -2537,6 +2577,9 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "node",
},
Spec: v1.NodeSpec{
ProviderID: providerID,
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{},
},
Expand All @@ -2545,6 +2588,9 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: "node",
},
Spec: v1.NodeSpec{
ProviderID: providerID,
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{
{
Expand Down Expand Up @@ -2728,6 +2774,72 @@ func Test_shouldSyncUpdatedNode_individualPredicates(t *testing.T) {
},
shouldSync: false,
},
{
name: "providerID set F->T",
oldNode: &v1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: "node",
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{
{
Type: v1.NodeReady,
Status: v1.ConditionTrue,
},
},
},
},
newNode: &v1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: "node",
},
Spec: v1.NodeSpec{
ProviderID: providerID,
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{
{
Type: v1.NodeReady,
Status: v1.ConditionTrue,
},
},
},
},
shouldSync: true,
},
{
name: "providerID set T->F",
oldNode: &v1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: "node",
},
Spec: v1.NodeSpec{
ProviderID: providerID,
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{
{
Type: v1.NodeReady,
Status: v1.ConditionTrue,
},
},
},
},
newNode: &v1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: "node",
},
Status: v1.NodeStatus{
Conditions: []v1.NodeCondition{
{
Type: v1.NodeReady,
Status: v1.ConditionTrue,
},
},
},
},
shouldSync: true,
},
}
for _, testcase := range testcases {
t.Run(testcase.name, func(t *testing.T) {
Expand Down

0 comments on commit d09792e

Please sign in to comment.