Remove kube2e dep from test/kubernetes/e2e #10587

jenshu · 2025-02-06T19:55:15Z

Some more testing-related cleanup - removed test/kubernetes/e2e's dependence on test/kube2e helpers (that way when we re-enable the tests under test/kubernetes/e2e we don't need to also re-enable the kube2e ones to get it to compile). See comments for details.

jenshu · 2025-02-06T20:04:21Z

install/helm/kgateway/templates/_helpers.tpl

@@ -46,6 +46,7 @@ app.kubernetes.io/managed-by: {{ .Release.Service }}
 Selector labels
 */}}
 {{- define "kgateway.selectorLabels" -}}
+kgateway: kgateway


added this because test/helpers/kube_dump.go looks for pods with label gloo=gloo and we didn't yet have a standard/hardcoded label that's added to all the kgateway pods (kgateway.name can be overridden)

I think I'd rather see us adopt the common labels instead of mirroring the existing pattern. We can address this in a follow-up as long as it's tracked though.

we are using common labels but they don't currently have hardcoded values that can be queried for the log dump. maybe we should change app.kubernetes.io/name to kgateway? i can add a tracking item somewhere

added #10591 as a sub-task of #10441

jenshu · 2025-02-06T20:04:45Z

pkg/utils/controllerutils/admincli/client_test.go

