[WIP] Improve workload creation in Felix FVs #11453

mazdakn · 2025-11-26T23:36:45Z

Description

Related issues/PRs

Todos

Tests
Documentation
Release note

Release Note

TBD

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

docs-pr-required: This change requires a change to the documentation that has not been completed yet.
docs-completed: This change has all necessary documentation completed.
docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

release-note-required: This PR has user-facing changes. Most PRs should have this label.
release-note-not-required: This PR has no user-facing changes.

Other optional labels:

cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

Copilot

Pull request overview

This WIP PR refactors workload creation in Felix functional verification (FV) tests to use an options pattern, improving API ergonomics and reducing parameter lists. The changes introduce WithPort() and WithProtocol() option functions and update the workload.New() signature to accept variadic options instead of explicit port/protocol parameters.

Key Changes

Refactored workload.New() to use options pattern with default values (port 8055, protocol tcp)
Added CreateWorkload() method to DatastoreInfra interface with K8s implementation
Migrated tiered_policy_test.go to use the new workload creation approach

Reviewed changes

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

Show a summary per file

File	Description
felix/fv/workload/workload.go	Added `WithPort()` and `WithProtocol()` option functions; changed `New()` signature to accept options instead of explicit parameters
felix/fv/infrastructure/infrastructure.go	Added `CreateWorkload()` method to `DatastoreInfra` interface
felix/fv/infrastructure/infra_k8s.go	Implemented `CreateWorkload()` for K8s datastore with IPAM integration; added helper `CreateWorkloadsPerHosts()`
felix/fv/tiered_policy_test.go	Migrated workload creation to use new `infra.CreateWorkload()` helper; updated IP for ep2_4 from 10.65.1.3 to 10.65.1.4
felix/fv/bpf_test.go	Updated one workload creation call to use new options pattern

Copilot · 2025-11-26T23:46:57Z

felix/fv/workload/workload.go

+func WithPort(ports string) Opt {
+	return func(w *Workload) {
+		w.Ports = ports
+	}
+}


Function name inconsistency: this function is named WithPort (singular) but sets the Ports field (plural). For consistency with the field name and common patterns in similar code, consider renaming to WithPorts to match the plural field name.

Copilot · 2025-11-26T23:46:58Z

felix/fv/workload/workload.go

