Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Automated cherry pick of #117388: Re-work logic in shouldSyncUpdatedNode #117452

Merged
merged 1 commit into from May 4, 2023
Merged

Automated cherry pick of #117388: Re-work logic in shouldSyncUpdatedNode #117452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 19 additions & 9 deletions staging/src/k8s.io/cloud-provider/controllers/service/controller.go
View file
Open in desktop
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
Open in desktop
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