@@ -7,7 +7,7 @@ import (
 	. "github.com/onsi/gomega"
 	"github.com/solo-io/go-utils/threadsafe"

-	"github.com/kgateway-dev/kgateway/pkg/utils/glooadminutils/admincli"
+	"github.com/kgateway-dev/kgateway/pkg/utils/controllerutils/admincli"


renamed pkg to remove the gloo ref

jenshu · 2025-02-06T20:05:04Z

test/testutils/env.go

@@ -23,9 +23,6 @@ const (
 	// test framework does not need to install/uninstall Istio.
 	SkipIstioInstall = "SKIP_ISTIO_INSTALL"

-	// KubeTestType is used to indicate which kube2e suite should be run while executing regression tests
-	KubeTestType = "KUBE2E_TESTS"


jenshu · 2025-02-06T20:07:35Z

test/kube2e/helper/curl.go

@@ -80,21 +79,6 @@ var (
 	DefaultCurlPollingTimeout = time.Second * 2  // DefaultCurlPollingTimeout is the default pollinginterval for "Eventually" curl assertions
 )

-var getTimeoutsAsInterfaces = helpers.GetDefaultTimingsTransform(DefaultCurlTimeout, DefaultCurlPollingTimeout)
-
-func GetTimeouts(timeout ...time.Duration) (currentTimeout, pollingInterval time.Duration) {


moved these out into test/helpers/timeouts.go

jenshu · 2025-02-06T20:08:31Z

test/helpers/util.go

@@ -58,87 +56,3 @@ func PercentileIndex(length, pct int) int {

 	return int(math.Ceil(float64(length)*(float64(pct)/float64(100)))) - 1
 }
-
-// GetDefaultEventuallyTimeoutsTransform returns timeout and polling interval values to use with a gomega eventually call


moved these into test/helpers/timeouts.go to have all the timeout-related stuff in one file

jenshu · 2025-02-06T20:09:11Z

test/helpers/timeouts.go

@@ -0,0 +1,115 @@
+//go:build ignore
+
+package helpers


helper functions moved from the other 2 files

jenshu · 2025-02-06T20:09:59Z

test/helpers/kube_dump.go

@@ -412,20 +395,14 @@ func EnvoyDumpOnFail(ctx context.Context, kubectlCli *kubectl.Cli, _ io.Writer,
 		for _, ns := range namespaces {
 			proxies := []string{}

+			// TODO need to get the right label, and pass in the Gateway namespaces


we can address this in a follow-up; need a way to select all the proxy pods

Are we planning on adding a line item to that e2e epic you linked?

good call, created #10592

jenshu · 2025-02-06T20:11:48Z

test/helpers/kube_dump.go

 	defer f.Close()
-	kubeCli := &install.CmdKubectl{}


CmdKubectl had already been deleted in a previous cleanup (and it had been deprecated), so changed this to use kubectl.Cli

jenshu · 2025-02-06T20:12:49Z

test/helpers/directories.go

@@ -1,56 +0,0 @@
-//go:build ignore
-
-package helpers


not used anywhere

jenshu · 2025-02-06T20:13:30Z

test/README.md

-
-## Performance tests
-
-Our tests include some performance tests which variably guard against regressions in performance or validate decisions made to choose one algorithm over others.


i believe these tests were deleted

I was under the impression we disabled the suite but didn't delete the suite. At least, that's what

kgateway/.github/workflows/nightly-tests.yaml

Lines 252 to 262 in 90f2a15

# performance_tests_main:

# name: main performance tests

# if: ${{ (github.event_name == 'workflow_dispatch' && inputs.run-performance && inputs.branch == 'main') || github.event.schedule == '0 5 * * *' }}

# runs-on: ubuntu-22.04

# timeout-minutes: 60

# steps:

# - uses: actions/checkout@v4

# with:

# ref: main

# - uses: ./.github/actions/prep-go-runner

# - uses: ./.github/actions/performance-tests

was implying.

i couldn't find any tests labeled with performance / Performance or any calls to the Measure functions mentioned in the readme (although i could have missed it, can you double check?)

timflannagan · 2025-02-06T21:25:45Z

test/helpers/kube_dump.go

 	controllerLogsFile := fileAtPath(filepath.Join(outDir, fmt.Sprintf("%s.controller.log", podName)))
 	controllerLogsCmd := kubectlCli.WithReceiver(controllerLogsFile).Command(ctx,
-		"-n", ns, "logs", podName, "-c", "gloo", "--tail=1000")
+		"-n", ns, "logs", podName, "-c", "kgateway", "--tail=1000")


There's the GlooDeploymentName constant we can leverage here as well instead of hardcoding the pod/deployment name and risking drift over time. I don't remember why I held off on renaming that constant when I last touched that file.

good call, 522130a added a const for the container name, but i can do another cleanup pass with the naming in the next PR

timflannagan · 2025-02-06T21:27:36Z

test/helpers/kube_dump.go

-// StandardGlooDumpOnFail creates adump of the kubernetes state and certain envoy data from
+// StandardKgatewayDumpOnFail creates a dump of the kubernetes state and certain envoy data from


nit: I'm less familiar with this test code -- I think my preference is to rip out the project name/branding from the function entirely unless there's any conflicts with existing util function names.

there are a few other functions in here - KubeDumpOnFail, ControllerDumpOnFail, EnvoyDumpOnFail and StandardKgatewayDumpOnFail calls all of them. I don't see anything outside of this file calling these other 3 functions, so what about:

renaming StandardKgatewayDumpOnFail to StandardDumpOnFail?

making KubeDumpOnFail, ControllerDumpOnFail, EnvoyDumpOnFail non-exported

That's fun. Nothing mission critical here -- let's revisit this if we have time once we're closer to the finish line for this stream of work.

timflannagan · 2025-02-06T21:28:40Z

test/helpers/kube_dump.go

@@ -65,7 +64,7 @@ func KubeDumpOnFail(ctx context.Context, kubectlCli *kubectl.Cli, outLog io.Writ

 		recordDockerState(fileAtPath(filepath.Join(outDir, "docker-state.log")))
 		recordProcessState(fileAtPath(filepath.Join(outDir, "process-state.log")))
-		recordKubeState(fileAtPath(filepath.Join(outDir, "kube-state.log")))
+		recordKubeState(ctx, kubectlCli, fileAtPath(filepath.Join(outDir, "kube-state.log")))


Just a heads up that line 59/60 has solo.io reference, but that's an existing problem with this code and we can fix that in #10584.

good catch, we can do a sweep of everything when we pick up that cleanup task you linked

jenshu added 12 commits February 4, 2025 14:16

cleanup

6b1e2f0

add back readme section

c37cf8c

delete unused file

236a74a

add back ignores

3e87754

remove refs to ResourceClients

c076610

remove unused file

1f4b7aa

Merge branch 'main' of github.com:jenshu/kgateway into remove-kube2e-dep

ec72ce5

change some gloo refs

496f69d

Merge branch 'main' of github.com:jenshu/kgateway into remove-kube2e-dep

5dedab3

update readme

3597cb5

remove unused var

e0a0c78

move timeouts helper

ccdebbf

jenshu commented Feb 6, 2025

View reviewed changes

format

267a8ed

jenshu mentioned this pull request Feb 5, 2025

e2e testing: get tests to compile/run #10522

Open

2 tasks

timflannagan reviewed Feb 6, 2025

View reviewed changes

timflannagan approved these changes Feb 6, 2025

View reviewed changes

Merge branch 'main' of github.com:jenshu/kgateway into remove-kube2e-dep

19f3db3

This was referenced Feb 7, 2025

Audit the kgateway helm chart labels #10591

Open

e2e testing: fix 'dump on fail' pod queries #10592

Open

const for container name

522130a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove kube2e dep from test/kubernetes/e2e #10587

Remove kube2e dep from test/kubernetes/e2e #10587

jenshu commented Feb 6, 2025 •

edited

Loading

jenshu Feb 6, 2025

timflannagan Feb 6, 2025

jenshu Feb 7, 2025

jenshu Feb 7, 2025

jenshu Feb 6, 2025

jenshu Feb 6, 2025

jenshu Feb 6, 2025

jenshu Feb 6, 2025

jenshu Feb 6, 2025

jenshu Feb 6, 2025

timflannagan Feb 6, 2025

jenshu Feb 7, 2025

jenshu Feb 6, 2025

jenshu Feb 6, 2025

jenshu Feb 6, 2025

timflannagan Feb 6, 2025

jenshu Feb 7, 2025

timflannagan Feb 6, 2025

jenshu Feb 7, 2025

timflannagan Feb 6, 2025

jenshu Feb 7, 2025

timflannagan Feb 7, 2025

timflannagan Feb 6, 2025

jenshu Feb 7, 2025


		## Performance tests

		Our tests include some performance tests which variably guard against regressions in performance or validate decisions made to choose one algorithm over others.

	# performance_tests_main:
	# name: main performance tests
	# if: ${{ (github.event_name == 'workflow_dispatch' && inputs.run-performance && inputs.branch == 'main') \|\| github.event.schedule == '0 5 * * *' }}
	# runs-on: ubuntu-22.04
	# timeout-minutes: 60
	# steps:
	# - uses: actions/checkout@v4
	# with:
	# ref: main
	# - uses: ./.github/actions/prep-go-runner
	# - uses: ./.github/actions/performance-tests

		// StandardGlooDumpOnFail creates adump of the kubernetes state and certain envoy data from
		// StandardKgatewayDumpOnFail creates a dump of the kubernetes state and certain envoy data from

Remove kube2e dep from test/kubernetes/e2e #10587

Are you sure you want to change the base?

Remove kube2e dep from test/kubernetes/e2e #10587

Conversation

jenshu commented Feb 6, 2025 • edited Loading

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

jenshu commented Feb 6, 2025 •

edited

Loading