Merge pull request #667 from alexander-demichev/tagsvsphere

Bug 1877281: [vSphere] Don't fail when tag is not found
openshift · Sep 14, 2020 · a682acd · a682acd
2 parents 840f6ed + 6fc674c
commit a682acd
Show file tree

Hide file tree

Showing 2 changed files with 78 additions and 18 deletions.
diff --git a/pkg/controller/vsphere/reconciler.go b/pkg/controller/vsphere/reconciler.go
@@ -892,8 +892,18 @@ func (vm *virtualMachine) reconcileTags(ctx context.Context, session *session.Se
 	return nil
 }
 
-// checkAttachedTag returns true if tag is already attached to a vm
+// checkAttachedTag returns true if tag is already attached to a vm or tag doesn't exist
 func (vm *virtualMachine) checkAttachedTag(ctx context.Context, tagName string, m *tags.Manager) (bool, error) {
+	// cluster ID tag doesn't exists in UPI, we should skip tag attachment if it's not found
+	foundTag, err := vm.foundTag(ctx, tagName, m)
+	if err != nil {
+		return false, err
+	}
+
+	if !foundTag {
+		return true, nil
+	}
+
 	tags, err := m.GetAttachedTags(ctx, vm.Ref)
 	if err != nil {
 		return false, err
@@ -908,6 +918,26 @@ func (vm *virtualMachine) checkAttachedTag(ctx context.Context, tagName string,
 	return false, nil
 }
 
+func (vm *virtualMachine) foundTag(ctx context.Context, tagName string, m *tags.Manager) (bool, error) {
+	tags, err := m.ListTags(ctx)
+	if err != nil {
+		return false, err
+	}
+
+	for _, id := range tags {
+		tag, err := m.GetTag(ctx, id)
+		if err != nil {
+			return false, err
+		}
+
+		if tag.Name == tagName {
+			return true, nil
+		}
+	}
+
+	return false, nil
+}
+
 type NetworkStatus struct {
 	// Connected is a flag that indicates whether this network is currently
 	// connected to the VM.

diff --git a/pkg/controller/vsphere/reconciler_test.go b/pkg/controller/vsphere/reconciler_test.go
@@ -901,21 +901,34 @@ func TestReconcileTags(t *testing.T) {
 	testCases := []struct {
 		name          string
 		expectedError bool
+		attachTag     bool
+		testCondition func()
 	}{
 		{
-			name:          "Fail when tag doesn't exist",
-			expectedError: true,
+			name:          "Don't fail when tag doesn't exist",
+			expectedError: false,
 		},
 		{
 			name:          "Successfully attach a tag",
 			expectedError: false,
+			attachTag:     true,
+			testCondition: func() {
+				createTagAndCategory(session, "CLUSTERID_CATEGORY", tagName)
+			},
+		},
+		{
+			name:          "Fail on vSphere API error",
+			expectedError: false,
+			testCondition: func() {
+				session.Logout(context.Background())
+			},
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			if !tc.expectedError {
-				createTagAndCategory(session, "CLUSTERID_CATEGORY", tagName)
+			if tc.testCondition != nil {
+				tc.testCondition()
 			}
 
 			err := vm.reconcileTags(context.TODO(), session, &machinev1.Machine{
@@ -933,21 +946,24 @@ func TestReconcileTags(t *testing.T) {
 				if err != nil {
 					t.Fatalf("Not expected error %v", err)
 				}
-				if err := session.WithRestClient(context.TODO(), func(c *rest.Client) error {
-					tagsMgr := tags.NewManager(c)
 
-					tags, err := tagsMgr.GetAttachedTags(context.TODO(), managedObjRef)
-					if err != nil {
-						return err
-					}
+				if tc.attachTag {
+					if err := session.WithRestClient(context.TODO(), func(c *rest.Client) error {
+						tagsMgr := tags.NewManager(c)
 
-					if tags[0].Name != tagName {
-						t.Fatalf("Expected tag %s, got %s", tagName, tags[0].Name)
-					}
+						tags, err := tagsMgr.GetAttachedTags(context.TODO(), managedObjRef)
+						if err != nil {
+							return err
+						}
 
-					return nil
-				}); err != nil {
-					t.Fatal(err)
+						if tags[0].Name != tagName {
+							t.Fatalf("Expected tag %s, got %s", tagName, tags[0].Name)
+						}
+
+						return nil
+					}); err != nil {
+						t.Fatal(err)
+					}
 				}
 			}
 
@@ -970,6 +986,7 @@ func TestCheckAttachedTag(t *testing.T) {
 	}
 
 	tagName := "CLUSTERID"
+	nonAttachedTagName := "nonAttachedTag"
 
 	if err := session.WithRestClient(context.TODO(), func(c *rest.Client) error {
 		tagsMgr := tags.NewManager(c)
@@ -995,6 +1012,14 @@ func TestCheckAttachedTag(t *testing.T) {
 			return err
 		}
 
+		_, err = tagsMgr.CreateTag(context.TODO(), &tags.Tag{
+			CategoryID: id,
+			Name:       nonAttachedTagName,
+		})
+		if err != nil {
+			return err
+		}
+
 		return nil
 	}); err != nil {
 		t.Fatal(err)
@@ -1011,8 +1036,13 @@ func TestCheckAttachedTag(t *testing.T) {
 			tagName: tagName,
 		},
 		{
-			name:    "Fail to find a tag",
+			name:    "Return true if a tag doesn't exist",
 			tagName: "non existent tag",
+			findTag: true,
+		},
+		{
+			name:    "Fail to find a tag",
+			tagName: nonAttachedTagName,
 		},
 	}