From 534800e53329ab8eae901ebfd9cd95fa3f9f2fe1 Mon Sep 17 00:00:00 2001 From: Yanjun Zhou Date: Mon, 6 Jan 2025 14:52:57 +0800 Subject: [PATCH] Fix Subnet update error detection Signed-off-by: Yanjun Zhou --- pkg/nsx/services/subnet/subnet.go | 53 +++++---- pkg/nsx/services/subnet/subnet_test.go | 155 ++++++++++++++++++++++++- 2 files changed, 180 insertions(+), 28 deletions(-) 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..1d6fafce1 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" ) @@ -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,7 +426,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 +434,128 @@ func TestSubnetService_UpdateSubnetSet(t *testing.T) { 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) + } + }) + } +}