Collect Gloo metrics and some snapshots on test failure #10400

ryanrolds · 2024-11-23T01:34:46Z

Description

I've encountered a couple of cases where this information would have been helpful in debugging

Adds collection of Gloo controller metrics, xds snapshot, and krt snapshot

Interesting decisions

I wanted to use the ephemeral debug containers and had to first fetch the pod name because
kubectl debug doesn't support deployment/gloo as a pod.

Testing steps

Take a look at the artifacts found this failed test.

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works

ryanrolds · 2024-11-23T01:35:27Z

test/kubernetes/e2e/features/virtualhost_options/vhost_opt_suite.go

@@ -225,6 +225,8 @@ func (s *testingSuite) TestConfigureVirtualHostOptionsWithSectionNameManualSetup
 		[]string{"conflict with more specific or older VirtualHostOptions"},
 		defaults.KubeGatewayReporter,
 	)
+
+	s.Assert().Equal(true, false, "intentionally failing to trigger drump, remove when done debugging")


I will be removing this after I have an artifact example to link.

ryanrolds · 2024-11-23T01:36:14Z

test/kubernetes/e2e/test.go

+	// Fetch the name of the Gloo Gateway controller pod
+	getGlooPodNameCmd := i.Actions.Kubectl().Command(ctx, "get", "pod", "-n", i.Metadata.InstallNamespace,
+		"--selector", "gloo=gloo", "--output", "jsonpath='{.items[0].metadata.name}'")
+	_ = getGlooPodNameCmd.WithStdout(podStdOut).WithStderr(podStdErr).Run()
+
+	// Clean up and check the output
+	glooPodName := strings.Trim(podStdOut.String(), "'")
+	if glooPodName == "" {
+		fmt.Printf("Failed to get the name of the Gloo Gateway controller pod: %s\n", podStdErr.String())
+		return
+	}


Happy to make this it's own method if we think we will need it more than in this case.

github-actions · 2024-11-23T01:55:13Z

Visit the preview URL for this PR (updated for commit 34ee46c):

https://gloo-edge--pr10400-rolds-test-failure-c-kkld9hrb.web.app

