Annotation `network.harvesterhci.io/mtu-source-vc` isn't cleaned up by votdev · Pull Request #237 · harvester/network-controller-harvester

votdev · 2026-02-26T11:17:58Z

Problem:
Build a cluster network, set the mtu annotation. Build a cluster network configuration. Then delete the cluster network configuration. The network.harvesterhci.io/mtu-source-vc annotation is left dangling on the Cluster Network.

Solution:
Make sure the annotations network.harvesterhci.io/mtu-source-vc and network.harvesterhci.io/uplink-mtu are updated/removed properly.

Do some additional code cleanup:

Rename GetMTUFromAnnotation to GetMTUFromString

Related Issue:
harvester/harvester#8783

Test plan:
ToDo

Copilot

Pull request overview

This PR addresses dangling MTU-related ClusterNetwork annotations left behind after deleting a VlanConfig (cluster network configuration), ensuring MTU source tracking and MTU annotations are updated/cleared correctly. It also includes a small MTU parsing utility rename and related call-site updates.

Changes:

Add VlanConfig remove handling to switch/clear network.harvesterhci.io/mtu-source-vc and network.harvesterhci.io/uplink-mtu on the referenced ClusterNetwork.
Update ClusterNetwork webhook validation to allow removing the uplink-mtu annotation without blocking updates.
Rename MTU parsing helper from GetMTUFromAnnotation to GetMTUFromString and update usages.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
pkg/webhook/nad/validator.go	Switch to `GetMTUFromString` when validating MTU annotation values.
pkg/webhook/nad/mutator.go	Switch to `GetMTUFromString` when mutating NAD MTU based on ClusterNetwork annotation.
pkg/webhook/clusternetwork/validator.go	Allow updates where `uplink-mtu` annotation is removed; update parser usage.
pkg/utils/mtu.go	Rename MTU parsing helper and adjust error messages accordingly.
pkg/controller/manager/vlanconfig/controller.go	Add `OnVlanConfigRemove` handler to update/clear ClusterNetwork MTU annotations on VC deletion.
pkg/controller/manager/nad/controller.go	Switch to `GetMTUFromString` when syncing CN MTU annotation into NAD config.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pkg/controller/manager/vlanconfig/controller.go

pkg/webhook/clusternetwork/validator.go

pkg/controller/manager/vlanconfig/controller.go

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-26T12:05:54Z

pkg/controller/manager/vlanconfig/controller.go

+	// present in the list).
+	var vcCandidate *networkv1.VlanConfig
+	for _, item := range vcs {
+		if item.Name == vc.Name || item.DeletionTimestamp != nil {
+			continue
+		}
+
+		if utils.IsValidMTU(utils.GetMTUFromVlanConfig(item)) {
+			vcCandidate = item
+			break
+		}
+	}
+


OnVlanConfigRemove picks the first remaining VlanConfig with a valid MTU, but utils.IsValidMTU treats MTU=0 as valid. This can cause the controller to switch mtu-source-vc to a VC with MTU=0 even when another remaining VC has an explicit non-zero MTU, leading to unnecessary annotation churn and potentially incorrect sourcing. Consider preferring a non-zero MTU candidate when available and only falling back to MTU=0 if no non-zero candidates exist (mirroring ensureClusterNetwork’s effective MTU logic).

Suggested change

// present in the list).

var vcCandidate *networkv1.VlanConfig

for _, item := range vcs {

if item.Name == vc.Name || item.DeletionTimestamp != nil {

continue

}

if utils.IsValidMTU(utils.GetMTUFromVlanConfig(item)) {

vcCandidate = item

break

}

}

// present in the list). Prefer a candidate with a non-zero MTU, and only

// fall back to a zero-MTU candidate if no non-zero MTU candidates exist.

var (

vcCandidateNonZero *networkv1.VlanConfig

vcCandidateZero *networkv1.VlanConfig

)

for _, item := range vcs {

if item.Name == vc.Name || item.DeletionTimestamp != nil {

continue

}

mtu := utils.GetMTUFromVlanConfig(item)

if !utils.IsValidMTU(mtu) {

continue

}

if mtu == 0 {

// Record a zero-MTU candidate as a fallback, but keep searching

// for a non-zero MTU candidate.

if vcCandidateZero == nil {

vcCandidateZero = item

}

continue

}

// Prefer the first non-zero MTU candidate.

vcCandidateNonZero = item

break

}

// Choose the best available candidate: prefer non-zero MTU, otherwise

