Fix Subnet update error detection #991
Conversation
yanjunz97
force-pushed
the
fix-subnet-update
branch
from
3f70328 to
1942e20
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #991 +/- ##
==========================================
+ Coverage 74.10% 74.19% +0.09%
==========================================
Files 118 118
Lines 16371 16373 +2
==========================================
+ Hits 12131 12148 +17
+ Misses 3466 3454 -12
+ Partials 774 771 -3
|
pkg/nsx/services/subnet/subnet.go
Outdated
| } | ||
| } | ||
| 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) |
There was a problem hiding this comment.
it is a little confusing to have both changed and isUpdate, maybe just use changed & !existingSubnet to pass to the parameter in createOrUpdateSubnet
| @@ -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) { | |||
There was a problem hiding this comment.
Do you think we need a new UT for createOrUpdateSubnet?
There was a problem hiding this comment.
Yes, some part of the code in createOrUpdateSubnet is not covered yet. Added the unit test, thanks
pkg/nsx/services/subnet/subnet.go
Outdated
| // For update case, wait for some time to make sure the realized state is updated. | ||
| if isUpdate { | ||
| time.Sleep(1 * time.Second) | ||
| } |
There was a problem hiding this comment.
it's better that NSX fix that issue. After NSX patch API called, the realized status should be updated in sync mode instead
There was a problem hiding this comment.
Opened a bug for NSX team, let's see how they reply on this observations, thanks
yanjunz97
force-pushed
the
fix-subnet-update
branch
4 times, most recently
from
ecf2948 to
eca3d76
Compare
yanjunz97
force-pushed
the
fix-subnet-update
branch
from
eca3d76 to
c8afa19
Compare
Signed-off-by: Yanjun Zhou <[email protected]>
yanjunz97
force-pushed
the
fix-subnet-update
branch
from
c8afa19 to
61f5514
Compare
Operator modifies the existing Subnet in store before updating the NSX Subnet,
which results in the next reconcile will find the updated Subnet already in store,
skip the update, and overwrite the Status as Ready.
This PR uses a copy when updating the Subnet to avoid this issue.