Skip to content

Commit

Permalink
Merge pull request #2653 from Bryce-Soghigian/removing-deprecated-labels
Browse files Browse the repository at this point in the history 
Removed Deprecated labels from cloud-node-manager
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Nov 4, 2022
2 parents 7f028b1 + 67c2407 commit 95e8db9
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 163 deletions.
35 changes: 1 addition & 34 deletions pkg/nodemanager/nodemanager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,32 +72,7 @@ var labelReconcileInfo = []struct {
primaryKey string
secondaryKey string
ensureSecondaryExists bool
}{
{
// Reconcile the beta and the GA zone label using the beta label as
// the source of truth
// TODO: switch the primary key to GA labels in v1.21
primaryKey: v1.LabelZoneFailureDomain,
secondaryKey: v1.LabelZoneFailureDomainStable,
ensureSecondaryExists: true,
},
{
// Reconcile the beta and the stable region label using the beta label as
// the source of truth
// TODO: switch the primary key to GA labels in v1.21
primaryKey: v1.LabelZoneRegion,
secondaryKey: v1.LabelZoneRegionStable,
ensureSecondaryExists: true,
},
{
// Reconcile the beta and the stable instance-type label using the beta label as
// the source of truth
// TODO: switch the primary key to GA labels in v1.21
primaryKey: v1.LabelInstanceType,
secondaryKey: v1.LabelInstanceTypeStable,
ensureSecondaryExists: true,
},
}
}{}