_{(expires Tue, 03 Dec 2024 18:59:38 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 77c2b86e287749579b7ff9cadb81e099042ef677}

nfuden · 2024-11-23T02:51:57Z

test/kubernetes/e2e/test.go

+	// Using an ephemeral debug pod fetch the metrics from the Gloo Gateway controller
+	metricsCmd := i.Actions.Kubectl().Command(ctx, "debug", "-n", i.Metadata.InstallNamespace,
+		"-it", "--image=curlimages/curl:7.83.1", glooPodName, "--",
+		"curl", "http://localhost:9091/metrics")
+	_ = metricsCmd.WithStdout(metricsFile).WithStderr(metricsFile).Run()
+	metricsFile.Close()
+
+	// Get krt snapshot from the Gloo Gateway controller pod and write it to a file
+	krtSnapshotFilePath := filepath.Join(failureDir, "krt_snapshot.log")
+	krtSnapshotFile, err := os.OpenFile(krtSnapshotFilePath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, os.ModePerm)
+	i.Assertions.Require.NoError(err)
+
+	// Using an ephemeral debug pod fetch the krt snapshot from the Gloo Gateway controller
+	krtSnapshotCmd := i.Actions.Kubectl().Command(ctx, "debug", "-n", i.Metadata.InstallNamespace,
+		"-it", "--image=curlimages/curl:7.83.1", glooPodName, "--",
+		"curl", "http://localhost:9095/snapshots/krt")
+	_ = krtSnapshotCmd.WithStdout(krtSnapshotFile).WithStderr(krtSnapshotFile).Run()
+	krtSnapshotFile.Close()


while these make sense it would be ideal if we could add them to the main admin gloo endpoint tooling as its a pretty general case. I know its annoying to do it on a first pass (ie one case) but Im pretty sure I would love to have it on all cases

https://github.com/solo-io/gloo/blob/main/test/helpers/kube_dump.go#L46 is an example of our end dump and it includes hitting the stats endpoint like https://github.com/solo-io/gloo/blob/main/test/helpers/kube_dump.go#L311 which would be a nice setup that we can add to these types of tests

…b.com/solo-io/gloo into rolds/test_failure_collect_metrics_dump

andy-fong · 2024-11-26T15:11:16Z

test/kubernetes/e2e/test.go

+		"-it", "--image=curlimages/curl:7.83.1", glooPodName, "--",
+		"curl", "http://localhost:9091/metrics")
+	cmdErr = metricsCmd.WithStdout(metricsFile).WithStderr(metricsFile).Run()
+	if cmdErr != nil {


nit: probably doesn't need the if check

I tried it without and it was always being treated at a failure.

I think it doesn't like something about it explicitly being a pointer, but I'm not sure and didn't dig in.

sam-heilbron

looking good!

sam-heilbron · 2024-11-26T15:20:16Z

test/kubernetes/e2e/test.go

+	podStdOut := bytes.NewBuffer(nil)
+	podStdErr := bytes.NewBuffer(nil)
+
+	// Fetch the name of the Gloo Gateway controller pod


I like it! I had written up some thoughts around this in https://github.com/solo-io/solo-projects/issues/7298. I think what you've added is a great start, though I wonder if we can iterate by making some of these actions (capturing details about a cluster, and dumping them in files) availabe to users as well in the CLI. That way improvements we build to debug our tests, also benefit customers debugging their environment.

Also, this work seems to build off of k8sgateway#10239. I would take a look at that PR and see what we can do to avoid duplicate functionality

Making it easier for customers to get snapshots and other useful debugging information is a good idea. My goal of this PR is to ensure that we/engs can more easily debug test failures in the short-term.

I agree with unifying the code for getting the snapshots between our test frameworks, which if done correctly should make the user face work easier to do later. I will get the framework unification work done in this PR.

I would like to defer any user facing work to future feature tickets.

Makes sense to me! More wanted to point out the existing user-facing work as something we could piggy back off of, since it does some of what you are trying to do here. I'm good with taking the phased approach of enabling devs first, and then we can work to enable users after

sam-heilbron · 2024-11-26T15:22:19Z

test/kubernetes/e2e/test.go

+	i.Assertions.Require.NoError(err)
+
+	// Using an ephemeral debug pod fetch the metrics from the Gloo Gateway controller
+	metricsCmd := i.Actions.Kubectl().Command(ctx, "debug", "-n", i.Metadata.InstallNamespace,


This seems to have an overlap with https://github.com/solo-io/gloo/blob/main/test/kube2e/helper/curl.go. It might be nice if we could align those two

sam-heilbron · 2024-11-26T15:23:13Z

test/kubernetes/e2e/test.go

+	}()
+
+	adminClient := admincli.NewClient().
+		WithReceiver(io.Discard). // adminAssertion can overwrite this


nit: I don't think this comment is relevant. It seems related to tests that perform assertions with the adminClient

sam-heilbron · 2024-11-26T15:24:05Z

test/kubernetes/e2e/test.go

+		WithStderr(krtSnapshotFile).
+		Run()
+	if cmdErr != nil {
+		i.Assertions.Require.NoError(cmdErr)


What is the behavior of our pre-fail handler if this assertion fails? Does it cause the action to short-circuit, or just ensure that everything makes a best attempt and then we fail the job?

Which would you like it to do?

My default position is to fail fast and fail often. So, I think if we are unable to generate snapshots that we should fail promptly, but I suspect there are cases where a test fails and the state of the system isn't able to collect all the debug information and double fails. I don't know if that is a problem if the original failure is still presented to the engineer.

My default position is to fail fast and fail often. I'm with you on this. In this case however, we are attempting to grab potentially critical and relevant details about the cluster, and so I figure a best-effort where we capture all failures, might make sense. We should still error the job (I think) if the snapshot generation/capture fails partially, but I think it's safer if that still produces some output for people to consume.

test/kubernetes/e2e/test.go

sam-heilbron · 2024-11-26T15:25:29Z

test/kubernetes/e2e/tests/manifests/common-recommendations.yaml

@@ -74,6 +74,8 @@ gloo:
      limits:
        cpu: 1000m
        memory: 10Gi
+    stats:
+      enabled: true # enable stats server for gloo so we can collect the metrics in CI


I like the comment! I wonder if we should consider making stats on the gloo pod default to on in future releases? https://github.com/solo-io/solo-projects/issues/7292 is an example of something similar. If you think this could be valuable, could you write up an RFE to capture it?

Most controllers I've worked with have metrics on by default as it's pretty typical to be running Prometheus Operator, making scraping easy (PodMonitor/ServiceMonitor resources).

k8sgateway#10383

Totally agree with you. in fact, I was surprised to see this wasn't the default. Thanks for writing up the issue.

…b.com/solo-io/gloo into rolds/test_failure_collect_metrics_dump

ryanrolds added 2 commits November 22, 2024 13:15

Enable stats in common recommendations

45537f3

Fetch and save Gloo metrics and some snapshots on failure

ff88db6

ryanrolds commented Nov 23, 2024

View reviewed changes

github-actions bot added keep pr updated work in progress labels Nov 23, 2024

ryanrolds commented Nov 23, 2024

View reviewed changes

ryanrolds added 2 commits November 22, 2024 17:37

Merge branch 'main' into rolds/test_failure_collect_metrics_dump

24322c5

Added changelog

e2b43ec

ryanrolds marked this pull request as ready for review November 23, 2024 01:42

nfuden reviewed Nov 23, 2024

View reviewed changes

changelog-bot and others added 10 commits November 25, 2024 18:00

Adding changelog file to new location

31d5f5b

Deleting changelog file from old location

ba62682

Move to using glood admin cli package

2cbdc1e

Merge branch 'rolds/test_failure_collect_metrics_dump' of ssh://githu…

da1b4ae

…b.com/solo-io/gloo into rolds/test_failure_collect_metrics_dump

Adjusting error handling

788c36f

Adjusting error handling

1e353f2

Adjusting error handling

f40c5a0

Fix error check failing

a32eac9

Removed intentional failure

1b100f5

Merge branch 'main' into rolds/test_failure_collect_metrics_dump

5618f3e

ryanrolds requested a review from nfuden November 25, 2024 20:22

ryanrolds added 2 commits November 25, 2024 14:59

Merge branch 'main' into rolds/test_failure_collect_metrics_dump

b88fb8d

Merge branch 'main' into rolds/test_failure_collect_metrics_dump

1f6a3be

andy-fong approved these changes Nov 26, 2024

View reviewed changes

Merge branch 'main' into rolds/test_failure_collect_metrics_dump

510fb88

nfuden approved these changes Nov 26, 2024

View reviewed changes

sam-heilbron reviewed Nov 26, 2024

View reviewed changes

ryanrolds and others added 2 commits November 26, 2024 10:10

Merge branch 'main' into rolds/test_failure_collect_metrics_dump

49d65dd

Merge branch 'main' into rolds/test_failure_collect_metrics_dump

34ee46c

ryanrolds added 2 commits November 26, 2024 16:08

Partial work unifying the dump logic

11d4336

Merge branch 'rolds/test_failure_collect_metrics_dump' of ssh://githu…

9eed2ca

…b.com/solo-io/gloo into rolds/test_failure_collect_metrics_dump

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collect Gloo metrics and some snapshots on test failure #10400

Collect Gloo metrics and some snapshots on test failure #10400

ryanrolds commented Nov 23, 2024 •

edited

Loading

ryanrolds Nov 23, 2024

ryanrolds Nov 23, 2024

github-actions bot commented Nov 23, 2024 •

edited

Loading

nfuden Nov 23, 2024

andy-fong Nov 26, 2024

ryanrolds Nov 26, 2024

ryanrolds Nov 26, 2024

sam-heilbron left a comment

sam-heilbron Nov 26, 2024

sam-heilbron Nov 26, 2024

ryanrolds Nov 26, 2024

sam-heilbron Nov 26, 2024

sam-heilbron Nov 26, 2024

sam-heilbron Nov 26, 2024

sam-heilbron Nov 26, 2024

ryanrolds Nov 26, 2024

ryanrolds Nov 26, 2024

sam-heilbron Nov 26, 2024

sam-heilbron Nov 26, 2024

ryanrolds Nov 26, 2024

sam-heilbron Nov 26, 2024

Collect Gloo metrics and some snapshots on test failure #10400

Are you sure you want to change the base?

Collect Gloo metrics and some snapshots on test failure #10400

Conversation

ryanrolds commented Nov 23, 2024 • edited Loading

Description

Interesting decisions

Testing steps

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 23, 2024 • 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

sam-heilbron left a 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

ryanrolds commented Nov 23, 2024 •

edited

Loading

github-actions bot commented Nov 23, 2024 •

edited

Loading