Testing: Save pod logs and resources after e2e test run #234

jmprusi · 2023-05-29T14:57:20Z

Description

Generate a summary of logs/resources after e2e runs.

Disabled by default, to enable, pass the ARTIFACT_PATH env var when running the e2e tests: ARTIFACT_PATH=/tmp/e2e/ make test
Enabled on GitHub CI, you can access the artifacts via the "e2e / e2e-kind (pull_request)" check/action.

Current output of a e2e test run:

.
├── argocd-operator-system
│   └── argocd-operator-controller-manager-deployment.yaml
├── catalogd-system
│   ├── catalogd-controller-manager-77b88f9f46-85hg2-kube-rbac-proxy-logs.txt
│   ├── catalogd-controller-manager-77b88f9f46-85hg2-manager-logs.txt
│   └── catalogd-controller-manager-deployment.yaml
├── cert-manager
│   ├── cert-manager-5f76f6fd49-4926c-cert-manager-logs.txt
│   ├── cert-manager-cainjector-68698c5958-nvxzg-cert-manager-logs.txt
│   ├── cert-manager-cainjector-deployment.yaml
│   ├── cert-manager-deployment.yaml
│   ├── cert-manager-webhook-5c9c98975c-pplrr-cert-manager-logs.txt
│   └── cert-manager-webhook-deployment.yaml
├── crdvalidator-system
│   ├── crd-validation-webhook-67795fdc9d-nvz57-crd-validation-webhook-logs.txt
│   └── crd-validation-webhook-deployment.yaml
├── default
├── local-path-storage
│   ├── local-path-provisioner-684f458cdd-5vsfh-local-path-provisioner-logs.txt
│   └── local-path-provisioner-deployment.yaml
├── operator-controller-system
│   ├── operator-controller-controller-manager-5fdb4997d8-l8sqp-kube-rbac-proxy-logs.txt
│   ├── operator-controller-controller-manager-5fdb4997d8-l8sqp-manager-logs.txt
│   └── operator-controller-controller-manager-deployment.yaml
├── operator-qztk2xvw-bundleDeployment.yaml
├── operator-qztk2xvw-bvww52-bundle.yaml
├── operator-qztk2xvw-operator.yaml
├── rukpak-system
│   ├── core-66d8b5494d-vr494-kube-rbac-proxy-logs.txt
│   ├── core-66d8b5494d-vr494-manager-logs.txt
│   ├── core-deployment.yaml
│   ├── helm-provisioner-68b8df478-p8gp7-kube-rbac-proxy-logs.txt
│   ├── helm-provisioner-68b8df478-p8gp7-manager-logs.txt
│   ├── helm-provisioner-deployment.yaml
│   ├── rukpak-webhooks-5cd9dddf98-f6gx8-webhooks-logs.txt
│   └── rukpak-webhooks-deployment.yaml
└── test-catalog-catalogsource.yaml

9 directories, 29 files

TODO:

Enable/Disable
Custom e2e artifacts path
Download artifact on GitHub actions.

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

.github/workflows/e2e.yaml

test/e2e/install_test.go

ncdc · 2023-05-30T14:23:04Z

test/e2e/install_test.go

+
+	// Get all namespaces
+	namespaces, err := kubeClient.CoreV1().Namespaces().List(ctx, metav1.ListOptions{})
+	Expect(err).To(Not(HaveOccurred()))


Global: I can't remember - does Expect abort the test immediately if the expectation fails? If so, I'd recommend distinguishing between a hard-failure (e.g. couldn't create kubeClient, couldn't create artifactPath) and soft failures (couldn't list something, couldn't marshal something, etc.). For the soft failures, I'd suggest logging the issue and continuing, so you try to capture as much as you can.

Expect does abort immediately

I don't see a useful logger... fmt.Printf is good enough? what do you propose?

https://onsi.github.io/ginkgo/#logging-output has details on what's available

fixed, PTAL.

test/e2e/install_test.go

tmshort · 2023-05-30T18:09:56Z

test/e2e/install_test.go

+				buf := make([]byte, 1024)
+				for {
+					_, err := logs.Read(buf)
+					if err != nil {
+						break
+					}
+				}
+
+				// Save logs to artifact path
+				err = os.WriteFile(filepath.Join(namespaceArtifactPath, pod.Name+"-"+container.Name+"-logs.txt"), buf, 0644)


My gut is telling me this will only read 1024 bytes of the log at a time, and only write out the last 1024 bytes? (Although golang surprises me sometimes.)

So my understanding is that this will read from the logs 1024 bytes at a time until it encounters an error (ideally and EOF error to signal the end of the file, but the error check doesn't seem to differentiate - and maybe there isn't a need to) and then will break and write the output. I would personally prefer to substitute this for loop with a buf, err := io.ReadAll(logs) as it should handle all the reading for us.

Logs can be very long and artifact gathers that get OOMKilled write nothing :)

That's why I previously suggested a small buffer and io.Copy...

I could be reading it wrong, but it looks like io.ReadAll() internally reads a small buffer at a time: https://cs.opensource.google/go/go/+/refs/tags/go1.20.4:src/io/io.go;drc=dc8e2a6a8ec94f2c98ba20edd57932eba284efb1;l=690

Definitely agree with the suggestion of io.Copy for writing as the stream is read

It starts small, but because this function returns the entire contents, io.ReadAll continually increases the size of the buffer as it reads more data.

using io.Copy now, let me know if that's better

everettraven · 2023-05-30T20:48:22Z

.github/workflows/e2e.yaml

+        ARTIFACT_PATH=/tmp/artifacts make e2e
+
+    - uses: cytopia/upload-artifact-retry-action@v0.1.7
+      if: ${{ always() }}
+      with:
+        name: e2e-artifacts
+        path: /tmp/artifacts/


Might be nice to make the artifact path a variable we define once and use in multiple places. Some docs on github action (environment) variables: https://docs.github.com/en/actions/learn-github-actions/variables#about-variables

test/e2e/install_test.go

everettraven · 2023-05-30T21:06:49Z

test/e2e/install_test.go

+
+	// Get all namespaces
+	namespaces, err := kubeClient.CoreV1().Namespaces().List(ctx, metav1.ListOptions{})
+	Expect(err).To(Not(HaveOccurred()))


Expect does abort immediately

everettraven · 2023-05-30T21:13:02Z

test/e2e/install_test.go

+				buf := make([]byte, 1024)
+				for {
+					_, err := logs.Read(buf)
+					if err != nil {
+						break
+					}
+				}
+
+				// Save logs to artifact path
+				err = os.WriteFile(filepath.Join(namespaceArtifactPath, pod.Name+"-"+container.Name+"-logs.txt"), buf, 0644)


So my understanding is that this will read from the logs 1024 bytes at a time until it encounters an error (ideally and EOF error to signal the end of the file, but the error check doesn't seem to differentiate - and maybe there isn't a need to) and then will break and write the output. I would personally prefer to substitute this for loop with a buf, err := io.ReadAll(logs) as it should handle all the reading for us.

test/e2e/install_test.go

codecov · 2023-07-19T08:45:09Z

Codecov Report

Patch coverage has no change and project coverage change: -4.42% ⚠️

Comparison is base (1ec4a23) 81.64% compared to head (b9824db) 77.23%.

❗ Current head b9824db differs from pull request most recent head 29b8758. Consider uploading reports for the commit 29b8758 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #234      +/-   ##
==========================================
- Coverage   81.64%   77.23%   -4.42%     
==========================================
  Files          21       18       -3     
  Lines         937      795     -142     
==========================================
- Hits          765      614     -151     
- Misses        119      146      +27     
+ Partials       53       35      -18

Flag	Coverage Δ
e2e	`?`
unit	`77.23% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joelanford · 2023-08-16T19:09:13Z

What's the status of this PR? Functionality like this would help shed light on this apparent e2e code coverage change. #336 (comment)

ncdc · 2023-09-07T14:46:14Z

test/e2e/install_test.go

+const (
+	defaultTimeout = 30 * time.Second
+	defaultPoll    = 1 * time.Second
+	testCatalogRef = "localhost/testdata/catalogs/test-catalog:e2e"


This needs to be removed - e2e_suite_test.go currently has consts for this, and a helper function getCatalogImageRef which supports optionally changing the catalog image via an env var.

ncdc · 2023-09-07T14:46:34Z

test/e2e/install_test.go

+// - bundle
+// - bundledeployments
+// - catalogsources
+


Suggested change

ncdc · 2023-09-07T14:46:59Z

test/e2e/install_test.go

@@ -222,3 +243,163 @@ var _ = Describe("Operator Install", func() {

 	})
 })
+
+// getArtifactsOutput gets all the artifacts from the test run and saves them to the artifact path.
+// right now it will save:


Suggested change

// right now it will save:

// Currently it saves:

ncdc · 2023-09-07T14:50:41Z

@jmprusi if you can rebase and make my requested changes, we can merge

Signed-off-by: Joaquim Moreno Prusi <joaquim@redhat.com>

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 29, 2023

jmprusi force-pushed the jmprusi/test_logs branch 2 times, most recently from 790d618 to 98e238d Compare May 30, 2023 13:39

jmprusi marked this pull request as ready for review May 30, 2023 13:56

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 30, 2023

jmprusi changed the title ~~Expose logs/resources after test run~~ Testing: Save pod logs and resources after e2e test run May 30, 2023