Fix storage profile update (#698)

* Fix Issue #648 adding a storage profile requires the vdc to be replaced * Fix Issue #696 Catalog deletion failure returns as success * Bump up version to 3.4.0 * Update CHANGELOG items Signed-off-by: Giuseppe Maxia <[email protected]>
vmware · Aug 6, 2021 · ae590e3 · ae590e3
1 parent 5579bf5
commit ae590e3
Show file tree

Hide file tree

Showing 11 changed files with 392 additions and 189 deletions.
diff --git a/.changes/v3.4.0/698-bug-fixes.md b/.changes/v3.4.0/698-bug-fixes.md
@@ -0,0 +1,2 @@
+* Fix Issue #696: Catalog deletion failure returns as success [GH-698]
+* Fix Issue #648 `vcd_org_vdc`: adding a storage profile requires the vdc to be replaced [GH-698]
diff --git a/.changes/v3.4.0/698-improvements.md b/.changes/v3.4.0/698-improvements.md
@@ -0,0 +1 @@
+* `vcd_org_vdc`: it is now possible adding and removing storage profiles [GH-698]
diff --git a/PREVIOUS_VERSION b/PREVIOUS_VERSION
@@ -1 +1 @@
-v3.3.0
+v3.3.1
diff --git a/VERSION b/VERSION
@@ -1 +1 @@
-v3.3.1
+v3.4.0
diff --git a/go.mod b/go.mod
@@ -7,5 +7,5 @@ require (
 	github.com/hashicorp/go-version v1.2.1
 	github.com/hashicorp/terraform-plugin-sdk/v2 v2.4.4
 	github.com/kr/pretty v0.2.1
-	github.com/vmware/go-vcloud-director/v2 v2.12.1
+	github.com/vmware/go-vcloud-director/v2 v2.13.0-alpha.1
 )
diff --git a/go.sum b/go.sum
@@ -288,8 +288,8 @@ github.com/ulikunitz/xz v0.5.8/go.mod h1:nbz6k7qbPmH4IRqmfOplQw/tblSgqTqBwxkY0oW
 github.com/vmihailenco/msgpack v3.3.3+incompatible/go.mod h1:fy3FlTQTDXWkZ7Bh6AcGMlsjHatGryHQYUTf1ShIgkk=
 github.com/vmihailenco/msgpack v4.0.4+incompatible h1:dSLoQfGFAo3F6OoNhwUmLwVgaUXK79GlxNBwueZn0xI=
 github.com/vmihailenco/msgpack v4.0.4+incompatible/go.mod h1:fy3FlTQTDXWkZ7Bh6AcGMlsjHatGryHQYUTf1ShIgkk=
-github.com/vmware/go-vcloud-director/v2 v2.12.1 h1:PO+dVQyZlECX87oVWgLb4/S2v5hiqKREVGsAwTCJHbQ=
-github.com/vmware/go-vcloud-director/v2 v2.12.1/go.mod h1:poaOwg7CoXO4m9Pv4TVhMpNF1wQQwKzxpdGYTfjzajs=
+github.com/vmware/go-vcloud-director/v2 v2.13.0-alpha.1 h1:NSsURoiIYFv+Ot2uqN2zQjmrm8PzApBjxa8fm8dxkgo=
+github.com/vmware/go-vcloud-director/v2 v2.13.0-alpha.1/go.mod h1:poaOwg7CoXO4m9Pv4TVhMpNF1wQQwKzxpdGYTfjzajs=
 github.com/xanzy/ssh-agent v0.2.1 h1:TCbipTQL2JiiCprBWx9frJ2eJlCYT00NmctrHxVAr70=
 github.com/xanzy/ssh-agent v0.2.1/go.mod h1:mLlQY/MoOhWBj+gOGMQkOeiEvkx+8pJSI+0Bx9h2kr4=
 github.com/yuin/goldmark v1.1.25/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=

diff --git a/vcd/org_vdc_common_test.go b/vcd/org_vdc_common_test.go
diff --git a/vcd/resource_vcd_org_vdc.go b/vcd/resource_vcd_org_vdc.go
@@ -11,6 +11,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
 	"github.com/vmware/go-vcloud-director/v2/govcd"
 	"github.com/vmware/go-vcloud-director/v2/types/v56"
+	"github.com/vmware/go-vcloud-director/v2/util"
 )
 
 func resourceVcdOrgVdc() *schema.Resource {
@@ -113,7 +114,7 @@ func resourceVcdOrgVdc() *schema.Resource {
 			"storage_profile": &schema.Schema{
 				Type:        schema.TypeSet,
 				Required:    true,
-				ForceNew:    true,
+				ForceNew:    false,
 				MinItems:    1,
 				Description: "Storage profiles supported by this VDC.",
 				Elem: &schema.Resource{
@@ -456,7 +457,7 @@ func getComputeStorageProfiles(vcdClient *VCDClient, profile *types.VdcStoragePr
 	root := make([]map[string]interface{}, 0)
 
 	for _, vdcStorageProfile := range profile.VdcStorageProfile {
-		vdcStorageProfileDetails, err := govcd.GetStorageProfileByHref(vcdClient.VCDClient, vdcStorageProfile.HREF)
+		vdcStorageProfileDetails, err := vcdClient.GetStorageProfileByHref(vdcStorageProfile.HREF)
 		if err != nil {
 			return nil, err
 		}
@@ -571,41 +572,177 @@ func resourceVcdVdcUpdate(d *schema.ResourceData, meta interface{}) error {
 
 	if d.HasChange("storage_profile") {
 		vdcStorageProfilesConfigurations := d.Get("storage_profile").(*schema.Set)
-		for _, storageConfigurationValues := range vdcStorageProfilesConfigurations.List() {
-			storageConfiguration := storageConfigurationValues.(map[string]interface{})
-			var matchedStorageProfile types.Reference
-			for _, vdcStorageProfile := range adminVdc.AdminVdc.VdcStorageProfiles.VdcStorageProfile {
-				if storageConfiguration["name"].(string) == vdcStorageProfile.Name {
-					matchedStorageProfile = *vdcStorageProfile
+		err = updateStorageProfiles(vdcStorageProfilesConfigurations, vcdClient, adminVdc, d.Get("provider_vdc_name").(string))
+		if err != nil {
+			return fmt.Errorf("[VDC update] error updating storage profiles: %s", err)
+		}
+	}
+	log.Printf("[TRACE] VDC update completed: %s", adminVdc.AdminVdc.Name)
+	return resourceVcdVdcRead(d, meta)
+}
+
+func updateStorageProfileDetails(vcdClient *VCDClient, adminVdc *govcd.AdminVdc, storageProfile *types.Reference, storageConfiguration map[string]interface{}) error {
+	util.Logger.Printf("updating storage profile %#v", storageProfile)
+	uuid, err := govcd.GetUuidFromHref(storageProfile.HREF, true)
+	if err != nil {
+		return fmt.Errorf("error parsing VDC storage profile ID : %s", err)
+	}
+	vdcStorageProfileDetails, err := vcdClient.GetStorageProfileByHref(storageProfile.HREF)
+	if err != nil {
+		return fmt.Errorf("error getting VDC storage profile: %s", err)
+	}
+	_, err = adminVdc.UpdateStorageProfile(uuid, &types.AdminVdcStorageProfile{
+		Name:         storageConfiguration["name"].(string),
+		IopsSettings: nil,
+		Units:        "MB", // only this value is supported
+		Limit:        int64(storageConfiguration["limit"].(int)),
+		Default:      storageConfiguration["default"].(bool),
+		Enabled:      takeBoolPointer(storageConfiguration["enabled"].(bool)),
+		ProviderVdcStorageProfile: &types.Reference{
+			HREF: vdcStorageProfileDetails.ProviderVdcStorageProfile.HREF,
+		},
+	})
+	if err != nil {
+		return fmt.Errorf("error updating VDC storage profile '%s': %s", storageConfiguration["name"].(string), err)
+	}
+	return nil
+}
+
+func updateStorageProfiles(set *schema.Set, client *VCDClient, adminVdc *govcd.AdminVdc, providerVdcName string) error {
+
+	type storageProfileCombo struct {
+		configuration map[string]interface{}
+		reference     *types.Reference
+	}
+	var (
+		existingStorageProfiles []storageProfileCombo
+		newStorageProfiles      []storageProfileCombo
+		removeStorageProfiles   []storageProfileCombo
+		defaultSp               = make(map[string]bool)
+	)
+
+	var isDefaultStorageProfileNew = false
+	// 1. find existing storage profiles: SP are both in the definition and in the VDC
+	for _, storageConfigurationValues := range set.List() {
+		storageConfiguration := storageConfigurationValues.(map[string]interface{})
+		for _, vdcStorageProfile := range adminVdc.AdminVdc.VdcStorageProfiles.VdcStorageProfile {
+			if storageConfiguration["name"].(string) == vdcStorageProfile.Name {
+				if storageConfiguration["default"].(bool) {
+					defaultSp[storageConfiguration["name"].(string)] = true
 				}
+				existingStorageProfiles = append(existingStorageProfiles, storageProfileCombo{
+					configuration: storageConfiguration,
+					reference:     vdcStorageProfile,
+				})
 			}
-			uuid, err := govcd.GetUuidFromHref(matchedStorageProfile.HREF, true)
-			if err != nil {
-				return fmt.Errorf("error parsing VDC storage profile ID : %s", err)
+		}
+	}
+
+	// 2. find new storage profiles: SP are in the definition, but not in the VDC
+	for _, storageConfigurationValues := range set.List() {
+		storageConfiguration := storageConfigurationValues.(map[string]interface{})
+		found := false
+		for _, vdcStorageProfile := range adminVdc.AdminVdc.VdcStorageProfiles.VdcStorageProfile {
+			if storageConfiguration["name"].(string) == vdcStorageProfile.Name {
+				found = true
 			}
-			vdcStorageProfileDetails, err := govcd.GetStorageProfileByHref(vcdClient.VCDClient, matchedStorageProfile.HREF)
-			if err != nil {
-				return fmt.Errorf("error getting VDC storage profile: %s", err)
+		}
+		if !found {
+			if storageConfiguration["default"].(bool) {
+				defaultSp[storageConfiguration["name"].(string)] = true
+				isDefaultStorageProfileNew = true
 			}
-			_, err = changedAdminVdc.UpdateStorageProfile(uuid, &types.AdminVdcStorageProfile{
-				Name:         storageConfiguration["name"].(string),
-				IopsSettings: nil,
-				Units:        "MB", // only this value is supported
-				Limit:        int64(storageConfiguration["limit"].(int)),
-				Default:      storageConfiguration["default"].(bool),
-				Enabled:      takeBoolPointer(storageConfiguration["enabled"].(bool)),
-				ProviderVdcStorageProfile: &types.Reference{
-					HREF: vdcStorageProfileDetails.ProviderVdcStorageProfile.HREF,
-				},
+			newStorageProfiles = append(newStorageProfiles, storageProfileCombo{
+				configuration: storageConfiguration,
+				reference:     nil,
 			})
-			if err != nil {
-				return fmt.Errorf("error updating VDC storage profile: %s", err)
+		}
+	}
+
+	// 3 find removed storage profiles: SP are in the VDC, but not in the definition
+	for _, vdcStorageProfile := range adminVdc.AdminVdc.VdcStorageProfiles.VdcStorageProfile {
+		found := false
+		for _, storageConfigurationValues := range set.List() {
+			storageConfiguration := storageConfigurationValues.(map[string]interface{})
+			if storageConfiguration["name"].(string) == vdcStorageProfile.Name {
+				found = true
+			}
+		}
+		if !found {
+			_, isDefault := defaultSp[vdcStorageProfile.Name]
+			if isDefault {
+				delete(defaultSp, vdcStorageProfile.Name)
 			}
+			removeStorageProfiles = append(removeStorageProfiles, storageProfileCombo{
+				configuration: nil,
+				reference:     vdcStorageProfile,
+			})
 		}
 	}
 
-	log.Printf("[TRACE] VDC update completed: %s", adminVdc.AdminVdc.Name)
-	return resourceVcdVdcRead(d, meta)
+	// 4. Check that there is one and only one default element
+	if len(defaultSp) == 0 {
+		return fmt.Errorf("updateStorageProfiles] no default storage profile left after update")
+	}
+	if len(defaultSp) > 1 {
+		defaultItems := ""
+		for d := range defaultSp {
+			defaultItems += " " + d
+		}
+		return fmt.Errorf("updateStorageProfiles] more than one default storage profile defined [%s]", defaultItems)
+	}
+
+	// 5. Set the default storage profile early
+	if !isDefaultStorageProfileNew {
+		defaultSpName := ""
+		for name := range defaultSp {
+			defaultSpName = name
+			break
+		}
+		err := adminVdc.SetDefaultStorageProfile(defaultSpName)
+		if err != nil {
+			return fmt.Errorf("[updateStorageProfiles] error setting default storage profile '%s': %s", defaultSpName, err)
+		}
+	}
+
+	// 6. Add new storage profiles
+	for _, spCombo := range newStorageProfiles {
+		storageProfile, err := client.QueryProviderVdcStorageProfileByName(spCombo.configuration["name"].(string), adminVdc.AdminVdc.ProviderVdcReference.HREF)
+		if err != nil {
+			return fmt.Errorf("[updateStorageProfiles] error retrieving storage profile '%s' from provider VDC '%s': %s", spCombo.configuration["name"].(string), providerVdcName, err)
+		}
+		err = adminVdc.AddStorageProfileWait(&types.VdcStorageProfileConfiguration{
+			Enabled: spCombo.configuration["enabled"].(bool),
+			Units:   "MB",
+			Limit:   int64(spCombo.configuration["limit"].(int)),
+			Default: spCombo.configuration["default"].(bool),
+			ProviderVdcStorageProfile: &types.Reference{
+				HREF: storageProfile.HREF,
+				Name: storageProfile.Name,
+			},
+		}, "")
+		if err != nil {
+			return fmt.Errorf("[updateStorageProfiles] error adding new storage profile: %s", err)
+		}
+	}
+
+	// 7. Update existing storage profiles
+	for _, spCombo := range existingStorageProfiles {
+		err := updateStorageProfileDetails(client, adminVdc, spCombo.reference, spCombo.configuration)
+		if err != nil {
+			return fmt.Errorf("[updateStorageProfiles] error updating storage profile '%s': %s", spCombo.reference.Name, err)
+		}
+	}
+
+	// 8. Delete unwanted storage profiles
+	for _, spCombo := range removeStorageProfiles {
+		err := adminVdc.RemoveStorageProfileWait(spCombo.reference.Name)
+		if err != nil {
+			return fmt.Errorf("[updateStorageProfiles] error removing storage profile %s: %s", spCombo.reference.Name, err)
+		}
+	}
+
+	return nil
 }
 
 // Deletes a VDC, optionally removing all objects in it as well
@@ -1051,9 +1188,9 @@ func getVcdVdcInput(d *schema.ResourceData, vcdClient *VCDClient) (*types.VdcCon
 	for _, storageConfigurationValues := range vdcStorageProfilesConfigurations.List() {
 		storageConfiguration := storageConfigurationValues.(map[string]interface{})
 
-		href, err := getStorageProfileHREF(vcdClient, storageConfiguration["name"].(string))
+		sp, err := vcdClient.QueryProviderVdcStorageProfileByName(storageConfiguration["name"].(string), providerVdcResults[0].HREF)
 		if err != nil {
-			return &types.VdcConfiguration{}, err
+			return &types.VdcConfiguration{}, fmt.Errorf("[getVcdVdcInput] error retrieving storage profile '%s' from provider VDC '%s': %s", storageConfiguration["name"].(string), providerVdcResults[0].Name, err)
 		}
 
 		vdcStorageProfile := &types.VdcStorageProfileConfiguration{
@@ -1062,7 +1199,7 @@ func getVcdVdcInput(d *schema.ResourceData, vcdClient *VCDClient) (*types.VdcCon
 			Default: storageConfiguration["default"].(bool),
 			Enabled: storageConfiguration["enabled"].(bool),
 			ProviderVdcStorageProfile: &types.Reference{
-				HREF: href,
+				HREF: sp.HREF,
 			},
 		}
 		vdcStorageProfiles = append(vdcStorageProfiles, vdcStorageProfile)
@@ -1148,24 +1285,6 @@ func getVcdVdcInput(d *schema.ResourceData, vcdClient *VCDClient) (*types.VdcCon
 	return params, nil
 }
 
-func getStorageProfileHREF(vcdClient *VCDClient, name string) (string, error) {
-	storageProfileRecords, err := govcd.QueryProviderVdcStorageProfileByName(vcdClient.VCDClient, name)
-	if err != nil {
-		return "", err
-	}
-	if len(storageProfileRecords) == 0 {
-		return "", fmt.Errorf("no provider VDC storage profile found with name %s", name)
-	}
-
-	// additional filtering done cause name like `*` returns more value and have to be manually selected
-	for _, profileRecord := range storageProfileRecords {
-		if profileRecord.Name == name {
-			return profileRecord.HREF, nil
-		}
-	}
-	return "", fmt.Errorf("no provider VDC storage profile found with name %s", name)
-}
-
 // resourceVcdOrgVdcImport is responsible for importing the resource.
 // The following steps happen as part of import
 // 1. The user supplies `terraform import _resource_name_ _the_id_string_` command

diff --git a/vcd/resource_vcd_org_vdc_nsxt_test.go b/vcd/resource_vcd_org_vdc_nsxt_test.go
@@ -14,19 +14,20 @@ func TestAccVcdOrgVdcNsxt(t *testing.T) {
 	allocationModel := "ReservationPool"
 
 	var params = StringMap{
-		"VdcName":                   t.Name(),
-		"OrgName":                   testConfig.VCD.Org,
-		"AllocationModel":           "ReservationPool",
-		"ProviderVdc":               testConfig.VCD.NsxtProviderVdc.Name,
-		"NetworkPool":               testConfig.VCD.NsxtProviderVdc.NetworkPool,
-		"Allocated":                 "1024",
-		"Reserved":                  "1024",
-		"Limit":                     "1024",
-		"LimitIncreased":            "1100",
-		"AllocatedIncreased":        "1100",
-		"ProviderVdcStorageProfile": testConfig.VCD.NsxtProviderVdc.StorageProfile,
-		"Tags":                      "vdc nsxt",
-		"FuncName":                  t.Name(),
+		"VdcName":                    t.Name(),
+		"OrgName":                    testConfig.VCD.Org,
+		"AllocationModel":            "ReservationPool",
+		"ProviderVdc":                testConfig.VCD.NsxtProviderVdc.Name,
+		"NetworkPool":                testConfig.VCD.NsxtProviderVdc.NetworkPool,
+		"Allocated":                  "1024",
+		"Reserved":                   "1024",
+		"Limit":                      "1024",
+		"LimitIncreased":             "1100",
+		"AllocatedIncreased":         "1100",
+		"ProviderVdcStorageProfile":  testConfig.VCD.NsxtProviderVdc.StorageProfile,
+		"ProviderVdcStorageProfile2": "", // The configuration file is missing the second storage profile for NSX-T.
+		"Tags":                       "vdc nsxt",
+		"FuncName":                   t.Name(),
 		// cause vDC ignores empty values and use default
 		"MemoryGuaranteed": "1",
 		"CpuGuaranteed":    "1",
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		* Fix Issue #696: Catalog deletion failure returns as success [GH-698]
		* Fix Issue #648 `vcd_org_vdc`: adding a storage profile requires the vdc to be replaced [GH-698]
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		* `vcd_org_vdc`: it is now possible adding and removing storage profiles [GH-698]