diff --git a/pkg/nsx/services/subnet/subnet.go b/pkg/nsx/services/subnet/subnet.go index 835d8a7bf..80cb1eee9 100644 --- a/pkg/nsx/services/subnet/subnet.go +++ b/pkg/nsx/services/subnet/subnet.go @@ -90,9 +90,9 @@ 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 { @@ -100,12 +100,15 @@ 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 } } if !changed { @@ -113,10 +116,10 @@ func (service *SubnetService) CreateOrUpdateSubnet(obj client.Object, vpcInfo co 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) diff --git a/pkg/nsx/services/subnet/subnet_test.go b/pkg/nsx/services/subnet/subnet_test.go index 9fc11ce8b..6fb0c12bf 100644 --- a/pkg/nsx/services/subnet/subnet_test.go +++ b/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" ) @@ -398,7 +401,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() @@ -406,3 +409,93 @@ func TestSubnetService_UpdateSubnetSet(t *testing.T) { err := service.UpdateSubnetSet("ns-1", vpcSubnets, tags, "") assert.Nil(t, err) } + +func TestSubnetService_createOrUpdateSubnet(t *testing.T) { + service := &SubnetService{ + Service: common.Service{ + NSXClient: &nsx.Client{ + OrgRootClient: &fakeOrgRootClient{}, + SubnetsClient: &fakeSubnetsClient{}, + }, + }, + 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("subnet-path-1")} + + 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 model.VpcSubnet{Id: common.String("subnet-1"), Path: common.String("subnet-path-1")}, nil + }) + patches.ApplyFunc((*SubnetService).UpdateSubnetSetStatus, + func(_ *SubnetService, obj *v1alpha1.SubnetSet) error { + return nil + }) + return patches + }, + crObj: &v1alpha1.SubnetSet{}, + }, + } + + 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) + } + }) + } +}