For sharing review comments

[tnqn]

No description provided.

[tnqn]

This is not proper to be in init as they will be executed as long as the package is imported, which means you need to prepare a config file even when running unit test for cmd package.

[tnqn]

If the func could fail to return a valid nsxClient, it should use nil error to indicate it, instead of returning nil nsxClient. Currently the code is contradictory, the err is always nil as it's not received from the function, and the func never returns nil nsxClient, even it fails (Instead, it just log an error`)

nsx-operator/pkg/nsx/client.go

Lines 188 to 197 in acf8557

	if !nsxClient.NSXCheckVersion(SecurityPolicy) {
	err := errors.New("SecurityPolicy feature support check failed")
	log.Error(err, "initial NSX version check for SecurityPolicy got error")
	}
	if !nsxClient.NSXCheckVersion(ServiceAccount) {
	err := errors.New("NSXServiceAccount feature support check failed")
	log.Error(err, "initial NSX version check for NSXServiceAccount got error")
	}
	return nsxClient

[tnqn]

Such one time log indicating the enablement of a module could be V(0) as it's few but provide key information.

[tnqn]

If the func is supposed to be called concurrently, it's not really thread-safe as L48 is without lock acquired.
If the func is not supposed to be called concurrently, the lock is not required at all.
From the usage, I think the lock and the global variable subnetServer makes it complicated, and it would be hard to use SubnetService in unit test, as you will need to reset the global variable every time after a test writes it.
A simpler and clearer way to initialzie the struct is just expose a constructor to main.go and inject the service to modules which rely on it:

func NewSubnetService(service common.Service) (*SubnetService, error)

...
# main.go
subnetService = subnet.NewSubnetService(commonService)
subnetReconciler.Service = subnetService
...
subnetsetReconciler.Service = subnetService

[tnqn]

I think the essential of the code is that it wants to get the result of InitializeResourceStore synchronously, regardless of success or failure. The usage of the channels and waitGroup makes it really complicated. Why don't just make InitializeResourceStore return an error?

func NewSubnetService(service common.Service) (*SubnetService, error) {
	subnetService := &SubnetService{
		Service: service,
		SubnetStore: &SubnetStore{
			ResourceStore: common.ResourceStore{
				Indexer: cache.NewIndexer(keyFunc, cache.Indexers{
					common.TagScopeSubnetCRUID: subnetIndexFunc,
				}),
				BindingType: model.VpcSubnetBindingType(),
			},
		},
	}
	if err := subnetService.InitializeResourceStore(ResourceTypeSubnet, nil, subnetService.SubnetStore); err != nil {
		return nil, err
	}
	return subnetService, nil
}

[tnqn]

Besides, closing a channel could make goroutines writting to it panic, which is the case of InitializeResourceStore.

[jwsui]

I think the essential of the code is that it wants to get the result of InitializeResourceStore synchronously, regardless of success or failure. The usage of the channels and waitGroup makes it really complicated. Why don't just make InitializeResourceStore return an error?

func NewSubnetService(service common.Service) (*SubnetService, error) {
	subnetService := &SubnetService{
		Service: service,
		SubnetStore: &SubnetStore{
			ResourceStore: common.ResourceStore{
				Indexer: cache.NewIndexer(keyFunc, cache.Indexers{
					common.TagScopeSubnetCRUID: subnetIndexFunc,
				}),
				BindingType: model.VpcSubnetBindingType(),
			},
		},
	}
	if err := subnetService.InitializeResourceStore(ResourceTypeSubnet, nil, subnetService.SubnetStore); err != nil {
		return nil, err
	}
	return subnetService, nil
}

For the case of Subnet service, we can just make InitializeResourceStore just return an error. I think InitializeResourceStore is designed to use channels/waitgroup to initialize multiple resource stores simultaneously in different goroutines. Taking firewall service as example, it initializes four kinds of resource stores simultaneously.

https://github.com/vmware-tanzu/nsx-operator/blob/vpc_dev/pkg/nsx/services/securitypolicy/firewall.go#L70

But I see that most of services ONLY initialize one store, so it also makes sense to simplify InitializeResourceStore.

[tnqn]

How does it guarantee NSXResourcePath has been set when processing this subnet? If there is no guarantee, errors may be generated randomly which might scare users who are monitoring component logs.
If having a valid NSXResourcePath is a precondition of creating subnet, perhaps it could skip enqueuing a subnet when VPC is not ready and enqueue associated subnets when VPC is ready.

[tnqn]

It could enforce the validation in CRD schema to ensure at most one SubnetSet/Subnet is set. Otherwise returning the error to make it retry is pointless.

[tnqn]

use consistent naming style: oldStatus

[tnqn]

Guess there is no guarantee that NSXResourcePath is not empty here. Instead of returning error, perhaps it could also skip enqueueing the subnetPort when its subnet's status is not ready, and watch subnet's status update event and make it enqueue associated subnetPorts which are not realized yet.

[tnqn]

Guess this is related to where @dantingl raised the race condition. Except for adding a lock to the method, there may be another solution:
subnetsetController also watches subsetPorts, and checks if the current capacity is enough by comparing the expected capacity and number of current subsetPorts. In its reconcile func, it auto-scales the subnetSet when the capacity is not enough.
Then subnetPort just waits for available subnet/subnetSet to be available and can unify the implementation.

[tnqn]

Don't use space to connect multi words in the key of structured logging, otherwise it could be hard to differentiate key and value in some cases and hard to use tool to extract key and values. Some suggestions about logging:
https://github.com/tnqn/code-review-comments#logging

[tnqn]

It doesn't make sense to use cpu number as the concurrency of network-intensive tasks, the efficiecy of the application may vary in different environments and unpredictable. It may run well when it's tested in a env but badly in another.

[tnqn]

It could conflict with the code that allocates empty subnet. If #1 (comment) is accepted, perhaps it could implement garbage collection in Reconcile, it could be retriggered by always returning RequeueAfter as the GCInterval