Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Subnet lock for SubnetPort creation #974

Merged
merged 1 commit into from
Dec 27, 2024

Conversation

yanjunz97
Copy link
Contributor

Instead of using lock for Subnet, this PR adds a cache in SubnetPort service
to record the number of SubnetPorts on each Subnet.
Read/Write operation for this cache is protected by lock to avoid race condition
and ensure the number of SubnetPort on the Subnet will be controlled below
the Subnet capacity.

Testing done:

  • Create 16 Pods on default SubnetSet with size 16 (capacity 12), got all Pods ready;
    delete all Pods and observe the Subnets under the SubnetSet will be deleted by GC.
  • Create 16 VMs on DHCPServer SubnetSet with size 16 (capacity 12), all Vms can get IP;
    delete all VMs and observe the Subnet under the SubnetSet will be deleted by GC.
  • Create 16 VMs on DHCPServer Subnet with size 16 (capacity 12), only 12 VMs can PowerOn
    and get IP. The remainings will have VirtualMachineNetworkReady as NotReady and
    Message like network interface "eth0" error: subnetPort is not ready: SubnetPortNotReady - error occurred while processing the SubnetPort CR. Error: no valid IP in Subnet /orgs/default/projects/project-quality/vpcs/ns-1_7fcad11c-da38-4137-96ec-f0f80ae8d96b/subnets/subnet-test-1_60ebfa36-bbe2-4e6e-85df-8228be4e190a

@yanjunz97 yanjunz97 force-pushed the refactor-subnet-lock branch 3 times, most recently from dc0b2c3 to f78aabf Compare December 23, 2024 08:20
@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 28 lines in your changes missing coverage. Please review.

Project coverage is 73.29%. Comparing base (86c0d65) to head (8494fed).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
pkg/nsx/services/subnet/subnet.go 37.50% 9 Missing and 1 partial ⚠️
...kg/controllers/subnetport/subnetport_controller.go 41.66% 5 Missing and 2 partials ⚠️
pkg/nsx/services/subnetport/subnetport.go 91.66% 4 Missing and 1 partial ⚠️
pkg/controllers/common/utils.go 72.72% 2 Missing and 1 partial ⚠️
pkg/controllers/subnetset/subnetset_controller.go 82.35% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #974      +/-   ##
==========================================
- Coverage   73.37%   73.29%   -0.08%     
==========================================
  Files         118      118              
  Lines       16394    16397       +3     
==========================================
- Hits        12029    12019      -10     
- Misses       3581     3591      +10     
- Partials      784      787       +3     
Flag Coverage Δ
unit-tests 73.29% <77.77%> (-0.08%) ⬇️
Files with missing lines Coverage Δ
pkg/controllers/pod/pod_controller.go 61.57% <100.00%> (-0.41%) ⬇️
pkg/controllers/subnet/subnet_controller.go 63.74% <100.00%> (+0.10%) ⬆️
pkg/nsx/services/subnet/store.go 71.64% <ø> (-3.90%) ⬇️
pkg/nsx/services/subnetport/store.go 77.10% <ø> (ø)
pkg/controllers/common/utils.go 88.62% <72.72%> (-1.86%) ⬇️
pkg/controllers/subnetset/subnetset_controller.go 66.08% <82.35%> (+0.09%) ⬆️
pkg/nsx/services/subnetport/subnetport.go 82.08% <91.66%> (+1.25%) ⬆️
...kg/controllers/subnetport/subnetport_controller.go 74.74% <41.66%> (-0.95%) ⬇️
pkg/nsx/services/subnet/subnet.go 64.78% <37.50%> (-1.97%) ⬇️

... and 3 files with indirect coverage changes

