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

Remove Subnet Finalizer #7

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Conversation

wenqiq
Copy link
Owner

@wenqiq wenqiq commented Sep 19, 2024

No description provided.

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

Attention: Patch coverage is 23.85321% with 249 lines in your changes missing coverage. Please review.

Project coverage is 45.90%. Comparing base (4357c7d) to head (927826a).

Files with missing lines Patch % Lines
pkg/controllers/subnetset/subnetset_controller.go 0.00% 111 Missing ⚠️
pkg/controllers/subnet/subnet_controller.go 1.16% 85 Missing ⚠️
...ollers/securitypolicy/securitypolicy_controller.go 5.26% 17 Missing and 1 partial ⚠️
pkg/nsx/services/securitypolicy/firewall.go 33.33% 11 Missing and 1 partial ⚠️
pkg/nsx/services/subnet/subnet.go 0.00% 7 Missing ⚠️
pkg/nsx/services/securitypolicy/expand.go 0.00% 6 Missing ⚠️
pkg/nsx/services/securitypolicy/builder.go 92.50% 3 Missing ⚠️
cmd/main.go 0.00% 2 Missing ⚠️
...trollers/networkpolicy/networkpolicy_controller.go 0.00% 2 Missing ⚠️
pkg/nsx/services/subnet/store.go 66.66% 2 Missing ⚠️
... and 1 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main       #7      +/-   ##
==========================================
+ Coverage   43.77%   45.90%   +2.13%     
==========================================
  Files         100      100              
  Lines       14688    12401    -2287     
==========================================
- Hits         6429     5693     -736     
+ Misses       7766     6221    -1545     
+ Partials      493      487       -6     
Flag Coverage Δ
unit-tests 45.90% <23.85%> (+2.13%) ⬆️
Files with missing lines Coverage Δ
pkg/nsx/services/common/types.go 100.00% <ø> (ø)
pkg/nsx/services/ipaddressallocation/builder.go 91.66% <100.00%> (+3.20%) ⬆️
pkg/nsx/services/subnet/builder.go 15.51% <ø> (+3.18%) ⬆️
pkg/nsx/services/vpc/vpc.go 21.59% <100.00%> (+0.34%) ⬆️
pkg/util/utils.go 50.65% <100.00%> (+0.51%) ⬆️
pkg/controllers/common/utils.go 10.86% <0.00%> (+0.15%) ⬆️
cmd/main.go 0.00% <0.00%> (ø)
...trollers/networkpolicy/networkpolicy_controller.go 0.00% <0.00%> (ø)
pkg/nsx/services/subnet/store.go 41.81% <66.66%> (+5.45%) ⬆️
pkg/nsx/services/securitypolicy/builder.go 81.73% <92.50%> (+5.35%) ⬆️
... and 6 more

... and 77 files with indirect coverage changes

Fix the nil pointer dereference panic issue

Update `EdgeClusterEnabled` function and add a cover unit test case

Signed-off-by: Wenqi Qiu <[email protected]>
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch 2 times, most recently from b69b21e to 1652a33 Compare September 20, 2024 10:44
timdengyun and others added 3 commits September 23, 2024 11:44
Previsouly, we use hyphen "-" to connect strings when building NSX resource
name, and use underline "_" to connect strings when builindg NSX
resource ID.

This patch is to unify NSX resource ID and name connecotr as underline when building
ID and name from K8s CR.

For the NSX resoruce ID and name convention,
this patch is follow the standard in PR:vmware-tanzu#643

These NSX resources name are impacted in this change:
VPC
Subnet
SubnetPort
SecurityPolicy and NetworkPolicy
NSGroup and IPSetGroup
NSRule
Share
StaticRoute
IPAllocation

This patch is aslo to add two flags for GC and vpcCleanup in order to
distinguish these two cases.
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch from f407519 to 7c8d783 Compare September 24, 2024 06:39
Integrate Codecov into CI pipeline
Uploaded coverage to Codecov
Viewed coverage reports in pull requests

Signed-off-by: Wenqi Qiu <[email protected]>
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch 2 times, most recently from 927826a to aa23e65 Compare September 24, 2024 15:47
Copy link

codecov bot commented Sep 24, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

seanpang-vmware and others added 5 commits September 25, 2024 10:12
The private ipblocks in VPC are now maintained by nsx, nsx operator
will not do operations on these nsx resources, so vpc service do not
need to build its store anymore.

Remove the private ip blocks store and its related logics.
The cleanup command, it's called by wcpsvc as a lib. Wcpsvc
may call have multiple cleanup instances.
The tokenProvider should have no global cache when LibMode is true
Remove tokenProvider cache for clean
…nzu#770)

Subnetport/pod controller will acquire a subnet, create nsx subnetport on it and
save the subnetport to store after it is created. But Subnetset GC may delete the
subnet during the subnetport creation

The PR adds locks for subnet by subnet path to avoid this race condition.

Testing done:
Decreased the subnetset garbage collector running interval to 10 seconds to increase
the potential of race. Created 8 pods, waited for all related subnetports created without
error and delete the pods. Repeated for 5 times and not found deadlock or race issue.

Signed-off-by: Yanjun Zhou <[email protected]>
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch from aa23e65 to 251eb16 Compare September 26, 2024 03:01
lxiaopei and others added 7 commits September 26, 2024 19:31
Based on new NSX VPC 2.0 API, we need to use below request body to create NSX subnet:
{
        "id": "subnet-1",
        "resource_type": "VpcSubnet",
        "display_name": "Subnet 1",
        "description": "This is test VpcSubnet",
        "access_mode": "Private",
        "ipv4_subnet_size":64,
        "dhcp_config": {
            "enable_dhcp": false
        },
        "_revision": 0
    }
…subnet

Build Subnet DHCPConfig with new NSX VPC 2.0 API
Create new tokenProvider for cleanup command
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch 2 times, most recently from a74cabd to 0fbef86 Compare September 28, 2024 20:41
seanpang-vmware and others added 3 commits September 29, 2024 10:46
Per FSS enable requirement, UT coverage should be over 70%.
In VPC creation procedure, if nsx vpc is created but failed to
realize, it is not insert into vpc store, so in vpc deletion method
if vpc is not found in store, it should also return OK.
seanpang-vmware and others added 2 commits September 29, 2024 17:39
Do not check cache when deleting unrealized vpc
NSX does not support change either from static IP Allocation to DHCP or from DHCP to static IP Allocation.
This PR updates the Subnet/SubnetSet CRD to make DHCPConfig immutable;

and also

- Prevent all the immutable fields in Subnet/SubnetSet from being updated from a value to empty
- Fixes a bug in subnet update to inherit other fields from the existing Subnet
to avoid overriding some immutable fields like IP Addresses and IP Blocks by empty during
the update.

Signed-off-by: Yanjun Zhou <[email protected]>
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch 2 times, most recently from da8e495 to 633087f Compare October 1, 2024 12:57
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch 5 times, most recently from f5ac307 to 1629917 Compare October 10, 2024 07:45
Remove SubnetSet Finalizer
Add unit-test for Subnet controller

Signed-off-by: Wenqi Qiu <[email protected]>

Signed-off-by: Wenqi Qiu <[email protected]>
@wenqiq wenqiq force-pushed the topic/wenqi/rm-subnet-finalizer branch from c390e0d to 0f72d32 Compare October 10, 2024 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants