add-metallb-nodeselector-tc #347

gkopels · 2024-12-28T14:24:35Z

moving test cases from cnf-gotest to eco-gotest

evgenLevin · 2025-01-14T15:58:25Z

tests/cnf/core/network/metallb/internal/frr/frr.go

+
+// ValidateLocalPref verifies local pref from FRR is equal to configured Local Pref.
+func ValidateLocalPref(frrPod *pod.Builder, localPref uint32, ipFamily string) error {
+	var (


evgenLevin · 2025-01-14T16:00:18Z

tests/cnf/core/network/metallb/internal/frr/frr.go

+		err error
+	)
+
+	res, err = frrPod.ExecCommand(append(netparam.VtySh,


please use getBGPStatus func

evgenLevin · 2025-01-14T16:00:40Z

tests/cnf/core/network/metallb/internal/frr/frr.go

+		return fmt.Errorf("failed to parse route local preference %w", err)
+	}
+
+	for _, locPref := range toParse.Routes {


Suggested change

for _, locPref := range toParse.Routes {

for _, route := range toParse.Routes {

evgenLevin · 2025-01-14T16:01:16Z

tests/cnf/core/network/metallb/internal/frr/frr.go

+	}
+
+	for _, locPref := range toParse.Routes {
+		if locPref[0].LocalPref != localPref {


do we expect to validate only first route?

It only has one route

evgenLevin · 2025-01-14T16:01:28Z

tests/cnf/core/network/metallb/internal/frr/frr.go

+		}
+	}
+
+	return err


evgenLevin · 2025-01-14T16:54:47Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+				Expect(err).ToNot(HaveOccurred(), "Fail to validate local preference")
+
+				By("Validate BGP Community exists with the node selector")
+				output, err := frr.GetBGPCommunityStatus(frrPod0, noAdvertise, strings.ToLower(netparam.IPV4Family))


Suggested change

output, err := frr.GetBGPCommunityStatus(frrPod0, noAdvertise, strings.ToLower(netparam.IPV4Family))

bgpStatus, err := frr.GetBGPCommunityStatus(frrPod0, noAdvertise, strings.ToLower(netparam.IPV4Family))

evgenLevin · 2025-01-14T16:55:06Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+				Expect(len(output.Routes)).To(Equal(1))
+
+				By("Validate BGP Community does not exist without the node selector")
+				output, err = frr.GetBGPCommunityStatus(frrPod1, customCommunity, strings.ToLower(netparam.IPV4Family))


Suggested change

output, err = frr.GetBGPCommunityStatus(frrPod1, customCommunity, strings.ToLower(netparam.IPV4Family))

bgpStatus, err = frr.GetBGPCommunityStatus(frrPod1, customCommunity, strings.ToLower(netparam.IPV4Family))

evgenLevin · 2025-01-14T16:57:18Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+	_, err := metallb.NewBPGPeerBuilder(APIClient, name, NetConfig.MlbOperatorNamespace,
+		peerIP, tsparams.LocalBGPASN, tsparams.LocalBGPASN).WithPassword(tsparams.BGPPassword).Create()
+
+	return err


validate error here

evgenLevin · 2025-01-14T16:57:31Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+	return ipAddressPool
+}
+
+func createBGPPeer(name, peerIP string) error {


why don't use createBGPPeerAndVerifyIfItsReady?

evgenLevin · 2025-01-14T17:01:24Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+	setupMetalLbService("service-1", netparam.IPV4Family, ipAddressPool1, "Cluster")
+	setupMetalLbService("service-2", netparam.IPV4Family, ipAddressPool2, "Cluster")


is that expected that you create 2 the same services with different name in case single IP addressPool?

There are two different ipaddresspools

I'm talking about line 112
frrPod0, frrPod1 := setupTestCase(ipAddressPool, ipAddressPool, frrk8sPods)

Yes there is two services on a single IPaddressPool in the original test case but lets talk to make sure I understand your question.

evgenLevin · 2025-01-31T15:12:17Z

tests/cnf/core/network/metallb/internal/frr/frr.go

+	toParse, err := getBgpStatus(frrPod, fmt.Sprintf("show ip bgp %s json", ipFamily))
+
+	if err != nil {
+		return fmt.Errorf("failed to parse route local preference %w", err)
+	}
+
+	for _, route := range toParse.Routes {
+		if route[0].LocalPref != localPref {
+			return fmt.Errorf("incorrect localPref: %d", localPref)
+		}
+	}
+
+	return nil


Suggested change

toParse, err := getBgpStatus(frrPod, fmt.Sprintf("show ip bgp %s json", ipFamily))

if err != nil {

return fmt.Errorf("failed to parse route local preference %w", err)

}

for _, route := range toParse.Routes {

if route[0].LocalPref != localPref {

return fmt.Errorf("incorrect localPref: %d", localPref)

}

}

return nil

bgpStatus, err := getBgpStatus(frrPod, fmt.Sprintf("show ip bgp %s json", ipFamily))

if err != nil {

return fmt.Errorf("failed to get BGP status %w", err)

}

for _, route := range bgpStatus.Routes {

if route[0].LocalPref != localPref {

return fmt.Errorf("incorrect localPref: %d", localPref)

}

}

return nil

evgenLevin · 2025-01-31T15:12:56Z

tests/cnf/core/network/metallb/internal/frr/frr.go

+
+	for _, route := range toParse.Routes {
+		if route[0].LocalPref != localPref {
+			return fmt.Errorf("incorrect localPref: %d", localPref)


Please add what local pref we expected

evgenLevin · 2025-01-31T15:19:47Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+		frrk8sPods, err = pod.List(APIClient, NetConfig.MlbOperatorNamespace, metav1.ListOptions{
+			LabelSelector: frrNodeLabel,
+		})
+		Expect(err).ToNot(HaveOccurred(), "Failed to list speaker pods")


Suggested change

Expect(err).ToNot(HaveOccurred(), "Failed to list speaker pods")

Expect(err).ToNot(HaveOccurred(), "Failed to list frrk8s pods")

evgenLevin · 2025-01-31T15:22:38Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+	Context("Single IPAddressPool", func() {
+
+		var (
+			err error


evgenLevin · 2025-01-31T15:23:25Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+			err = metallbenv.CreateNewMetalLbDaemonSetAndWaitUntilItsRunning(tsparams.DefaultTimeout, workerLabelMap)
+			Expect(err).ToNot(HaveOccurred(), "Failed to recreate metalLb daemonset")
+
+			waitForWebHookRunning()


Suggested change

waitForWebHookRunning()

waitForFRRK8SWebHookRunning()

After I moved to general afterall no longer needed as a separate func. Removed

evgenLevin · 2025-01-31T15:39:12Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+				Expect(len(output.Routes)).To(Equal(1))
+
+				By("Validate BGP Community does not exist without the node selector")
+				output, err = frr.GetBGPCommunityStatus(frrPod1, tsparams.CustomCommunity, strings.ToLower(netparam.IPV4Family))


evgenLevin · 2025-01-31T15:40:23Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+				Expect(err).ToNot(HaveOccurred(), "Failed to collect bgp community status")
+				Expect(len(output.Routes)).To(Equal(1))
+
+				By("Validate BGP Community does not exist without the node selector")


How do we check that the comunity doesn't exist?

I changed the description. We can add a negitive check as well. But not sure it is needed. We are just validating the the correct community has been added to the route.

evgenLevin · 2025-01-31T15:43:13Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+	setupMetalLbService("service-1", netparam.IPV4Family, ipAddressPool1, "Cluster")
+	setupMetalLbService("service-2", netparam.IPV4Family, ipAddressPool2, "Cluster")


I'm talking about line 112
frrPod0, frrPod1 := setupTestCase(ipAddressPool, ipAddressPool, frrk8sPods)

evgenLevin · 2025-01-31T15:50:41Z

tests/cnf/core/network/metallb/internal/tsparams/consts.go

+	// BGPTestPeer is the bgppeer name.
+	BGPTestPeer = "testpeer"
+	// FrrK8WebHookServer is the web hook server running in namespace metallb-system.
+	FrrK8WebHookServer = "frr-k8s-webhook-server"


If you created the variable, please replace it everywhere it is used.

evgenLevin · 2025-01-31T15:52:36Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+			By("Creating a new instance of MetalLB Speakers on workers")
+			err = metallbenv.CreateNewMetalLbDaemonSetAndWaitUntilItsRunning(tsparams.DefaultTimeout, workerLabelMap)
+			Expect(err).ToNot(HaveOccurred(), "Failed to recreate metalLb daemonset")
+
+			waitForWebHookRunning()


Why do we need to wait specifically for the frrk8s webhook? If so, it should be a part of metallbenv.CreateNewMetalLbDaemonSetAndWaitUntilItsRunning()

I remember from the frrk8 test cases that the webhook was the took longer to come up. For those test cases I didnt add it to CreateNewMetalLbDaemonSetAndWaitUntilItsRunning but can.

The webhook is the last to come up. I added it to the CreateNewMetalLbDaemonSetAndWaitUntilItsRunning. Without the check it fails

evgenLevin · 2025-02-03T17:49:46Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+		nodeAddrList     []string
+		addressPool      []string
+		frrk8sPods       []*pod.Builder
+		frrNodeLabel     = "app=frr-k8s"


Moved to const.

evgenLevin · 2025-02-03T17:51:11Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+				_, err = frr.GetBGPCommunityStatus(frrPod1, tsparams.CustomCommunity,
+					strings.ToLower(netparam.IPV4Family))
+				Expect(err).ToNot(HaveOccurred(), "Failed to collect bgp community status")
+				Expect(len(bgpStatus.Routes)).To(Equal(2))


Suggested change

_, err = frr.GetBGPCommunityStatus(frrPod1, tsparams.CustomCommunity,

strings.ToLower(netparam.IPV4Family))

Expect(err).ToNot(HaveOccurred(), "Failed to collect bgp community status")

Expect(len(bgpStatus.Routes)).To(Equal(2))

bgpStatus, err = frr.GetBGPCommunityStatus(frrPod1, tsparams.CustomCommunity,

strings.ToLower(netparam.IPV4Family))

Expect(err).ToNot(HaveOccurred(), "Failed to collect bgp community status")

Expect(len(bgpStatus.Routes)).To(Equal(2))

updated to change

evgenLevin · 2025-02-03T17:51:56Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+				ipAddressPool1 := createIPAddressPool(ipaddressPoolName1, addressPool)
+				ipAddressPool2 := createIPAddressPool(ipaddressPoolName2, addressPool2)
+
+				frrPod0, frrPod1 := setupTestCase(ipAddressPool1, ipAddressPool2, frrk8sPods)


Please add By()

evgenLevin · 2025-02-03T17:52:09Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+				By("Create a single IPAddressPool")
+				ipAddressPool := createIPAddressPool(ipaddressPoolName1, addressPool)
+
+				frrPod0, frrPod1 := setupTestCase(ipAddressPool, ipAddressPool, frrk8sPods)


Please add By()

evgenLevin · 2025-02-03T17:53:56Z

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go

+	setupMetalLbService("service-1", netparam.IPV4Family, ipAddressPool1, "Cluster")
+	setupMetalLbService("service-2", netparam.IPV4Family, ipAddressPool2, "Cluster")


gkopels force-pushed the add-nodeselector-testcase branch 5 times, most recently from c92959e to 1019bfe Compare December 29, 2024 06:25

evgenLevin reviewed Jan 14, 2025

View reviewed changes

gkopels force-pushed the add-nodeselector-testcase branch 5 times, most recently from c225bdf to 876bc6e Compare January 30, 2025 15:26

evgenLevin reviewed Jan 31, 2025

View reviewed changes

gkopels force-pushed the add-nodeselector-testcase branch 4 times, most recently from 90eb7ab to 87b31d6 Compare February 1, 2025 19:32

evgenLevin reviewed Feb 3, 2025

View reviewed changes

gkopels force-pushed the add-nodeselector-testcase branch from 87b31d6 to c0bee71 Compare February 4, 2025 11:14

add-metallb-nodeselector-tc

b7c20ae

gkopels force-pushed the add-nodeselector-testcase branch from c0bee71 to b7c20ae Compare February 4, 2025 21:09

	for _, locPref := range toParse.Routes {
	for _, route := range toParse.Routes {

	output, err := frr.GetBGPCommunityStatus(frrPod0, noAdvertise, strings.ToLower(netparam.IPV4Family))
	bgpStatus, err := frr.GetBGPCommunityStatus(frrPod0, noAdvertise, strings.ToLower(netparam.IPV4Family))

	output, err = frr.GetBGPCommunityStatus(frrPod1, customCommunity, strings.ToLower(netparam.IPV4Family))
	bgpStatus, err = frr.GetBGPCommunityStatus(frrPod1, customCommunity, strings.ToLower(netparam.IPV4Family))

		setupMetalLbService("service-1", netparam.IPV4Family, ipAddressPool1, "Cluster")
		setupMetalLbService("service-2", netparam.IPV4Family, ipAddressPool2, "Cluster")

	Expect(err).ToNot(HaveOccurred(), "Failed to list speaker pods")
	Expect(err).ToNot(HaveOccurred(), "Failed to list frrk8s pods")

add-metallb-nodeselector-tc #347

Are you sure you want to change the base?

add-metallb-nodeselector-tc #347

Conversation

gkopels commented Dec 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment