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

Cleanup enhancement #1009

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

Conversation

wenyingd
Copy link
Contributor

@wenyingd wenyingd commented Jan 16, 2025

This change is trying to use HAPI in cleanup logic, and remove unnecessary NSX API calls if a VPC is already deleted recursively. It also introduces the VPC deletion in parallel.

Test Done:

  1. With a benchmark for 50 VPCs, 5 subnets/ 100 subnet ports / 5 subnet connection binding maps / 1 static routes / 1 vpc IP address allocation / 1 security policy configured per VPC, the new logic takes about 3m to complete the clean up job, while the original logic may take almost 30m to do the same thing.

@wenyingd wenyingd force-pushed the cleanup_enhancement branch 2 times, most recently from d776fe1 to 2b12a1d Compare January 16, 2025 10:47
@wenyingd wenyingd requested a review from lxiaopei January 20, 2025 07:25
@wenyingd wenyingd force-pushed the cleanup_enhancement branch 4 times, most recently from 89d24fe to ae03ceb Compare January 21, 2025 06:04
@codecov-commenter
Copy link

codecov-commenter commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 77.90698% with 361 lines in your changes missing coverage. Please review.

Project coverage is 74.87%. Comparing base (954ed89) to head (86367da).

Files with missing lines Patch % Lines
pkg/nsx/services/common/policy_tree.go 78.55% 68 Missing and 9 partials ⚠️
pkg/clean/types.go 65.90% 45 Missing and 15 partials ⚠️
pkg/nsx/services/common/wrap.go 77.88% 29 Missing and 15 partials ⚠️
pkg/clean/clean_lb_infra.go 84.46% 28 Missing and 11 partials ⚠️
pkg/nsx/services/securitypolicy/cleanup.go 74.78% 21 Missing and 9 partials ⚠️
pkg/nsx/services/common/store.go 71.62% 18 Missing and 3 partials ⚠️
pkg/nsx/services/ipaddressallocation/store.go 26.66% 11 Missing ⚠️
pkg/nsx/services/staticroute/store.go 26.66% 11 Missing ⚠️
pkg/nsx/services/subnetbinding/store.go 45.00% 11 Missing ⚠️
pkg/nsx/services/ipaddressallocation/cleanup.go 52.38% 9 Missing and 1 partial ⚠️
... and 9 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1009      +/-   ##
==========================================
+ Coverage   74.22%   74.87%   +0.64%     
==========================================
  Files         118      125       +7     
  Lines       16378    17183     +805     
==========================================
+ Hits        12157    12865     +708     
- Misses       3451     3531      +80     
- Partials      770      787      +17     
Flag Coverage Δ
unit-tests 74.87% <77.90%> (+0.64%) ⬆️
Files with missing lines Coverage Δ
pkg/nsx/client.go 84.61% <100.00%> (-0.24%) ⬇️
pkg/nsx/services/common/types.go 100.00% <100.00%> (ø)
...ervices/ipaddressallocation/ipaddressallocation.go 44.76% <100.00%> (-2.03%) ⬇️
pkg/nsx/services/securitypolicy/builder.go 85.32% <100.00%> (ø)
pkg/nsx/services/securitypolicy/store.go 78.14% <100.00%> (+2.09%) ⬆️
pkg/nsx/services/staticroute/staticroute.go 74.78% <100.00%> (-2.49%) ⬇️
pkg/nsx/services/subnet/store.go 70.42% <100.00%> (-1.22%) ⬇️
pkg/nsx/services/subnet/subnet.go 68.61% <100.00%> (-1.66%) ⬇️
pkg/nsx/services/subnetport/cleanup.go 100.00% <100.00%> (ø)
pkg/nsx/services/subnetport/store.go 78.16% <100.00%> (+1.05%) ⬆️
... and 20 more

... and 1 file with indirect coverage changes

@wenyingd wenyingd force-pushed the cleanup_enhancement branch from ae03ceb to 86367da Compare January 21, 2025 06:21

func (p *PolicyResourcePath[T]) Length() int {
resourceTypes := ([]PolicyResourceType)(*p)
return len(resourceTypes)
Copy link
Contributor

Choose a reason for hiding this comment

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

return len(*p)?

}

func getVPCPathFromResourcePath(path string) ([]string, error) {
resInfo, err := ParseVPCResourcePath(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

may parse the vpcPath directly:
func ParseVPCPathFromResourcePath(nsxResourcePath string) (VPCPath string, err error) {
reExp := regexp.MustCompile((/orgs/([^/]+)/projects/([^/]+)/vpcs/([^/]+))([/\S+]*))
matches := reExp.FindStringSubmatch(nsxResourcePath)
if len(matches) != 6 {
err := fmt.Errorf("invalid path '%s'", nsxResourcePath)
return "", err
}
return matches[1], nil

if len(objects) == 0 {
return nil
}
fmt.Println(b.rootType, b.leafType, "count", len(objects))
Copy link
Contributor

Choose a reason for hiding this comment

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

use log.Info?

func PagingNSXResources[T any](resources []T, pageSize int) [][]T {
totalCount := len(resources)
pages := (totalCount + pageSize - 1) / pageSize
pagedResources := make([][]T, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

make([][]T,0, pages)

leafWrapper LeafDataWrapper[T]
pathGetter GetPath[T]
idGetter GetId[T]

Copy link
Contributor

Choose a reason for hiding this comment

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

why the three function defined as a property?

}

func wrapInfra(children []*data.StructValue) *model.Infra {
// This is the outermost layer of the hierarchy infra client.
Copy link
Contributor

Choose a reason for hiding this comment

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

wrapInfra data.StructValue-> model.Infra
WrapVPC model.Vpc-> data.StructValue.
but they have the same WrapXXX name?

}

func PagingDeleteResources[T any](ctx context.Context, builder *PolicyTreeBuilder[T], objs []T, pageSize int, nsxClient *nsx.Client, delFn func(deletedObjs []T)) error {
if len(objs) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

name delFun to deleteObjectsFromStore?

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.

4 participants