pkg/nsx/services/subnetport/subnetport.go Outdated Show resolved Hide resolved
pkg/nsx/services/subnetport/store.go Outdated Show resolved Hide resolved
pkg/nsx/services/subnetport/store.go Outdated Show resolved Hide resolved
pkg/nsx/services/subnetport/subnetport.go Outdated Show resolved Hide resolved
pkg/controllers/subnetport/subnetport_controller.go Outdated Show resolved Hide resolved
@yanjunz97 yanjunz97 force-pushed the refactor-subnet-lock branch 4 times, most recently from 3637eec to e0ad08b Compare December 26, 2024 06:28
@yanjunz97 yanjunz97 requested a review from wenyingd December 26, 2024 07:28
@yanjunz97 yanjunz97 force-pushed the refactor-subnet-lock branch from e0ad08b to 590b20d Compare December 26, 2024 07:54
wenyingd
wenyingd previously approved these changes Dec 26, 2024
Copy link
Contributor

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

dantingl
dantingl previously approved these changes Dec 27, 2024
Copy link
Contributor

@lxiaopei lxiaopei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a general comment: for the new introduced functions, could you add some docs to explain what the func is for? including the struct, import fields like "dirtyCounts" etc, it would be helpful for reviewer and other members to understand.

@yanjunz97 yanjunz97 dismissed stale reviews from dantingl and wenyingd via 8494fed December 27, 2024 06:38
@yanjunz97 yanjunz97 force-pushed the refactor-subnet-lock branch from 590b20d to 8494fed Compare December 27, 2024 06:38
@yanjunz97 yanjunz97 merged commit c729a64 into vmware-tanzu:main Dec 27, 2024
2 checks passed
yanjunz97 added a commit to yanjunz97/nsx-operator that referenced this pull request Dec 27, 2024
Instead of using lock for Subnet, this PR adds a cache in SubnetPort service
to record the number of SubnetPorts on each Subnet.
Read/Write operation for this cache is protected by lock to avoid race condition
and ensure the number of SubnetPort on the Subnet will be controlled below
the Subnet capacity.

Testing done:
- Create 16 Pods on default SubnetSet with size 16 (capacity 12), got all Pods ready; 
   delete all Pods and observe the Subnets under the SubnetSet will be deleted by GC.
- Create 16 VMs on DHCPServer SubnetSet with size 16 (capacity 12), all Vms can get IP; 
   delete all VMs and observe the Subnet under the SubnetSet will be deleted by GC.
- Create 16 VMs on DHCPServer Subnet with size 16 (capacity 12), only 12 VMs can PowerOn
   and get IP. The remainings will have `VirtualMachineNetworkReady` as `NotReady` and
   Message like `network interface "eth0" error: subnetPort is not ready: SubnetPortNotReady - error occurred while processing the SubnetPort CR. Error: no valid IP in Subnet /orgs/default/projects/project-quality/vpcs/ns-1_7fcad11c-da38-4137-96ec-f0f80ae8d96b/subnets/subnet-test-1_60ebfa36-bbe2-4e6e-85df-8228be4e190a`

Signed-off-by: Yanjun Zhou <[email protected]>
yanjunz97 added a commit that referenced this pull request Dec 27, 2024
Instead of using lock for Subnet, this PR adds a cache in SubnetPort service
to record the number of SubnetPorts on each Subnet.
Read/Write operation for this cache is protected by lock to avoid race condition
and ensure the number of SubnetPort on the Subnet will be controlled below
the Subnet capacity.

Testing done:
- Create 16 Pods on default SubnetSet with size 16 (capacity 12), got all Pods ready; 
   delete all Pods and observe the Subnets under the SubnetSet will be deleted by GC.
- Create 16 VMs on DHCPServer SubnetSet with size 16 (capacity 12), all Vms can get IP; 
   delete all VMs and observe the Subnet under the SubnetSet will be deleted by GC.
- Create 16 VMs on DHCPServer Subnet with size 16 (capacity 12), only 12 VMs can PowerOn
   and get IP. The remainings will have `VirtualMachineNetworkReady` as `NotReady` and
   Message like `network interface "eth0" error: subnetPort is not ready: SubnetPortNotReady - error occurred while processing the SubnetPort CR. Error: no valid IP in Subnet /orgs/default/projects/project-quality/vpcs/ns-1_7fcad11c-da38-4137-96ec-f0f80ae8d96b/subnets/subnet-test-1_60ebfa36-bbe2-4e6e-85df-8228be4e190a`

Signed-off-by: Yanjun Zhou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants