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 cache stores for VPCNetworkConfig #1025

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

Conversation

yanjunz97
Copy link
Contributor

@yanjunz97 yanjunz97 commented Feb 19, 2025

Current we maintain 2 caches, VPCNSNetworkConfigStore and VPCNetworkConfigStore,
for mapping between Namespace, VPCNetworkConfig name and VPCNetworkConfig Info.
But those information are parsed based Namespace and VPCNetworkConfig CRs, which
are cached in controller runtime client. Thus we remove the cache in NSX Operator and
rely on controller runtime client cache to get those information.

Testing done:
Create a Namespace and create VM on the Namespace. The VM can power on with IP.

@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2025

Codecov Report

Attention: Patch coverage is 74.77876% with 57 lines in your changes missing coverage. Please review.

Project coverage is 74.27%. Comparing base (9d0074b) to head (30f85aa).

Files with missing lines Patch % Lines
.../controllers/networkinfo/networkinfo_controller.go 65.07% 15 Missing and 7 partials ⚠️
pkg/nsx/services/vpc/vpc.go 83.33% 9 Missing and 5 partials ⚠️
pkg/controllers/namespace/namespace_controller.go 64.28% 3 Missing and 2 partials ⚠️
...ontrollers/networkinfo/vpcnetworkconfig_handler.go 50.00% 5 Missing ⚠️
pkg/controllers/networkinfo/networkinfo_utils.go 42.85% 3 Missing and 1 partial ⚠️
pkg/controllers/subnet/subnet_controller.go 33.33% 3 Missing and 1 partial ⚠️
pkg/controllers/subnetset/subnetset_controller.go 71.42% 2 Missing ⚠️
...ervices/ipaddressallocation/ipaddressallocation.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1025      +/-   ##
==========================================
- Coverage   74.52%   74.27%   -0.25%     
==========================================
  Files         118      118              
  Lines       16358    16351       -7     
==========================================
- Hits        12191    12145      -46     
- Misses       3395     3414      +19     
- Partials      772      792      +20     
Flag Coverage Δ
unit-tests 74.27% <74.77%> (-0.25%) ⬇️
Files with missing lines Coverage Δ
pkg/nsx/services/vpc/builder.go 90.90% <100.00%> (+4.06%) ⬆️
...ervices/ipaddressallocation/ipaddressallocation.go 63.77% <0.00%> (-0.33%) ⬇️
pkg/controllers/subnetset/subnetset_controller.go 75.14% <71.42%> (-1.22%) ⬇️
pkg/controllers/networkinfo/networkinfo_utils.go 76.74% <42.85%> (-1.36%) ⬇️
pkg/controllers/subnet/subnet_controller.go 70.82% <33.33%> (-0.88%) ⬇️
pkg/controllers/namespace/namespace_controller.go 60.60% <64.28%> (-5.54%) ⬇️
...ontrollers/networkinfo/vpcnetworkconfig_handler.go 62.68% <50.00%> (-20.65%) ⬇️
pkg/nsx/services/vpc/vpc.go 55.87% <83.33%> (+0.50%) ⬆️
.../controllers/networkinfo/networkinfo_controller.go 65.04% <65.07%> (-4.01%) ⬇️

@zhengxiexie
Copy link
Contributor

/e2e

1 similar comment
@zhengxiexie
Copy link
Contributor

/e2e

@yanjunz97 yanjunz97 force-pushed the rm-vpcconfig-cache branch 2 times, most recently from 6fe027e to 1e165d5 Compare February 24, 2025 06:01
@zhengxiexie
Copy link
Contributor

/e2e

2 similar comments
@zhengxiexie
Copy link
Contributor

/e2e

@zhengxiexie
Copy link
Contributor

/e2e

@yanjunz97 yanjunz97 force-pushed the rm-vpcconfig-cache branch 2 times, most recently from 518b665 to b119538 Compare February 25, 2025 07:36
log.Info("Failed to get network config info using network config name", "Name", ncName)
return nil
log.Info("Network config info does not exist", "Name", ncName)
return nil, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

how about use 'NetworkConfig' for line 122/118?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. There are many occurrence of network config, I replaced them all by NetworkConfig, thanks

}

func IsPreCreatedVPC(nc common.VPCNetworkConfigInfo) bool {
return nc.VPCPath != ""
}

func buildNetworkConfigInfo(vpcConfigCR *v1alpha1.VPCNetworkConfiguration) (*common.VPCNetworkConfigInfo, error) {
org, project, err := nsxProjectPathToId(vpcConfigCR.Spec.NSXProject)
Copy link
Contributor

Choose a reason for hiding this comment

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

move it to build.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks!


func isDefaultNetworkConfigCR(vpcConfigCR *v1alpha1.VPCNetworkConfiguration) bool {
annos := vpcConfigCR.GetAnnotations()
val, exist := annos[common.AnnotationDefaultNetworkConfig]
Copy link
Contributor

Choose a reason for hiding this comment

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

the same function isDefaultNetworkConfigCR/nsxProjectPathToId in the vpcnetworkconfig_handler.go, could you reuse them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These 2 functions are not used in vpcnetworkconfig_handler, removed them, thanks!

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants