Fix Subnet update error detection

Signed-off-by: Yanjun Zhou <[email protected]>

pkg/nsx/services/subnet/subnet.go

@@ -90,33 +90,36 @@ func (service *SubnetService) CreateOrUpdateSubnet(obj client.Object, vpcInfo co
 		return nil, err
 	}
 	// Only check whether it needs update when obj is v1alpha1.Subnet
+	var changed, existingSubnet bool
 	if subnet, ok := obj.(*v1alpha1.Subnet); ok {
 		existingSubnet := service.SubnetStore.GetByKey(service.BuildSubnetID(subnet))
-		changed := false
 		if existingSubnet == nil {
 			changed = true
 		} else {
 			changed = common.CompareResource(SubnetToComparable(existingSubnet), SubnetToComparable(nsxSubnet))
 			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
 			}
 		}
 		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, (changed && !existingSubnet))
 }
 
-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 +141,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 +436,27 @@ func (service *SubnetService) UpdateSubnetSet(ns string, vpcSubnets []*model.Vpc
 		}
 		newTags := append(service.buildBasicTags(subnetSet), tags...)
 
-		var updatedSubnet *model.VpcSubnet
-		if vpcSubnets[i].SubnetDhcpConfig == nil {
-			updatedSubnet = &model.VpcSubnet{
-				Tags: newTags,
-			}
-		} else {
-			updatedSubnet = &model.VpcSubnet{
-				Tags:             newTags,
-				SubnetDhcpConfig: service.buildSubnetDHCPConfig(dhcpMode),
-			}
+		// 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 updatedSubnet.SubnetDhcpConfig != nil {
+			updatedSubnet.SubnetDhcpConfig = service.buildSubnetDHCPConfig(dhcpMode)
+			updatedSubnet.AdvancedConfig.StaticIpAllocation.Enabled = &staticIpAllocation
 		}
-		changed := common.CompareResource(SubnetToComparable(vpcSubnets[i]), SubnetToComparable(updatedSubnet))
+		changed := common.CompareResource(SubnetToComparable(vpcSubnets[i]), SubnetToComparable(&updatedSubnet))
 		if !changed {
 			log.Info("NSX Subnet unchanged, skipping update", "Subnet", *vpcSubnet.Id)
 			continue
 		}
 
-		vpcSubnets[i].Tags = newTags
-		// Update the SubnetSet DHCP Config
-		if vpcSubnets[i].SubnetDhcpConfig != nil {
-			vpcSubnets[i].SubnetDhcpConfig = service.buildSubnetDHCPConfig(dhcpMode)
-			vpcSubnets[i].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)

pkg/nsx/services/subnet/subnet_test.go

@@ -2,6 +2,7 @@ package subnet
 
 import (
 	"context"
+	"errors"
 	"reflect"
 	"testing"
 
@@ -15,6 +16,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/apimachinery/pkg/util/wait"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/tools/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -25,6 +27,7 @@ import (
 	mock_client "github.com/vmware-tanzu/nsx-operator/pkg/mock/controller-runtime/client"
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx"
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common"
+	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/realizestate"
 	"github.com/vmware-tanzu/nsx-operator/pkg/util"
 )
 
@@ -135,6 +138,13 @@ func (f fakeSubnetsClient) Update(orgIdParam string, projectIdParam string, vpcI
 	return model.VpcSubnet{}, nil
 }
 
+type fakeSubnetStatusClient struct {
+}
+
+func (f fakeSubnetStatusClient) List(orgIdParam string, projectIdParam string, vpcIdParam string, subnetIdParam string) (model.VpcSubnetStatusListResult, error) {
+	return model.VpcSubnetStatusListResult{}, nil
+}
+
 type fakeRealizedEntitiesClient struct {
 }
 
@@ -151,6 +161,24 @@ func (f fakeRealizedEntitiesClient) List(intentPathParam string, sitePathParam *
 	}, nil
 }
 
+type fakeStatusWriter struct {
+	t           *testing.T
+	validateObj bool
+	expectObj   client.Object
+}
+
+func (writer fakeStatusWriter) Create(ctx context.Context, obj client.Object, subResource client.Object, opts ...client.SubResourceCreateOption) error {
+	return nil
+}
+
+func (writer fakeStatusWriter) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error {
+	return nil
+}
+
+func (writer fakeStatusWriter) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error {
+	return nil
+}
+
 func TestInitializeSubnetService(t *testing.T) {
 	clusterName := "k8scl-one:test"
 	subnetID := "fakeSubnetUID"
@@ -398,11 +426,136 @@ 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()
 
 	err := service.UpdateSubnetSet("ns-1", vpcSubnets, tags, "")
 	assert.Nil(t, err)
 }
+
+func TestSubnetService_createOrUpdateSubnet(t *testing.T) {
+	mockCtl := gomock.NewController(t)
+	k8sClient := mock_client.NewMockClient(mockCtl)
+	defer mockCtl.Finish()
+	service := &SubnetService{
+		Service: common.Service{
+			Client: k8sClient,
+			NSXClient: &nsx.Client{
+				OrgRootClient:      &fakeOrgRootClient{},
+				SubnetsClient:      &fakeSubnetsClient{},
+				SubnetStatusClient: &fakeSubnetStatusClient{},
+			},
+		},
+		SubnetStore: &SubnetStore{
+			ResourceStore: common.ResourceStore{
+				Indexer: cache.NewIndexer(keyFunc, cache.Indexers{
+					common.TagScopeSubnetCRUID:    subnetIndexFunc,
+					common.TagScopeSubnetSetCRUID: subnetSetIndexFunc,
+					common.TagScopeVMNamespace:    subnetIndexVMNamespaceFunc,
+					common.TagScopeNamespace:      subnetIndexNamespaceFunc,
+				}),
+				BindingType: model.VpcSubnetBindingType(),
+			},
+		},
+	}
+
+	fakeSubnet := model.VpcSubnet{
+		Id:   common.String("subnet-1"),
+		Path: common.String("/orgs/default/projects/default/vpcs/default/subnets/subnet-path-1"),
+		Tags: []model.Tag{
+			{
+				Scope: common.String(common.TagScopeSubnetSetCRUID),
+				Tag:   common.String("subnetset-1"),
+			},
+		},
+	}
+	fakewriter := &fakeStatusWriter{}
+
+	testCases := []struct {
+		name        string
+		prepareFunc func() *gomonkey.Patches
+		expectedErr string
+		crObj       client.Object
+	}{
+		{
+			name: "Update Subnet with RealizedState and deletion error",
+			prepareFunc: func() *gomonkey.Patches {
+				patches := gomonkey.ApplyFunc((*realizestate.RealizeStateService).CheckRealizeState,
+					func(_ *realizestate.RealizeStateService, _ wait.Backoff, _ string) error {
+						return realizestate.NewRealizeStateError("mocked realized error")
+					})
+				patches.ApplyFunc((*SubnetService).DeleteSubnet, func(_ *SubnetService, _ model.VpcSubnet) error {
+					return errors.New("mocked deletion error")
+				})
+				patches.ApplyFunc(fakeSubnetsClient.Get, func(f fakeSubnetsClient, orgIdParam string, projectIdParam string, vpcIdParam string, subnetIdParam string) (model.VpcSubnet, error) {
+					return fakeSubnet, nil
+				})
+				return patches
+			},
+			crObj:       &v1alpha1.Subnet{},
+			expectedErr: "realization check failed: mocked realized error; deletion failed: mocked deletion error",
+		},
+		{
+			name: "Create Subnet for SubnetSet Success",
+			prepareFunc: func() *gomonkey.Patches {
+				patches := gomonkey.ApplyFunc((*realizestate.RealizeStateService).CheckRealizeState,
+					func(_ *realizestate.RealizeStateService, _ wait.Backoff, _ string) error {
+						return nil
+					})
+				patches.ApplyFunc(fakeSubnetsClient.Get,
+					func(f fakeSubnetsClient, orgIdParam string, projectIdParam string, vpcIdParam string, subnetIdParam string) (model.VpcSubnet, error) {
+						return fakeSubnet, nil
+					})
+				patches.ApplyFunc(fakeSubnetStatusClient.List,
+					func(_ fakeSubnetStatusClient, orgIdParam string, projectIdParam string, vpcIdParam string, subnetIdParam string) (model.VpcSubnetStatusListResult, error) {
+						return model.VpcSubnetStatusListResult{
+							Results: []model.VpcSubnetStatus{
+								{
+									NetworkAddress:    common.String("10.0.0.0/28"),
+									GatewayAddress:    common.String("10.0.0.1/28"),
+									DhcpServerAddress: common.String("10.0.0.2/28"),
+								},
+							},
+						}, nil
+					})
+				k8sClient.EXPECT().Status().Return(fakewriter)
+				patches.ApplyFunc(fakeStatusWriter.Update,
+					func(writer fakeStatusWriter, ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error {
+						subnetSet := obj.(*v1alpha1.SubnetSet)
+						assert.Equal(t, 1, len(subnetSet.Status.Subnets))
+						assert.Equal(t, "10.0.0.0/28", subnetSet.Status.Subnets[0].NetworkAddresses[0])
+						assert.Equal(t, "10.0.0.1/28", subnetSet.Status.Subnets[0].GatewayAddresses[0])
+						assert.Equal(t, "10.0.0.2/28", subnetSet.Status.Subnets[0].DHCPServerAddresses[0])
+						return nil
+					})
+				return patches
+			},
+			crObj: &v1alpha1.SubnetSet{
+				ObjectMeta: metav1.ObjectMeta{UID: "subnetset-1"},
+			},
+		},
+	}
+
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.prepareFunc != nil {
+				patches := tt.prepareFunc()
+				defer patches.Reset()
+			}
+			nsxSubnet, err := service.createOrUpdateSubnet(
+				tt.crObj,
+				&fakeSubnet,
+				&common.VPCResourceInfo{},
+				true,
+			)
+			if tt.expectedErr != "" {
+				assert.Equal(t, tt.expectedErr, err.Error())
+			} else {
+				assert.Nil(t, err)
+				assert.Equal(t, fakeSubnet, *nsxSubnet)
+			}
+		})
+	}
+}