// fall back to a zero-MTU candidate (if any).

vcCandidate := vcCandidateNonZero

if vcCandidate == nil {

vcCandidate = vcCandidateZero

}

Copilot · 2026-02-26T12:05:55Z

pkg/controller/manager/vlanconfig/controller.go

+	if vcCandidate != nil {
+		mtu := utils.GetMTUFromVlanConfig(vcCandidate)
+
+		// Please note that updating the MTU annotation here may be redundant,
+		// as all `VlanConfig`s should have the same MTU value. However, it is
+		// safer to update them in case the boundary conditions change in the
+		// future.
+		cnCopy.Annotations[utils.KeyUplinkMTU] = fmt.Sprintf("%v", mtu)
+		cnCopy.Annotations[utils.KeyMTUSourceVlanConfig] = vcCandidate.Name


When vcCandidate has MTU=0, this code writes the clusternetwork uplink-mtu annotation as the string "0". Elsewhere (ensureClusterNetwork) MTU=0 is treated as “default” and the annotation is set to DefaultMTU instead. This mismatch can cause the next reconcile to flip the annotation back (thrash). Consider normalizing MTU=0 to DefaultMTU before writing KeyUplinkMTU here, consistent with ensureClusterNetwork.

pkg/webhook/clusternetwork/validator.go

Do some additional code cleanup: - Rename `GetMTUFromAnnotation` to `GetMTUFromString` Related to: harvester/harvester#8783 Signed-off-by: Volker Theile <[email protected]>

votdev self-assigned this Feb 26, 2026

Copilot AI review requested due to automatic review settings February 26, 2026 11:17

Copilot started reviewing on behalf of votdev February 26, 2026 11:18 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

votdev force-pushed the issue_8783_cleanup_annotation branch 2 times, most recently from 9b7abe3 to fc4a3c6 Compare February 26, 2026 11:58

votdev requested review from a team and Copilot February 26, 2026 12:00

Copilot started reviewing on behalf of votdev February 26, 2026 12:01 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

Annotation network.harvesterhci.io/mtu-source-vc isn't cleaned up

6c275e8

Do some additional code cleanup: - Rename `GetMTUFromAnnotation` to `GetMTUFromString` Related to: harvester/harvester#8783 Signed-off-by: Volker Theile <[email protected]>

votdev force-pushed the issue_8783_cleanup_annotation branch from fc4a3c6 to 6c275e8 Compare February 26, 2026 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotation `network.harvesterhci.io/mtu-source-vc` isn't cleaned up#237

Annotation `network.harvesterhci.io/mtu-source-vc` isn't cleaned up#237
votdev wants to merge 1 commit intoharvester:masterfrom
votdev:issue_8783_cleanup_annotation

votdev commented Feb 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 26, 2026

Uh oh!

Copilot AI Feb 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-	// present in the list).
-	var vcCandidate *networkv1.VlanConfig
-	for _, item := range vcs {
-		if item.Name == vc.Name || item.DeletionTimestamp != nil {
-			continue
-		}
-		if utils.IsValidMTU(utils.GetMTUFromVlanConfig(item)) {
-			vcCandidate = item
-			break
-		}
-	}
+	// present in the list). Prefer a candidate with a non-zero MTU, and only
+	// fall back to a zero-MTU candidate if no non-zero MTU candidates exist.
+	var (
+		vcCandidateNonZero *networkv1.VlanConfig
+		vcCandidateZero    *networkv1.VlanConfig
+	)
+	for _, item := range vcs {
+		if item.Name == vc.Name || item.DeletionTimestamp != nil {
+			continue
+		}
+		mtu := utils.GetMTUFromVlanConfig(item)
+		if !utils.IsValidMTU(mtu) {
+			continue
+		}
+		if mtu == 0 {
+			// Record a zero-MTU candidate as a fallback, but keep searching
+			// for a non-zero MTU candidate.
+			if vcCandidateZero == nil {
+				vcCandidateZero = item
+			}
+			continue
+		}
+		// Prefer the first non-zero MTU candidate.
+		vcCandidateNonZero = item
+		break
+	}
+	// Choose the best available candidate: prefer non-zero MTU, otherwise
+	// fall back to a zero-MTU candidate (if any).
+	vcCandidate := vcCandidateNonZero
+	if vcCandidate == nil {
+		vcCandidate = vcCandidateZero
+	}

Conversation

votdev commented Feb 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants