Skip to content

[WIP] Improve workload creation in Felix FVs #11453

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mazdakn wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[WIP] Improve workload creation in Felix FVs #11453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion felix/fv/bpf_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1241,7 +1241,7 @@ func describeBPFTests(opts ...bpfTestOpt) bool {
infrastructure.AssignIP(wName, wIP, tc.Felixes[ii].Hostname, calicoClient)
}
w := workload.New(tc.Felixes[ii], wName, "default",
wIP, strconv.Itoa(port), testOpts.protocol)
wIP, workload.WithPort(strconv.Itoa(port)), workload.WithProtocol(testOpts.protocol))

labels["name"] = w.Name
labels["workload"] = "regular"
Expand Down
39 changes: 39 additions & 0 deletions felix/fv/infrastructure/infra_k8s.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import (
"sync"
"time"

. "github.com/onsi/gomega"

"github.com/davecgh/go-spew/spew"
"github.com/onsi/ginkgo"
"github.com/onsi/gomega"
Expand All @@ -40,14 +42,17 @@ import (

"github.com/projectcalico/calico/felix/fv/containers"
"github.com/projectcalico/calico/felix/fv/utils"
"github.com/projectcalico/calico/felix/fv/workload"
"github.com/projectcalico/calico/felix/ip"
"github.com/projectcalico/calico/libcalico-go/lib/apiconfig"
libapi "github.com/projectcalico/calico/libcalico-go/lib/apis/v3"
bapi "github.com/projectcalico/calico/libcalico-go/lib/backend/api"
"github.com/projectcalico/calico/libcalico-go/lib/backend/k8s/conversion"
"github.com/projectcalico/calico/libcalico-go/lib/backend/model"
client "github.com/projectcalico/calico/libcalico-go/lib/clientv3"
"github.com/projectcalico/calico/libcalico-go/lib/ipam"
"github.com/projectcalico/calico/libcalico-go/lib/names"
cnet "github.com/projectcalico/calico/libcalico-go/lib/net"
"github.com/projectcalico/calico/libcalico-go/lib/options"
)

Expand Down Expand Up @@ -804,6 +809,40 @@ func (kds *K8sDatastoreInfra) ensureNamespace(name string) {
}
}

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
}

Comment on lines +812 to +823
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copilot uses AI. Check for mistakes.
func (kds *K8sDatastoreInfra) CreateWorkload(
felix *Felix,
name string,
addr string,
opts ...workload.Opt,
) *workload.Workload {
if felix.TopologyOptions.UseIPPools {
// Assign the workload's IP in IPAM, this will trigger calculation of routes.
err := kds.calicoClient.IPAM().AssignIP(context.Background(), ipam.AssignIPArgs{
IP: cnet.MustParseIP(addr),
HandleID: &name,
Attrs: map[string]string{
ipam.AttributeNode: felix.Hostname,
},
Hostname: felix.Hostname,
})
ExpectWithOffset(1, err).NotTo(HaveOccurred())
}
w := workload.New(felix, name, "default", addr, opts...)
return w
}

func (kds *K8sDatastoreInfra) RemoveWorkload(ns, name string) error {
wepIDs, err := names.ParseWorkloadEndpointName(name)
if err != nil {
Expand Down
3 changes: 3 additions & 0 deletions felix/fv/infrastructure/infrastructure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
api "github.com/projectcalico/api/pkg/apis/projectcalico/v3"

"github.com/projectcalico/calico/felix/fv/utils"
"github.com/projectcalico/calico/felix/fv/workload"
libapi "github.com/projectcalico/calico/libcalico-go/lib/apis/v3"
client "github.com/projectcalico/calico/libcalico-go/lib/clientv3"
)
Expand Down Expand Up @@ -68,6 +69,8 @@ type DatastoreInfra interface {
// which the tunnel is created). needBGP is used (only in etcd) to
// 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
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
// AddWorkload will take the appropriate steps to create a workload in the
// datastore with the passed in wep values. If this succeeds then the
// *libapi.WorkloadEndpoint will be returned, otherwise an error will be
Expand Down
25 changes: 15 additions & 10 deletions felix/fv/tiered_policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,24 +101,29 @@ var _ = infrastructure.DatastoreDescribe("connectivity tests and flow logs with
infra.AddDefaultAllow()

// Create workload on host 1.
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")
//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")
Comment on lines +104 to +125
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
ep2_4 = infra.CreateWorkload(tc.Felixes[1], "ep2-4", "10.65.1.4")
Comment on lines +124 to +126
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
ep2_4.ConfigureInInfra(infra)

ensureRoutesProgrammed(tc.Felixes)
Expand Down
18 changes: 15 additions & 3 deletions felix/fv/workload/workload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,19 @@ func WithHostNetworked() Opt {
}
}

func New(c *infrastructure.Felix, name, profile, ip, ports, protocol string, opts ...Opt) *Workload {
func WithPort(ports string) Opt {
return func(w *Workload) {
w.Ports = ports
}
}
Comment on lines +187 to +191
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

func WithProtocol(proto string) Opt {
return func(w *Workload) {
w.Protocol = proto
}
}

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

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
workloadIdx++
n := fmt.Sprintf("%s-idx%v", name, workloadIdx)
interfaceName := conversion.NewConverter().VethNameForWorkload(profile, n)
Expand Down Expand Up @@ -218,8 +230,8 @@ func New(c *infrastructure.Felix, name, profile, ip, ports, protocol string, opt
SpoofName: spoofN,
InterfaceName: interfaceName,
SpoofInterfaceName: spoofIfaceName,
Ports: ports,
Protocol: protocol,
Ports: "8055",
Protocol: "tcp",
WorkloadEndpoint: wep,
MTU: defaultMTU,
}
Expand Down
Loading