fix(controller): defer subnet processing until vlan is fully ready (#6527)

* fix(controller): clear subnet IP status when vlan conflict is detected

When two VLANs with the same ID are created on the same provider network,
a race condition causes the conflict subnet to retain its IP range in
status. This happens because:

1. The subnet handler can process the subnet before the vlan handler
   marks the vlan as conflicting (informer cache propagation delay of
   ~3ms), allowing IPAM allocation and IP status to be set.

2. When the vlan handler detects a conflict, it returns early without
   re-enqueuing associated subnets, so the subnet is never re-validated.

3. Even when the subnet is re-processed and detects the vlan conflict,
   patchSubnetStatus serializes the full status (including the stale IP
   range) without clearing IP fields.

Fix all three issues:
- In handleAddVlan/handleUpdateVlan: re-enqueue subnets referencing the
  conflicting vlan so they can re-validate.
- In handleAddOrUpdateSubnet: when vlan validation fails, remove the
  subnet from IPAM and clear all IP status fields before patching.

Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>

* fix(controller): address review feedback for vlan conflict handling

Address Copilot review comments:

1. Prevent queue churn: only re-enqueue subnets when vlan conflict
   status transitions from false to true (not on every retry). Save
   wasConflict before checkVlanConflict and gate the re-enqueue.

2. Scope IPAM cleanup: introduce errVlanConflict sentinel error in
   validateSubnetVlan and only clear IPAM/IP status fields when
   errors.Is(err, errVlanConflict) is true. Transient lister errors
   no longer trigger IPAM deletion.

3. Add unit tests: Test_validateSubnetVlan_conflict verifies the
   sentinel error is correctly returned for conflict vs normal vs
   missing vlans. Test_handleAddOrUpdateSubnet_clearsIPStatusOnVlanConflict
   verifies IPAM removal and status field clearing on conflict.

Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>

* refactor: revert vlan.go changes, keep minimal subnet-only fix

Remove vlan handler re-enqueue logic — controller logs confirm subnet2
is already re-processed through the normal error retry mechanism (the
reconcileVlan check at subnet.go:1687 detects conflict and returns
error, triggering requeue). The only fix needed is clearing IP status
fields when the subnet handler detects a vlan conflict on re-processing.

Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>

* fix(controller): defer subnet processing until vlan is fully ready

Instead of cleaning up stale IPAM state after a race condition, prevent
the race entirely: do not process a subnet until its vlan has been fully
handled by the vlan controller.

A newly created vlan has Status.Conflict=false (zero value), which is
indistinguishable from a processed non-conflicting vlan. Use
pn.Status.Vlans as a "vlan ready" signal — it is only populated after
handleAddVlan completes successfully (including the conflict check).

In validateSubnetVlan, after confirming the vlan is not conflicting,
verify it appears in the provider network's Status.Vlans. If not, the
vlan has not been fully processed yet, so return an error to defer
subnet processing until the vlan handler re-enqueues it.

In handleAddVlan, re-enqueue subnets referencing a conflicting vlan so
they can see the updated conflict status and be properly rejected.

This replaces the previous sentinel-error + IPAM-cleanup approach with
a simpler ordering guarantee that eliminates the race window entirely.

Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>

* fix(controller): address review - silent requeue for unready vlan

- Introduce errVlanNotReady sentinel: when the vlan hasn't been fully
  processed yet (empty provider, provider network not found, or vlan
  not in pn.Status.Vlans), return errVlanNotReady instead of a generic
  error. handleAddOrUpdateSubnet checks for this and requeues silently
  without patching subnet status as ValidateSubnetVlanFailed, avoiding
  misleading Warning events for a normal transient condition.

- Treat empty vlan.Spec.Provider as "not ready" (vlan handler hasn't
  run yet to default the provider) rather than falling back to
  DefaultProviderName.

- Add unit test for empty-provider vlan case.

Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>

* fix(controller): maintain pn.Status.Vlans in handleUpdateVlan

handleUpdateVlan did not update the provider network's Status.Vlans
list after a successful conflict check. This caused a regression where
validateSubnetVlan's new pn.Status.Vlans readiness check blocked
subnet processing after a vlan provider change — the vlan was not
registered in the new provider network's Vlans list, so all associated
subnets were stuck in "vlan not ready" state.

Add the same pn.Status.Vlans maintenance logic that handleAddVlan
already has: after checkVlanConflict passes, ensure the vlan appears
in its provider network's Status.Vlans.

Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>

---------

Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: oilbeater

pkg/controller/controller_test.go

@@ -80,6 +80,7 @@ type FakeControllerOptions struct {
 	VpcNatGateways     []*kubeovnv1.VpcNatGateway
 	IPs                []*kubeovnv1.IP
 	Vlans              []*kubeovnv1.Vlan
+	ProviderNetworks   []*kubeovnv1.ProviderNetwork
 	NetworkAttachments []*nadv1.NetworkAttachmentDefinition
 	Pods               []*corev1.Pod
 	Nodes              []*corev1.Node
@@ -158,6 +159,13 @@ func newFakeControllerWithOptions(t *testing.T, opts *FakeControllerOptions) (*f
 			return nil, err
 		}
 	}
+	for _, pn := range opts.ProviderNetworks {
+		_, err := kubeovnClient.KubeovnV1().ProviderNetworks().Create(
+			context.Background(), pn, metav1.CreateOptions{})
+		if err != nil {
+			return nil, err
+		}
+	}
 
 	// Create informer factories
 	kubeInformerFactory := informers.NewSharedInformerFactoryWithOptions(kubeClient, 0,
@@ -192,6 +200,7 @@ func newFakeControllerWithOptions(t *testing.T, opts *FakeControllerOptions) (*f
 	ipInformer := kubeovnInformerFactory.Kubeovn().V1().IPs()
 	vpcNatGwInformer := kubeovnInformerFactory.Kubeovn().V1().VpcNatGateways()
 	vlanInformer := kubeovnInformerFactory.Kubeovn().V1().Vlans()
+	providerNetworkInformer := kubeovnInformerFactory.Kubeovn().V1().ProviderNetworks()
 
 	fakeInformers := &fakeControllerInformers{
 		vpcInformer:       vpcInformer,
@@ -223,6 +232,7 @@ func newFakeControllerWithOptions(t *testing.T, opts *FakeControllerOptions) (*f
 		ipsLister:               ipInformer.Lister(),
 		ipSynced:                alwaysReady,
 		vlansLister:             vlanInformer.Lister(),
+		providerNetworksLister:  providerNetworkInformer.Lister(),
 		netAttachLister:         nadInformer.Lister(),
 		netAttachSynced:         alwaysReady,
 		OVNNbClient:             mockOvnClient,

pkg/controller/subnet.go

@@ -163,6 +163,11 @@ func (c *Controller) formatSubnet(subnet *kubeovnv1.Subnet) (*kubeovnv1.Subnet,
 	return subnet, nil
 }
 
+// errVlanNotReady is returned when the vlan has not been fully processed by
+// the vlan handler yet. The caller should requeue silently without patching
+// the subnet status as failed.
+var errVlanNotReady = errors.New("vlan not ready")
+
 func (c *Controller) validateSubnetVlan(subnet *kubeovnv1.Subnet) error {
 	if subnet.Spec.Vlan == "" {
 		return nil
@@ -180,6 +185,25 @@ func (c *Controller) validateSubnetVlan(subnet *kubeovnv1.Subnet) error {
 		klog.Error(err)
 		return err
 	}
+
+	// Ensure the vlan has been fully processed by the vlan handler before
+	// allowing the subnet to proceed. A newly created vlan has Conflict=false
+	// (zero value) which is indistinguishable from a processed non-conflicting
+	// vlan. Use pn.Status.Vlans as a "vlan ready" signal — only set after
+	// the vlan handler completes successfully (including conflict check).
+	// This prevents a race where the subnet allocates IPAM before the vlan's
+	// conflict status is determined.
+	if vlan.Spec.Provider == "" {
+		return fmt.Errorf("vlan %s provider not yet set, deferring subnet %s: %w", vlan.Name, subnet.Name, errVlanNotReady)
+	}
+	pn, err := c.providerNetworksLister.Get(vlan.Spec.Provider)
+	if err != nil {
+		return fmt.Errorf("vlan %s not yet ready (provider network %s not found): %w", vlan.Name, vlan.Spec.Provider, errVlanNotReady)
+	}
+	if !slices.Contains(pn.Status.Vlans, vlan.Name) {
+		return fmt.Errorf("vlan %s not yet processed by vlan handler, deferring subnet %s: %w", vlan.Name, subnet.Name, errVlanNotReady)
+	}
+
 	return nil
 }
 
@@ -491,6 +515,12 @@ func (c *Controller) handleAddOrUpdateSubnet(key string) error {
 
 	err = c.validateSubnetVlan(subnet)
 	if err != nil {
+		if errors.Is(err, errVlanNotReady) {
+			// vlan hasn't been processed yet, requeue silently without
+			// marking the subnet as failed
+			klog.Infof("deferring subnet %s: %v", key, err)
+			return err
+		}
 		err = fmt.Errorf("failed to validate vlan for subnet %s, %w", key, err)
 		klog.Error(err)
 		if patchErr := c.patchSubnetStatus(subnet, "ValidateSubnetVlanFailed", err.Error()); patchErr != nil {

pkg/controller/subnet_test.go

@@ -604,3 +604,97 @@ func Test_checkSubnetConflict(t *testing.T) {
 		require.NoError(t, err)
 	})
 }
+
+func Test_validateSubnetVlan(t *testing.T) {
+	t.Parallel()
+
+	pn := &kubeovnv1.ProviderNetwork{
+		ObjectMeta: metav1.ObjectMeta{Name: "test-pn"},
+		Status: kubeovnv1.ProviderNetworkStatus{
+			Vlans: []string{"ready-vlan"},
+		},
+	}
+	conflictVlan := &kubeovnv1.Vlan{
+		ObjectMeta: metav1.ObjectMeta{Name: "conflict-vlan"},
+		Spec:       kubeovnv1.VlanSpec{ID: 100, Provider: "test-pn"},
+		Status:     kubeovnv1.VlanStatus{Conflict: true},
+	}
+	readyVlan := &kubeovnv1.Vlan{
+		ObjectMeta: metav1.ObjectMeta{Name: "ready-vlan"},
+		Spec:       kubeovnv1.VlanSpec{ID: 200, Provider: "test-pn"},
+		Status:     kubeovnv1.VlanStatus{Conflict: false},
+	}
+	unprocessedVlan := &kubeovnv1.Vlan{
+		ObjectMeta: metav1.ObjectMeta{Name: "unprocessed-vlan"},
+		Spec:       kubeovnv1.VlanSpec{ID: 300, Provider: "test-pn"},
+		Status:     kubeovnv1.VlanStatus{Conflict: false}, // same as ready, but NOT in pn.Status.Vlans
+	}
+	emptyProviderVlan := &kubeovnv1.Vlan{
+		ObjectMeta: metav1.ObjectMeta{Name: "empty-provider-vlan"},
+		Spec:       kubeovnv1.VlanSpec{ID: 400, Provider: ""}, // not yet defaulted by vlan handler
+	}
+
+	fakeCtrl, err := newFakeControllerWithOptions(t, &FakeControllerOptions{
+		Vlans:            []*kubeovnv1.Vlan{conflictVlan, readyVlan, unprocessedVlan, emptyProviderVlan},
+		ProviderNetworks: []*kubeovnv1.ProviderNetwork{pn},
+	})
+	require.NoError(t, err)
+	ctrl := fakeCtrl.fakeController
+
+	t.Run("conflict vlan is rejected", func(t *testing.T) {
+		subnet := &kubeovnv1.Subnet{
+			ObjectMeta: metav1.ObjectMeta{Name: "test-subnet"},
+			Spec:       kubeovnv1.SubnetSpec{Vlan: "conflict-vlan"},
+		}
+		err := ctrl.validateSubnetVlan(subnet)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "conflict")
+	})
+
+	t.Run("ready vlan passes validation", func(t *testing.T) {
+		subnet := &kubeovnv1.Subnet{
+			ObjectMeta: metav1.ObjectMeta{Name: "test-subnet"},
+			Spec:       kubeovnv1.SubnetSpec{Vlan: "ready-vlan"},
+		}
+		err := ctrl.validateSubnetVlan(subnet)
+		require.NoError(t, err)
+	})
+
+	t.Run("unprocessed vlan defers subnet processing", func(t *testing.T) {
+		subnet := &kubeovnv1.Subnet{
+			ObjectMeta: metav1.ObjectMeta{Name: "test-subnet"},
+			Spec:       kubeovnv1.SubnetSpec{Vlan: "unprocessed-vlan"},
+		}
+		err := ctrl.validateSubnetVlan(subnet)
+		require.Error(t, err)
+		require.ErrorIs(t, err, errVlanNotReady)
+	})
+
+	t.Run("empty provider vlan is not ready", func(t *testing.T) {
+		subnet := &kubeovnv1.Subnet{
+			ObjectMeta: metav1.ObjectMeta{Name: "test-subnet"},
+			Spec:       kubeovnv1.SubnetSpec{Vlan: "empty-provider-vlan"},
+		}
+		err := ctrl.validateSubnetVlan(subnet)
+		require.Error(t, err)
+		require.ErrorIs(t, err, errVlanNotReady)
+	})
+
+	t.Run("missing vlan returns error", func(t *testing.T) {
+		subnet := &kubeovnv1.Subnet{
+			ObjectMeta: metav1.ObjectMeta{Name: "test-subnet"},
+			Spec:       kubeovnv1.SubnetSpec{Vlan: "nonexistent-vlan"},
+		}
+		err := ctrl.validateSubnetVlan(subnet)
+		require.Error(t, err)
+	})
+
+	t.Run("empty vlan passes validation", func(t *testing.T) {
+		subnet := &kubeovnv1.Subnet{
+			ObjectMeta: metav1.ObjectMeta{Name: "test-subnet"},
+			Spec:       kubeovnv1.SubnetSpec{Vlan: ""},
+		}
+		err := ctrl.validateSubnetVlan(subnet)
+		require.NoError(t, err)
+	})
+}

pkg/controller/vlan.go

@@ -122,6 +122,12 @@ func (c *Controller) handleAddVlan(key string) error {
 
 	if err = c.checkVlanConflict(vlan); err != nil {
 		klog.Errorf("failed to check vlan %s: %v", vlan.Name, err)
+		// re-enqueue subnets so they can see the conflict status and reject
+		for _, subnet := range subnets {
+			if subnet.Spec.Vlan == vlan.Name {
+				c.addOrUpdateSubnetQueue.Add(subnet.Name)
+			}
+		}
 		return err
 	}
 
@@ -136,7 +142,7 @@ func (c *Controller) handleAddVlan(key string) error {
 	}
 
 	// re-enqueue subnets that reference this vlan, so they can proceed
-	// if they were previously blocked by the vlan not being ready
+	// now that the vlan is fully processed
 	for _, subnet := range subnets {
 		if subnet.Spec.Vlan == vlan.Name {
 			c.addOrUpdateSubnetQueue.Add(subnet.Name)
@@ -207,6 +213,24 @@ func (c *Controller) handleUpdateVlan(key string) error {
 		return err
 	}
 
+	// ensure the vlan is registered in the provider network's Status.Vlans,
+	// which is used by validateSubnetVlan as a "vlan ready" signal.
+	// This is especially important after a provider change, where the vlan
+	// needs to appear in the new provider network's Vlans list.
+	pn, err := c.providerNetworksLister.Get(vlan.Spec.Provider)
+	if err != nil {
+		klog.Errorf("failed to get provider network %s: %v", vlan.Spec.Provider, err)
+		return err
+	}
+	if !slices.Contains(pn.Status.Vlans, vlan.Name) {
+		newPn := pn.DeepCopy()
+		newPn.Status.Vlans = append(newPn.Status.Vlans, vlan.Name)
+		if _, err = c.config.KubeOvnClient.KubeovnV1().ProviderNetworks().UpdateStatus(context.Background(), newPn, metav1.UpdateOptions{}); err != nil {
+			klog.Errorf("failed to update status of provider network %s: %v", pn.Name, err)
+			return err
+		}
+	}
+
 	subnets, err := c.subnetsLister.List(labels.Everything())
 	if err != nil {
 		klog.Errorf("failed to list subnets: %v", err)