e2e: allow using external containers #1604

liornoy · 2022-09-12T14:33:19Z

This commit adds the option to run e2etests with existing containers, i.e. the tests don't create the external FRR containers, but use existing ones. see e2etest/README.md for more information.

Extra general code gardening:

On e2etest_suite_test.go - Deleted redundant assertion.
On container.go - Renamed function "Stop" to "Delete" to reflect better what it does.
On container.go - Deleted redundant "todo" comment.

fedepaol · 2022-09-13T12:39:07Z

e2etest/bgptests/infra_setup.go

 		hostIPv6 = ip
 	}
+
+	if _, res := os.LookupEnv("USE_IBGP_SINGLE_HOP"); res {


Given the name of the containers must follow our convention, how about accepting a comma-separated list of external containers instead of a list of bools as environment variables? We could have a validation over that list (to make sure no wrong parameters were passed), have a func that returns the configs given that list, and use it to feed UseExisting

And in general, I'd prefer having a flag over an env variable.

done. changed to flags. added the required changes to tasks.py as well.

fedepaol · 2022-09-13T12:40:27Z

e2etest/bgptests/infra_setup.go

+
+// multiHopSetUp connects the ebgp-single-hop container to the multi-hop-net network,
+// and creates the required static routes between the multi-hop containers and the speaker pods.
+func multiHopSetUp(externalContainers []*frrcontainer.FRR, cs *clientset.Clientset) error {


externalContainers -> containers, as it can be called also on existing containers

fedepaol · 2022-09-13T12:41:24Z

e2etest/bgptests/infra_setup.go

+}
+
+func multiHopTearDown(cs *clientset.Clientset) error {
+	if containersNetwork == "host" || !isMultiHop {


I'd keep these outside, as the purpouse of the function is to "tear down the multihop" and not to "tear it down when..."

Also, can we make this more robust and just not do nothing if the network is not there? That will allow us to get rid of the global isMultiHop

fedepaol · 2022-09-13T12:51:41Z

e2etest/pkg/frr/container/container.go

+}
+
+// validateContainerIsRunning validates that the given container is up and running.
+func validateContainerIsRunning(containerName string) error {


just containerIsRunning ?

fedepaol · 2022-09-13T12:52:38Z

e2etest/pkg/frr/container/container.go

+
+// UseExisting validates that the existing frr containers that correspond to the
+// given configurations are up and running, and returns the corresponding *FRRs.
+func UseExisting(cs *clientset.Clientset, c ...Config) ([]*FRR, error) {


cs is not used here I think?

you're right. removed. thanks!

fedepaol · 2022-09-13T12:53:16Z

e2etest/bgptests/infra_setup.go

+			if !useEBGPSingleHop {
+				return nil, fmt.Errorf("Failed to setup test infra. Must use ebgp-single-hop container for multi-hop.")
+			}
+			err = multiHopSetUp(res, cs)


if this errs, just leave

fedepaol · 2022-09-13T12:53:23Z

e2etest/bgptests/infra_setup.go

-			return nil, errors.Wrapf(err, "failed to connect %s to %s: %s", ebgpSingleHopContainerConfig.Name, multiHopNetwork, out)
-		}
+		isMultiHop = true
+		err = multiHopSetUp(res, cs)


fedepaol · 2022-09-13T12:54:37Z

e2etest/pkg/frr/container/container.go

+
+// UseExisting validates that the existing frr containers that correspond to the
+// given configurations are up and running, and returns the corresponding *FRRs.
+func UseExisting(cs *clientset.Clientset, c ...Config) ([]*FRR, error) {


this is a public API, let's put it on the top of the file

Also, this is more ConfigureExisting

fedepaol · 2022-09-13T12:59:51Z

e2etest/pkg/frr/container/container.go

+		return nil, fmt.Errorf("Failed to start container %s. %w", cfg.Name, err)
 	}

+	frr, err := configureContainer(cfg, configDir)


here I'd get rid of

configDir, err := ioutil.TempDir("", "frr-conf")

add it to startContainer and remove start() entirely.
It feels confusing to have both start and startContainer

fedepaol · 2022-09-13T14:46:28Z

Looks really nice! I left a few comments, but overall seems great!

e2etest/bgptests/infra_setup.go

fedepaol · 2022-09-22T14:18:56Z

e2etest/bgptests/infra_setup.go

-			ASN:      metalLBASN,
-			Password: "ibgp-test",
-			MultiHop: false,
+	frrContainers := []frrcontainer.Config{


why the need to use a slice here? If it's only for convenience of using the slice in config.For, then create a local array.

I reverted to the way it was before without a slice

fedepaol · 2022-09-22T14:19:38Z

e2etest/bgptests/infra_setup.go

 // InfraSetup brings up the external container mimicking external routers, and set up the routing needed for
 // testing.
-func InfraSetup(ipv4Addresses, ipv6Addresses []string, cs *clientset.Clientset) ([]*frrcontainer.FRR, error) {
+func InfraSetup(ipv4Addresses, ipv6Addresses []string, containers string, cs *clientset.Clientset) ([]*frrcontainer.FRR, error) {


nit: externalContainers / existingContainers

fedepaol · 2022-09-22T14:22:03Z

e2etest/bgptests/infra_setup.go

+			}
+		}
+	} else if containersNetwork == "host" {
+		res, err = frrcontainer.Create(frrContainers[ibgpSingleHopContainerConfig])


see the comment above about the slice. It's clearer if you refer the container by name (we could rename the index as something like ...Index, but I still think there's no value in converting the single instances to a slice)

fedepaol · 2022-09-22T14:23:00Z

tasks.py

+    "containers": "a comma separated list of containers names to use for the test. (valid parameters are: ibgp-single-hop / ibgp-multi-hop / ebgp-single-hop / ebgp-multi-hop)",
 })
-def e2etest(ctx, name="kind", export=None, kubeconfig=None, system_namespaces="kube-system,metallb-system", service_pod_port=80, skip_docker=False, focus="", skip="", ipv4_service_range=None, ipv6_service_range=None, prometheus_namespace="", node_nics="kind", local_nics="kind"):
+def e2etest(ctx, name="kind", export=None, kubeconfig=None, system_namespaces="kube-system,metallb-system", service_pod_port=80, skip_docker=False, focus="", skip="", ipv4_service_range=None, ipv6_service_range=None, prometheus_namespace="", node_nics="kind", local_nics="kind", containers=""):


same: external_containers / external_peers

done. changed to external_containers

This commit adds the option to run e2etests with existing containers, i.e. the tests don't create the external FRR containers, but use existing. see e2etest/README.md for more information. Extra general code gardening: * On e2etest_suite_test.go - Deleted redundant assertion. * On container.go - Renamed function "Stop" to "Delete" to reflect better what it does. * On container.go - Deleted redundant "todo" comment.

fedepaol · 2022-09-23T14:15:02Z

LGTM thanks!

liornoy requested review from fedepaol, gclawes, johananl, oribon, rata and russellb as code owners September 12, 2022 14:33

fedepaol reviewed Sep 13, 2022

View reviewed changes

liornoy force-pushed the e2etestmodify branch 3 times, most recently from c0a755f to 628e01d Compare September 22, 2022 12:20

fedepaol reviewed Sep 22, 2022

View reviewed changes

liornoy force-pushed the e2etestmodify branch from 628e01d to 2f213df Compare September 23, 2022 14:13

fedepaol merged commit 44a4ba2 into metallb:main Sep 24, 2022

liornoy deleted the e2etestmodify branch September 25, 2022 09:17

cprivitere mentioned this pull request Oct 12, 2022

Add MetalLB 0.13 support kubernetes-sigs/cloud-provider-equinix-metal#322

Closed

eyenx mentioned this pull request Oct 19, 2022

chore(misc/metallb): update to 0.13.x adfinis/helm-charts#834

Merged

6 tasks

e2e: allow using external containers #1604

e2e: allow using external containers #1604

Uh oh!

Conversation

liornoy commented Sep 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedepaol Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liornoy Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liornoy Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedepaol commented Sep 13, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedepaol commented Sep 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

fedepaol Sep 13, 2022 •

edited

Loading

liornoy Sep 21, 2022 •

edited

Loading

liornoy Sep 21, 2022 •

edited

Loading