Skip to content

Commit

Permalink
Fix Subnet update error detection
Browse files Browse the repository at this point in the history 
Signed-off-by: Yanjun Zhou <[email protected]>
  • Loading branch information
@yanjunz97
yanjunz97 committed Jan 7, 2025
1 parent 57d792c commit 06fa16e
Show file tree
Hide file tree
Showing 2 changed files with 120 additions and 28 deletions.
53 changes: 26 additions & 27 deletions pkg/nsx/services/subnet/subnet.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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.
Expand Down Expand Up @@ -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)
Expand Down
95 changes: 94 additions & 1 deletion pkg/nsx/services/subnet/subnet_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package subnet

import (
"context"
"errors"
"reflect"
"testing"

Expand All @@ -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"
Expand All @@ -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"
)

Expand Down Expand Up @@ -398,11 +401,101 @@ 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) {
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)
}
})
}
}

0 comments on commit 06fa16e

Please sign in to comment.