// UpdateNodeSpecBackoff is the back configure for node update.
var UpdateNodeSpecBackoff = wait.Backoff{
Expand Down Expand Up @@ -464,44 +439,36 @@ func (cnc *CloudNodeController) getNodeModifiersFromCloudProvider(ctx context.Co
if instanceType, err := cnc.getInstanceTypeByName(ctx, node); err != nil {
return nil, err
} else if instanceType != "" {
klog.V(2).Infof("Adding node label from cloud provider: %s=%s", v1.LabelInstanceType, instanceType)
klog.V(2).Infof("Adding node label from cloud provider: %s=%s", v1.LabelInstanceTypeStable, instanceType)
nodeModifiers = append(nodeModifiers, func(n *v1.Node) {
if n.Labels == nil {
n.Labels = map[string]string{}
}
n.Labels[v1.LabelInstanceType] = instanceType
n.Labels[v1.LabelInstanceTypeStable] = instanceType
})
}

zone, err := cnc.getZoneByName(ctx, node)
if err != nil {
return nil, fmt.Errorf("failed to get zone from cloud provider: %w", err)
}
if zone.FailureDomain != "" {
klog.V(2).Infof("Adding node label from cloud provider: %s=%s", v1.LabelZoneFailureDomain, zone.FailureDomain)
klog.V(2).Infof("Adding node label from cloud provider: %s=%s", v1.LabelZoneFailureDomainStable, zone.FailureDomain)
nodeModifiers = append(nodeModifiers, func(n *v1.Node) {
if n.Labels == nil {
n.Labels = map[string]string{}
}
n.Labels[v1.LabelZoneFailureDomain] = zone.FailureDomain
n.Labels[v1.LabelZoneFailureDomainStable] = zone.FailureDomain
})
}
if zone.Region != "" {
klog.V(2).Infof("Adding node label from cloud provider: %s=%s", v1.LabelZoneRegion, zone.Region)
klog.V(2).Infof("Adding node label from cloud provider: %s=%s", v1.LabelZoneRegionStable, zone.Region)
nodeModifiers = append(nodeModifiers, func(n *v1.Node) {
if n.Labels == nil {
n.Labels = map[string]string{}
}
n.Labels[v1.LabelZoneRegion] = zone.Region
n.Labels[v1.LabelZoneRegionStable] = zone.Region
})
}

platformSubFaultDomain, err := cnc.getPlatformSubFaultDomain()
if err != nil {
return nil, fmt.Errorf("failed to get platformSubFaultDomain: %w", err)
Expand Down
139 changes: 10 additions & 129 deletions pkg/nodemanager/nodemanager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ package nodemanager
import (
"context"
"errors"
"reflect"
"fmt"
"testing"
"time"

Expand Down Expand Up @@ -404,17 +404,12 @@ func TestZoneInitialized(t *testing.T) {
cloudNodeController.AddCloudNode(context.TODO(), fnh.Existing[0])

assert.Equal(t, 1, len(fnh.UpdatedNodes), "Node was not updated")
fmt.Println(fnh.UpdatedNodes[0].ObjectMeta.Labels)
assert.Equal(t, "eastus", fnh.UpdatedNodes[0].ObjectMeta.Labels[v1.LabelZoneRegionStable])
assert.Equal(t, "eastus-1", fnh.UpdatedNodes[0].ObjectMeta.Labels[v1.LabelZoneFailureDomainStable])
assert.Equal(t, "node0", fnh.UpdatedNodes[0].Name, "Node was not updated")
assert.Equal(t, 6, len(fnh.UpdatedNodes[0].ObjectMeta.Labels),
assert.Equal(t, 3, len(fnh.UpdatedNodes[0].ObjectMeta.Labels),
"Node label for Region and Zone were not set")
assert.Equal(t, "eastus", fnh.UpdatedNodes[0].ObjectMeta.Labels[v1.LabelZoneRegionStable],
"Node Region not correctly updated")
assert.Equal(t, "eastus-1", fnh.UpdatedNodes[0].ObjectMeta.Labels[v1.LabelZoneFailureDomainStable],
"Node FailureDomain not correctly updated")
assert.Equal(t, "eastus", fnh.UpdatedNodes[0].ObjectMeta.Labels[v1.LabelZoneRegion],
"Node Region not correctly updated")
assert.Equal(t, "eastus-1", fnh.UpdatedNodes[0].ObjectMeta.Labels[v1.LabelZoneFailureDomain],
"Node FailureDomain not correctly updated")
}

// This test checks that a node with the external cloud provider taint is cloudprovider initialized and
Expand Down Expand Up @@ -466,10 +461,7 @@ func TestAddCloudNode(t *testing.T) {
mockNP := mocknodeprovider.NewMockNodeProvider(ctrl)
mockNP.EXPECT().InstanceID(gomock.Any(), types.NodeName("node0")).Return("node0", nil)
mockNP.EXPECT().InstanceType(gomock.Any(), types.NodeName("node0")).Return("Standard_D2_v3", nil)
mockNP.EXPECT().GetZone(gomock.Any(), gomock.Any()).Return(cloudprovider.Zone{
Region: "eastus",
FailureDomain: "eastus-1",
}, nil)

mockNP.EXPECT().NodeAddresses(gomock.Any(), types.NodeName("node0")).Return([]v1.NodeAddress{
{
Type: v1.NodeHostName,
Expand All @@ -484,6 +476,10 @@ func TestAddCloudNode(t *testing.T) {
Address: "132.143.154.163",
},
}, nil).AnyTimes()
mockNP.EXPECT().GetZone(gomock.Any(), gomock.Any()).Return(cloudprovider.Zone{
Region: "eastus",
FailureDomain: "eastus-1",
}, nil)
mockNP.EXPECT().GetPlatformSubFaultDomain().Return("", nil)

factory := informers.NewSharedInformerFactory(fnh, 0)
Expand Down Expand Up @@ -669,121 +665,6 @@ func TestNodeProvidedIPAddresses(t *testing.T) {
assert.Equal(t, "10.0.0.1", updatedNodes[0].Status.Addresses[0].Address, "Node Addresses not correctly updated")
}

func Test_reconcileNodeLabels(t *testing.T) {
testcases := []struct {
name string
labels map[string]string
expectedLabels map[string]string
expectedErr error
}{
{
name: "no labels",
labels: map[string]string{},
expectedLabels: map[string]string{},
expectedErr: nil,
},
{
name: "requires reconcile",
labels: map[string]string{
v1.LabelZoneFailureDomain: "foo",
v1.LabelZoneRegion: "bar",
v1.LabelInstanceType: "the-best-type",
},
expectedLabels: map[string]string{
v1.LabelZoneFailureDomain: "foo",
v1.LabelZoneRegion: "bar",
v1.LabelZoneFailureDomainStable: "foo",
v1.LabelZoneRegionStable: "bar",
v1.LabelInstanceType: "the-best-type",
v1.LabelInstanceTypeStable: "the-best-type",
},
expectedErr: nil,
},
{
name: "doesn't require reconcile",
labels: map[string]string{
v1.LabelZoneFailureDomain: "foo",
v1.LabelZoneRegion: "bar",
v1.LabelZoneFailureDomainStable: "foo",
v1.LabelZoneRegionStable: "bar",
v1.LabelInstanceType: "the-best-type",
v1.LabelInstanceTypeStable: "the-best-type",
},
expectedLabels: map[string]string{
v1.LabelZoneFailureDomain: "foo",
v1.LabelZoneRegion: "bar",
v1.LabelZoneFailureDomainStable: "foo",
v1.LabelZoneRegionStable: "bar",
v1.LabelInstanceType: "the-best-type",
v1.LabelInstanceTypeStable: "the-best-type",
},
expectedErr: nil,
},
{
name: "require reconcile -- secondary labels are different from primary",
labels: map[string]string{
v1.LabelZoneFailureDomain: "foo",
v1.LabelZoneRegion: "bar",
v1.LabelZoneFailureDomainStable: "wrongfoo",
v1.LabelZoneRegionStable: "wrongbar",
v1.LabelInstanceType: "the-best-type",
v1.LabelInstanceTypeStable: "the-wrong-type",
},
expectedLabels: map[string]string{
v1.LabelZoneFailureDomain: "foo",
v1.LabelZoneRegion: "bar",
v1.LabelZoneFailureDomainStable: "foo",
v1.LabelZoneRegionStable: "bar",
v1.LabelInstanceType: "the-best-type",
v1.LabelInstanceTypeStable: "the-best-type",
},
expectedErr: nil,
},
}

for _, test := range testcases {
t.Run(test.name, func(t *testing.T) {
testNode := &v1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: "node01",
Labels: test.labels,
},
}

clientset := fake.NewSimpleClientset(testNode)
factory := informers.NewSharedInformerFactory(clientset, 0)

cnc := &CloudNodeController{
kubeClient: clientset,
nodeInformer: factory.Core().V1().Nodes(),
}

// activate node informer
factory.Core().V1().Nodes().Informer()
factory.Start(nil)
factory.WaitForCacheSync(nil)

err := cnc.reconcileNodeLabels(testNode)
if !errors.Is(err, test.expectedErr) {
t.Logf("actual err: %v", err)
t.Logf("expected err: %v", test.expectedErr)
t.Errorf("unexpected error")
}

actualNode, err := clientset.CoreV1().Nodes().Get(context.TODO(), "node01", metav1.GetOptions{})
if err != nil {
t.Fatalf("error getting updated node: %v", err)
}

if !reflect.DeepEqual(actualNode.Labels, test.expectedLabels) {
t.Logf("actual node labels: %v", actualNode.Labels)
t.Logf("expected node labels: %v", test.expectedLabels)
t.Errorf("updated node did not match expected node")
}
})
}
}

// Tests that node address changes are detected correctly
func TestNodeAddressesChangeDetected(t *testing.T) {
addressSet1 := []v1.NodeAddress{
Expand Down

0 comments on commit 95e8db9

Please sign in to comment.