Fix Subnet update error detection

Signed-off-by: Yanjun Zhou <[email protected]>
vmware-tanzu · Jan 6, 2025 · 1942e20 · 1942e20
1 parent 57d792c
commit 1942e20
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 17 deletions.
diff --git a/pkg/nsx/services/subnet/subnet.go b/pkg/nsx/services/subnet/subnet.go
@@ -89,6 +89,7 @@ func (service *SubnetService) CreateOrUpdateSubnet(obj client.Object, vpcInfo co
 		log.Error(err, "Failed to build Subnet")
 		return nil, err
 	}
+	isUpdate := false
 	// Only check whether it needs update when obj is v1alpha1.Subnet
 	if subnet, ok := obj.(*v1alpha1.Subnet); ok {
 		existingSubnet := service.SubnetStore.GetByKey(service.BuildSubnetID(subnet))
@@ -100,23 +101,27 @@ func (service *SubnetService) CreateOrUpdateSubnet(obj client.Object, vpcInfo co
 			if changed {
 				// Only tags and dhcp are expected to be updated
 				// inherit other fields from the existing Subnet
-				existingSubnet.Tags = nsxSubnet.Tags
-				if existingSubnet.SubnetDhcpConfig != nil {
-					existingSubnet.SubnetDhcpConfig = nsxSubnet.SubnetDhcpConfig
-					existingSubnet.AdvancedConfig.StaticIpAllocation.Enabled = nsxSubnet.AdvancedConfig.StaticIpAllocation.Enabled
+				// Avoid modification on existingSubnet to ensure
+				// Subnet store is only updated after the updating succeeds.
+				updatedSubnet := *existingSubnet
+				updatedSubnet.Tags = nsxSubnet.Tags
+				if updatedSubnet.SubnetDhcpConfig != nil {
+					updatedSubnet.SubnetDhcpConfig = nsxSubnet.SubnetDhcpConfig
+					updatedSubnet.AdvancedConfig.StaticIpAllocation.Enabled = nsxSubnet.AdvancedConfig.StaticIpAllocation.Enabled
 				}
-				nsxSubnet = existingSubnet
+				nsxSubnet = &updatedSubnet
+				isUpdate = true
 			}
 		}
 		if !changed {
 			log.Info("Subnet not changed, skip updating", "SubnetId", uid)
 			return existingSubnet, nil
 		}
 	}
-	return service.createOrUpdateSubnet(obj, nsxSubnet, &vpcInfo)
+	return service.createOrUpdateSubnet(obj, nsxSubnet, &vpcInfo, isUpdate)
 }
 
-func (service *SubnetService) createOrUpdateSubnet(obj client.Object, nsxSubnet *model.VpcSubnet, vpcInfo *common.VPCResourceInfo) (*model.VpcSubnet, error) {
+func (service *SubnetService) createOrUpdateSubnet(obj client.Object, nsxSubnet *model.VpcSubnet, vpcInfo *common.VPCResourceInfo, isUpdate bool) (*model.VpcSubnet, error) {
 	orgRoot, err := service.WrapHierarchySubnet(nsxSubnet, vpcInfo)
 	if err != nil {
 		log.Error(err, "Failed to WrapHierarchySubnet")
@@ -138,6 +143,11 @@ func (service *SubnetService) createOrUpdateSubnet(obj client.Object, nsxSubnet
 		Jitter:   0,
 		Steps:    6,
 	}
+
+	// For update case, wait for some time to make sure the realized state is updated.
+	if isUpdate {
+		time.Sleep(1 * time.Second)
+	}
 	// Failure of CheckRealizeState may result in the creation of an existing Subnet.
 	// For Subnets, it's important to reuse the already created NSXSubnet.
 	// For SubnetSets, since the ID includes a random value, the created NSX Subnet needs to be deleted and recreated.
@@ -428,36 +438,39 @@ func (service *SubnetService) UpdateSubnetSet(ns string, vpcSubnets []*model.Vpc
 		}
 		newTags := append(service.buildBasicTags(subnetSet), tags...)
 
-		var updatedSubnet *model.VpcSubnet
+		var comparableSubnet *model.VpcSubnet
 		if vpcSubnets[i].SubnetDhcpConfig == nil {
-			updatedSubnet = &model.VpcSubnet{
+			comparableSubnet = &model.VpcSubnet{
 				Tags: newTags,
 			}
 		} else {
-			updatedSubnet = &model.VpcSubnet{
+			comparableSubnet = &model.VpcSubnet{
 				Tags:             newTags,
 				SubnetDhcpConfig: service.buildSubnetDHCPConfig(dhcpMode),
 			}
 		}
-		changed := common.CompareResource(SubnetToComparable(vpcSubnets[i]), SubnetToComparable(updatedSubnet))
+		changed := common.CompareResource(SubnetToComparable(vpcSubnets[i]), SubnetToComparable(comparableSubnet))
 		if !changed {
 			log.Info("NSX Subnet unchanged, skipping update", "Subnet", *vpcSubnet.Id)
 			continue
 		}
 
-		vpcSubnets[i].Tags = newTags
+		// Avoid updating vpcSubnets[i] to ensure Subnet store
+		// is only updated after the updating succeeds.
+		updatedSubnet := *vpcSubnets[i]
+		updatedSubnet.Tags = newTags
 		// Update the SubnetSet DHCP Config
-		if vpcSubnets[i].SubnetDhcpConfig != nil {
-			vpcSubnets[i].SubnetDhcpConfig = service.buildSubnetDHCPConfig(dhcpMode)
-			vpcSubnets[i].AdvancedConfig.StaticIpAllocation.Enabled = &staticIpAllocation
+		if updatedSubnet.SubnetDhcpConfig != nil {
+			updatedSubnet.SubnetDhcpConfig = service.buildSubnetDHCPConfig(dhcpMode)
+			updatedSubnet.AdvancedConfig.StaticIpAllocation.Enabled = &staticIpAllocation
 		}
 
 		vpcInfo, err := common.ParseVPCResourcePath(*vpcSubnets[i].Path)
 		if err != nil {
 			err := fmt.Errorf("failed to parse NSX VPC path for Subnet %s: %s", *vpcSubnets[i].Path, err)
 			return err
 		}
-		if _, err := service.createOrUpdateSubnet(subnetSet, vpcSubnets[i], &vpcInfo); err != nil {
+		if _, err := service.createOrUpdateSubnet(subnetSet, &updatedSubnet, &vpcInfo, true); err != nil {
 			return fmt.Errorf("failed to update Subnet %s in SubnetSet %s: %w", *vpcSubnet.Id, subnetSet.Name, err)
 		}
 		log.Info("Successfully updated SubnetSet", "subnetSet", subnetSet, "Subnet", *vpcSubnet.Id)

diff --git a/pkg/nsx/services/subnet/subnet_test.go b/pkg/nsx/services/subnet/subnet_test.go
@@ -398,7 +398,7 @@ func TestSubnetService_UpdateSubnetSet(t *testing.T) {
 	})
 
 	patchesCreateOrUpdateSubnet := gomonkey.ApplyFunc((*SubnetService).createOrUpdateSubnet,
-		func(r *SubnetService, obj client.Object, nsxSubnet *model.VpcSubnet, vpcInfo *common.VPCResourceInfo) (*model.VpcSubnet, error) {
+		func(r *SubnetService, obj client.Object, nsxSubnet *model.VpcSubnet, vpcInfo *common.VPCResourceInfo, isUpdate bool) (*model.VpcSubnet, error) {
 			return &model.VpcSubnet{Path: &fakeSubnetPath}, nil
 		})
 	defer patchesCreateOrUpdateSubnet.Reset()