+	}
+}
+
+func New(c *infrastructure.Felix, name, profile, ip string, opts ...Opt) *Workload {


The signature of New() has been changed to remove ports and protocol parameters, but the internal run() function (line 251-258) and the exported Run() function (line 148-157) still use the old signature. This will cause compilation errors when workload.Run() is called, as it will pass 5 parameters to New() which now only accepts 4. Either update run() and Run() to use the new options pattern, or consider this change as breaking existing code that uses Run().

Copilot · 2025-11-26T23:46:58Z

felix/fv/tiered_policy_test.go

+		//infrastructure.AssignIP("ep1-1", "10.65.0.0", tc.Felixes[0].Hostname, client)
+		//ep1_1 = workload.Run(tc.Felixes[0], "ep1-1", "default", "10.65.0.0", wepPortStr, "tcp")
+		ep1_1 = infra.CreateWorkload(tc.Felixes[0], "ep1-1", "10.65.0.0")
 		ep1_1.ConfigureInInfra(infra)

-		infrastructure.AssignIP("ep2-1", "10.65.1.0", tc.Felixes[1].Hostname, client)
-		ep2_1 = workload.Run(tc.Felixes[1], "ep2-1", "default", "10.65.1.0", wepPortStr, "tcp")
+		//infrastructure.AssignIP("ep2-1", "10.65.1.0", tc.Felixes[1].Hostname, client)
+		//ep2_1 = workload.Run(tc.Felixes[1], "ep2-1", "default", "10.65.1.0", wepPortStr, "tcp")
+		ep2_1 = infra.CreateWorkload(tc.Felixes[1], "ep2-1", "10.65.1.0")
 		ep2_1.ConfigureInInfra(infra)

-		infrastructure.AssignIP("ep2-2", "10.65.1.1", tc.Felixes[1].Hostname, client)
-		ep2_2 = workload.Run(tc.Felixes[1], "ep2-2", "default", "10.65.1.1", wepPortStr, "tcp")
+		//infrastructure.AssignIP("ep2-2", "10.65.1.1", tc.Felixes[1].Hostname, client)
+		//ep2_2 = workload.Run(tc.Felixes[1], "ep2-2", "default", "10.65.1.1", wepPortStr, "tcp")
+		ep2_2 = infra.CreateWorkload(tc.Felixes[1], "ep2-2", "10.65.1.1")
 		ep2_2.ConfigureInInfra(infra)

-		infrastructure.AssignIP("ep2-3", "10.65.1.2", tc.Felixes[1].Hostname, client)
-		ep2_3 = workload.Run(tc.Felixes[1], "ep2-3", "default", "10.65.1.2", wepPortStr, "tcp")
+		//infrastructure.AssignIP("ep2-3", "10.65.1.2", tc.Felixes[1].Hostname, client)
+		//ep2_3 = workload.Run(tc.Felixes[1], "ep2-3", "default", "10.65.1.2", wepPortStr, "tcp")
+		ep2_3 = infra.CreateWorkload(tc.Felixes[1], "ep2-3", "10.65.1.2")
 		ep2_3.ConfigureInInfra(infra)

-		infrastructure.AssignIP("ep2-3", "10.65.1.3", tc.Felixes[1].Hostname, client)
-		ep2_4 = workload.Run(tc.Felixes[1], "ep2-4", "default", "10.65.1.3", wepPortStr, "tcp")
+		//infrastructure.AssignIP("ep2-3", "10.65.1.3", tc.Felixes[1].Hostname, client)
+		//ep2_4 = workload.Run(tc.Felixes[1], "ep2-4", "default", "10.65.1.3", wepPortStr, "tcp")


These commented-out lines should be removed rather than left in the code. Keeping old implementation as comments clutters the codebase and makes it harder to maintain. If this code needs to be preserved for reference, it should be documented in the commit history or PR description.

Copilot · 2025-11-26T23:46:58Z

felix/fv/tiered_policy_test.go

+		//infrastructure.AssignIP("ep2-3", "10.65.1.3", tc.Felixes[1].Hostname, client)
+		//ep2_4 = workload.Run(tc.Felixes[1], "ep2-4", "default", "10.65.1.3", wepPortStr, "tcp")
+		ep2_4 = infra.CreateWorkload(tc.Felixes[1], "ep2-4", "10.65.1.4")


IP address inconsistency: The new code assigns IP "10.65.1.4" to ep2_4, but the commented-out code on line 125 shows it was previously "10.65.1.3". If this is an intentional fix, the change should be documented. If not, this could break existing tests that rely on ep2_4 having IP 10.65.1.3. Additionally, the comment on line 124 incorrectly refers to "ep2-3" when it should say "ep2-4".

Suggested change

//infrastructure.AssignIP("ep2-3", "10.65.1.3", tc.Felixes[1].Hostname, client)

//ep2_4 = workload.Run(tc.Felixes[1], "ep2-4", "default", "10.65.1.3", wepPortStr, "tcp")

ep2_4 = infra.CreateWorkload(tc.Felixes[1], "ep2-4", "10.65.1.4")

//infrastructure.AssignIP("ep2-4", "10.65.1.3", tc.Felixes[1].Hostname, client)

//ep2_4 = workload.Run(tc.Felixes[1], "ep2-4", "default", "10.65.1.3", wepPortStr, "tcp")

ep2_4 = infra.CreateWorkload(tc.Felixes[1], "ep2-4", "10.65.1.3")

Copilot · 2025-11-26T23:46:59Z

felix/fv/infrastructure/infrastructure.go

 	// add a NodeBGPSpec if true or otherwise not.
 	AddNode(felix *Felix, v4CIDR *net.IPNet, v6CIDR *net.IPNet, idx int, needBGP bool)
+
+	CreateWorkload(felix *Felix, name, addr string, opts ...workload.Opt) *workload.Workload


The CreateWorkload method is being added to the DatastoreInfra interface but appears to only be implemented for K8s datastore. This will cause compilation errors for the etcd datastore implementation (EtcdDatastoreInfra in infra_etcd.go) which doesn't implement this method. Either implement this method for all datastores that implement this interface, or reconsider whether this should be part of the common interface.

Suggested change

CreateWorkload(felix *Felix, name, addr string, opts ...workload.Opt) *workload.Workload

Copilot · 2025-11-26T23:46:59Z

felix/fv/infrastructure/infra_k8s.go

+func (kds *K8sDatastoreInfra) CreateWorkloadsPerHosts(numOfWorkloads int, opts ...workload.Opt) []*workload.Workload {
+	var workloads []*workload.Workload
+	for fi, felix := range kds.felixes {
+		for i := range numOfWorkloads {
+			wName := fmt.Sprintf("w%d-felix%d", i, fi)
+			wIP := fmt.Sprintf("10.65.%d.%d", fi, i)
+			workloads = append(workloads, kds.CreateWorkload(felix, wName, wIP, opts...))
+		}
+	}
+	return workloads
+}
+


The CreateWorkloadsPerHosts function is added but never used in this PR and is not part of the DatastoreInfra interface. Consider removing it if it's not needed, or document its intended use case. If it's intended for future use, consider adding a comment explaining its purpose.

Suggested change

func (kds *K8sDatastoreInfra) CreateWorkloadsPerHosts(numOfWorkloads int, opts ...workload.Opt) []*workload.Workload {

var workloads []*workload.Workload

for fi, felix := range kds.felixes {

for i := range numOfWorkloads {

wName := fmt.Sprintf("w%d-felix%d", i, fi)

wIP := fmt.Sprintf("10.65.%d.%d", fi, i)

workloads = append(workloads, kds.CreateWorkload(felix, wName, wIP, opts...))

}

}

return workloads

}

fist commit

af25e18

Copilot AI review requested due to automatic review settings November 26, 2025 23:36

mazdakn requested a review from a team as a code owner November 26, 2025 23:36

marvin-tigera added this to the Calico v3.32.0 milestone Nov 26, 2025

marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Nov 26, 2025

Copilot started reviewing on behalf of mazdakn November 26, 2025 23:37 View session

Copilot finished reviewing on behalf of mazdakn November 26, 2025 23:40

Copilot AI reviewed Nov 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] Improve workload creation in Felix FVs #11453

[WIP] Improve workload creation in Felix FVs #11453

mazdakn commented Nov 26, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[WIP] Improve workload creation in Felix FVs #11453

Are you sure you want to change the base?

[WIP] Improve workload creation in Felix FVs #11453

Conversation

mazdakn commented Nov 26, 2025

Description

Related issues/PRs

Todos

Release Note

Reminder for the reviewer

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants