Implement Mixed Mode State Detection#1396
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1396 +/- ##
==========================================
- Coverage 76.77% 75.32% -1.45%
==========================================
Files 151 152 +1
Lines 21315 21441 +126
==========================================
- Hits 16364 16150 -214
- Misses 3782 3796 +14
- Partials 1169 1495 +326
🚀 New features to boost your workflow:
|
c29cf80 to
0fea7b3
Compare
Replace the global boolean cf.CoeConfig.EnableVPCNetwork with namespace-label-driven mixed-mode state: HasT1Namespaces and HasVPCNamespaces. New module: pkg/config/mixed_mode.go - Checks SupervisorCapabilities CRD (iaas.vmware.com/v1alpha1) for supports_per_namespace_network_providers capability. - If supported: scans namespace labels (iaas.vmware.com/network-provider) to derive HasT1Namespaces (nsx-t1) and HasVPCNamespaces (nsx-vpc or vsphere-network). - If not supported (legacy/pre-9.2): falls back to EnableVPCNetwork config flag. - Provides InitializeMixedModeState(), RefreshMixedModeState(), HasT1Namespaces(), HasVPCNamespaces(), GetNamespaceNetworkProvider() APIs. This enables NSX Operator to run in mixed mode where both T1 and VPC namespaces coexist, as required for VDS->VPC and T1->VPC migration. NOTE: This patch only ensures that the existing pure T1 or pure VPC envs preserve existing behaviours. The full functionality will be implemented in the follow-up patches.
|
|
||
| func init() { | ||
| zapLogger, _ := zap.NewProduction() | ||
| stateLog = zapLogger.Sugar() |
There was a problem hiding this comment.
why not use the existing log like logger.Log as other components?
| } else { | ||
| stateLog.Infof("Failed to get SupervisorCapabilities: %v; "+ | ||
| "falling back to legacy config", err) | ||
| } |
There was a problem hiding this comment.
Shall we return error or retry for errors other than notfound, like temporally k8s network issue?
| obj, err := dynClient.Resource(supervisorCapabilitiesGVR).Get( | ||
| ctx, supervisorCapabilitiesName, metav1.GetOptions{}) |
There was a problem hiding this comment.
Just to mention we may need to update rbac for ncp to get supervisorcapabilities right?
| case ProviderVSphereNetwork: | ||
| hasVPC = true |
There was a problem hiding this comment.
Just to double confirm vsphere-network is also considered as vpc mode?
| nsList, err := clientset.CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) | ||
| if err != nil { | ||
| stateLog.Errorf("Failed to list namespaces for mixed-mode state detection: %v", err) | ||
| return false, false |
There was a problem hiding this comment.
Shall we return an error here as list ns failure does not mean there is no vpc/t1 namespace?
|
|
||
| // RefreshMixedMode re-scans namespace labels and updates state. | ||
| // Returns true if the state changed (caller should consider restarting). | ||
| func RefreshMixedMode(ctx context.Context, clientset kubernetes.Interface) bool { |
There was a problem hiding this comment.
This function is not called anywhere?
|
|
||
| // SetMixedModeStateForTest sets hasT1Namespaces and hasVPCNamespaces for unit tests. | ||
| // Must only be used from test code so production always goes through InitMixedMode. | ||
| func SetMixedModeStateForTest(hasT1, hasVPC bool) { |
There was a problem hiding this comment.
As this is only for test can we move it to mixed_mode_test.go?
|
|
||
| // SetEnableVpcNetwork is deprecated; mixed-mode state is now managed by | ||
| // config.InitMixedMode. Kept for backward compatibility. | ||
| func SetEnableVpcNetwork(vpcNetwork bool) { |
There was a problem hiding this comment.
By backward compatibility, would you mean there is some consumers outside nsx operator using this function?
| @@ -188,59 +188,55 @@ func TestUpdateFeatureLicense(t *testing.T) { | |||
| } | |||
There was a problem hiding this comment.
I see SetEnableVpcNetwork is called TestUpdateFeatureLicense, does it matter?
https://github.com/heypnus/nsx-operator/blob/c0064c0955b24fbbabaf6b41f46fa7b00c916bd3/pkg/nsx/util/license_test.go#L134
https://github.com/heypnus/nsx-operator/blob/c0064c0955b24fbbabaf6b41f46fa7b00c916bd3/pkg/nsx/util/license_test.go#L152
https://github.com/heypnus/nsx-operator/blob/c0064c0955b24fbbabaf6b41f46fa7b00c916bd3/pkg/nsx/util/license_test.go#L171
| // IsVPCEnabled returns whether VPC namespaces exist. Callers must ensure mixed-mode | ||
| // state has been initialized (InitMixedMode in main; SetMixedModeStateForTest in tests). | ||
| // The "no CR" fallback to config is handled inside InitMixedMode, not here. | ||
| func IsVPCEnabled(service *SecurityPolicyService) bool { |
There was a problem hiding this comment.
service is not used, maybe we can remove it form the parameter?
Replace the global boolean cf.CoeConfig.EnableVPCNetwork with
namespace-label-driven mixed-mode state: HasT1Namespaces and
HasVPCNamespaces.
New module: pkg/config/mixed_mode.go
supports_per_namespace_network_providers capability.
(iaas.vmware.com/network-provider) to derive HasT1Namespaces
(nsx-t1) and HasVPCNamespaces (nsx-vpc or vsphere-network).
config flag.
HasT1Namespaces(), HasVPCNamespaces(),
GetNamespaceNetworkProvider() APIs.
This enables NSX Operator to run in mixed mode where both T1 and VPC
namespaces coexist, as required for VDS->VPC and T1->VPC migration.
NOTE: This patch only ensures that the existing pure T1 or pure VPC envs
preserve existing behaviours. The full functionality will be
implemented in the follow-up patches.
Testing done: