Remove v1alpha1 Runs Client setup in init test #6571

JeromeJu · 2023-04-21T12:46:43Z

Changes

This commit removes the alpha1 Runs client setup in init_test
because the v1alpha1 Run support has been removed according to
TEP-114. The error occurs during e2e tests that is introduced as
a flake:
ie. https://storage.googleapis.com/tekton-prow/pr-logs/pull/tektoncd_pipeline/6567/pull-tekton-pipeline-alpha-integration-tests/1649045464097492992/build-log.txt

/kind misc

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

[n/a] Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
[n/a] Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
[n/a] Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
[n/a] Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

JeromeJu · 2023-04-21T13:23:15Z

/test pull-tekton-pipeline-integration-tests

wlynch · 2023-04-21T15:28:51Z

test/init_test.go

@@ -291,15 +291,6 @@ func getCRDYaml(ctx context.Context, cs *clients, ns string) ([]byte, error) {
 		printOrAdd(i)
 	}

-	v1alpha1Runs, err := cs.V1alpha1RunClient.List(ctx, metav1.ListOptions{})


I may be missing some context here.

If this is panicking, shouldn't we try to fix the panic instead of removing this part of the test altogether? (my assumption is because the client isn't present?)

Thanks Billy. I was mistaken thought that with the removal of clients setup at

pipeline/test/clients.go

Lines 101 to 112 in a7424c4

c.V1beta1PipelineClient = cs.TektonV1beta1().Pipelines(namespace)

c.V1beta1ClusterTaskClient = cs.TektonV1beta1().ClusterTasks()

c.V1beta1TaskClient = cs.TektonV1beta1().Tasks(namespace)

c.V1beta1TaskRunClient = cs.TektonV1beta1().TaskRuns(namespace)

c.V1beta1PipelineRunClient = cs.TektonV1beta1().PipelineRuns(namespace)

c.V1beta1CustomRunClient = cs.TektonV1beta1().CustomRuns(namespace)

c.V1alpha1ResolutionRequestclient = rrcs.ResolutionV1alpha1().ResolutionRequests(namespace)

c.V1alpha1VerificationPolicyClient = cs.TektonV1alpha1().VerificationPolicies(namespace)

c.V1PipelineClient = cs.TektonV1().Pipelines(namespace)

c.V1TaskClient = cs.TektonV1().Tasks(namespace)

c.V1TaskRunClient = cs.TektonV1().TaskRuns(namespace)

c.V1PipelineRunClient = cs.TektonV1().PipelineRuns(namespace)

for TEP114, this would not be necessary moving forward.

when running the following e2e test on local branch the following error occured.
Currently debugging through the reason why it failed but I think the removed line 294 in init_test could be the reason it failed

± |v1-storage → origin ↑1 ↓2 {51} U:2 ✗| → go test -v -tags=e2e -count=1 ./test -run ^TestLargerResultsSidecarLogs === RUN TestLargerResultsSidecarLogs === PAUSE TestLargerResultsSidecarLogs === CONT TestLargerResultsSidecarLogs === RUN TestLargerResultsSidecarLogs/larger_results_via_sidecar_logs === PAUSE TestLargerResultsSidecarLogs/larger_results_via_sidecar_logs === CONT TestLargerResultsSidecarLogs/larger_results_via_sidecar_logs larger_results_sidecar_logs_test.go:69: Create namespace arendelle-qtjd7 to deploy to larger_results_sidecar_logs_test.go:69: Verify SA "default" is created in namespace "arendelle-qtjd7" larger_results_sidecar_logs_test.go:74: Setting up test resources for "larger results via sidecar logs" test in namespace arendelle-qtjd7 larger_results_sidecar_logs_test.go:82: Waiting for PipelineRun larger-results-sidecar-logs in namespace arendelle-qtjd7 to complete larger_results_sidecar_logs_test.go:84: Error waiting for PipelineRun larger-results-sidecar-logs to finish: "larger-results-sidecar-logs" failed panic.go:522: ############################################ panic.go:522: ### Dumping objects from arendelle-qtjd7 ### panic.go:522: ############################################ --- FAIL: TestLargerResultsSidecarLogs (14.11s) --- FAIL: TestLargerResultsSidecarLogs/larger_results_via_sidecar_logs (14.11s) panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x2 addr=0x38 pc=0x103dfed20] goroutine 12 [running]: testing.tRunner.func1.2({0x1044ec4a0, 0x105cf9360}) /usr/local/go/src/testing/testing.go:1526 +0x1c8 testing.tRunner.func1() /usr/local/go/src/testing/testing.go:1529 +0x384 panic({0x1044ec4a0, 0x105cf9360}) /usr/local/go/src/runtime/panic.go:884 +0x204 github.com/tektoncd/pipeline/test.getCRDYaml({0x104924b98, 0x1400036fd10}, 0x14000328b60, {0x14000304d30, 0xf}) /Users/jeromeju/go/src/github.com/tektoncd/pipeline/test/init_test.go:294 +0xab0 github.com/tektoncd/pipeline/test.tearDown({0x104924b98, 0x1400036fd10}, 0x14000573520, 0x14000328b60, {0x14000304d30, 0xf}) /Users/jeromeju/go/src/github.com/tektoncd/pipeline/test/init_test.go:98 +0xc0 runtime.Goexit() /usr/local/go/src/runtime/panic.go:522 +0x180 testing.(*common).FailNow(0x14000573520) /usr/local/go/src/testing/testing.go:980 +0x44 testing.(*common).Fatalf(0x14000573520, {0x103e9a76d?, 0x14000328b60?}, {0x14000d1fef0?, 0x1b?, 0x0?}) /usr/local/go/src/testing/testing.go:1064 +0x60 github.com/tektoncd/pipeline/test.TestLargerResultsSidecarLogs.func1(0x14000573520) /Users/jeromeju/go/src/github.com/tektoncd/pipeline/test/larger_results_sidecar_logs_test.go:84 +0x4b8 testing.tRunner(0x14000573520, 0x14000945290) /usr/local/go/src/testing/testing.go:1576 +0x10c created by testing.(*T).Run /usr/local/go/src/testing/testing.go:1629 +0x368 FAIL github.com/tektoncd/pipeline/test 15.322s FAIL

I'm noticing this on other PRs too, for example https://storage.googleapis.com/tekton-prow/pr-logs/pull/tektoncd_pipeline/6516/pull-tekton-pipeline-alpha-integration-tests/1649485487607386112/build-log.txt

It seems like this was introduced by #6508, but I'm not sure why it is nondeterministic

lbernick · 2023-04-24T12:51:04Z

Labeling critical/urgent since I think this issue is breaking other PRs as well. @JeromeJu can you please remove the v1alpha1 client here too? Could you also add a bit more detail to the commit message? I want to better understand why the PR that introduced this panic passed CI.

JeromeJu · 2023-04-24T13:25:55Z

Thanks Lee!

@JeromeJu can you please remove the v1alpha1 client here too?

Might be wrong but I didn't remove this because I thought we might want to leave the clients struct that is available to fetch alpha1 Runs during the release version upgrading?

Could you also add a bit more detail to the commit message? I want to better understand why the PR that introduced this panic passed CI.

Updated the commit message. Currently I haven't been able to reproduce this on remote/main but in remote/v1-storage branch it persists when I run e2e against test ie. TestLargerResultsSidecarLogs.

Currently working on it and I will file an issue once I am able to reproduce on main.

lbernick · 2023-04-24T13:34:39Z

Thanks Lee!

@JeromeJu can you please remove the v1alpha1 client here too?

Might be wrong but I didn't remove this because I thought we might want to leave the clients struct that is available to fetch alpha1 Runs during the release version upgrading?

v1alpha1 Run client is still available in pkg/client. The file I linked is used only in tekton pipelines tests, which don't test v1alpha1 Runs anymore.

Could you also add a bit more detail to the commit message? I want to better understand why the PR that introduced this panic passed CI.

Updated the commit message. Currently I haven't been able to reproduce this on remote/main but in remote/v1-storage branch it persists when I run e2e against test ie. TestLargerResultsSidecarLogs.

Currently working on it and I will file an issue once I am able to reproduce on main.

Here's an example of the same failure on a documentation PR: https://storage.googleapis.com/tekton-prow/pr-logs/pull/tektoncd_pipeline/6567/pull-tekton-pipeline-alpha-integration-tests/1649045464097492992/build-log.txt
So it's definitely a flaky test introduced on main.

This commit removes the alpha1 Runs client setup in init_test because the v1alpha1 Run support has been removed according to TEP-114. The error occurs during e2e tests that is introduced as a flake: ie. https://storage.googleapis.com/tekton-prow/pr-logs/pull/tektoncd_pipeline/6567/pull-tekton-pipeline-alpha-integration-tests/1649045464097492992/build-log.txt

JeromeJu · 2023-04-24T14:00:54Z

Thank you. Updated (thought that an issue is required to help understand the flake but it seems sufficient with the log.

Thanks Lee!

@JeromeJu can you please remove the v1alpha1 client here too?

Might be wrong but I didn't remove this because I thought we might want to leave the clients struct that is available to fetch alpha1 Runs during the release version upgrading?

v1alpha1 Run client is still available in pkg/client. The file I linked is used only in tekton pipelines tests, which don't test v1alpha1 Runs anymore.

Could you also add a bit more detail to the commit message? I want to better understand why the PR that introduced this panic passed CI.

Updated the commit message. Currently I haven't been able to reproduce this on remote/main but in remote/v1-storage branch it persists when I run e2e against test ie. TestLargerResultsSidecarLogs.
Currently working on it and I will file an issue once I am able to reproduce on main.

Here's an example of the same failure on a documentation PR: https://storage.googleapis.com/tekton-prow/pr-logs/pull/tektoncd_pipeline/6567/pull-tekton-pipeline-alpha-integration-tests/1649045464097492992/build-log.txt So it's definitely a flaky test introduced on main.

lbernick

Thanks Jerome! I still need to think more about why this behavior is nondeterministic, not sure yet.

tekton-robot · 2023-04-24T14:08:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [lbernick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lbernick · 2023-04-24T14:11:10Z

Alpha test failure for this PR: https://storage.googleapis.com/tekton-prow/pr-logs/pull/tektoncd_pipeline/6571/pull-tekton-pipeline-alpha-integration-tests/1650488907080404992/build-log.txt
However the failure is

    examples_test.go:62: Failed waiting for V1 pipeline run done: "pipelinerun-with-params" failed
    panic.go:540: ############################################
    panic.go:540: ### Dumping objects from arendelle-vx26n ###
    panic.go:540: ############################################
    panic.go:540:

this looks like an actual transient issue unrelated to the problem this PR fixes

jerop

/lgtm

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/misc Categorizes issue or PR as a miscellaneuous one. labels Apr 21, 2023

tekton-robot requested review from vdemeester and wlynch April 21, 2023 12:46

tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 21, 2023

wlynch reviewed Apr 21, 2023

View reviewed changes

JeromeJu closed this Apr 21, 2023

JeromeJu reopened this Apr 21, 2023

lbernick added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Apr 24, 2023

JeromeJu force-pushed the alpharun-init branch from 861fa32 to fbc99a4 Compare April 24, 2023 13:16

JeromeJu force-pushed the alpharun-init branch from fbc99a4 to b19a8f2 Compare April 24, 2023 13:58

tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 24, 2023

lbernick approved these changes Apr 24, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2023

jerop changed the title ~~Remove alpha1 Runs Client setup in init test~~ Remove v1alpha1 Runs Client setup in init test Apr 24, 2023

jerop assigned lbernick and jerop Apr 24, 2023

jerop reviewed Apr 24, 2023

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2023

tekton-robot merged commit 8e8c163 into tektoncd:main Apr 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove v1alpha1 Runs Client setup in init test #6571

Remove v1alpha1 Runs Client setup in init test #6571

JeromeJu commented Apr 21, 2023 •

edited

Loading

JeromeJu commented Apr 21, 2023

wlynch Apr 21, 2023

JeromeJu Apr 21, 2023

JeromeJu Apr 21, 2023

lbernick Apr 21, 2023

lbernick commented Apr 24, 2023

JeromeJu commented Apr 24, 2023 •

edited

Loading

lbernick commented Apr 24, 2023

JeromeJu commented Apr 24, 2023

lbernick left a comment

tekton-robot commented Apr 24, 2023

lbernick commented Apr 24, 2023

jerop left a comment

	c.V1beta1PipelineClient = cs.TektonV1beta1().Pipelines(namespace)
	c.V1beta1ClusterTaskClient = cs.TektonV1beta1().ClusterTasks()
	c.V1beta1TaskClient = cs.TektonV1beta1().Tasks(namespace)
	c.V1beta1TaskRunClient = cs.TektonV1beta1().TaskRuns(namespace)
	c.V1beta1PipelineRunClient = cs.TektonV1beta1().PipelineRuns(namespace)
	c.V1beta1CustomRunClient = cs.TektonV1beta1().CustomRuns(namespace)
	c.V1alpha1ResolutionRequestclient = rrcs.ResolutionV1alpha1().ResolutionRequests(namespace)
	c.V1alpha1VerificationPolicyClient = cs.TektonV1alpha1().VerificationPolicies(namespace)
	c.V1PipelineClient = cs.TektonV1().Pipelines(namespace)
	c.V1TaskClient = cs.TektonV1().Tasks(namespace)
	c.V1TaskRunClient = cs.TektonV1().TaskRuns(namespace)
	c.V1PipelineRunClient = cs.TektonV1().PipelineRuns(namespace)

Remove v1alpha1 Runs Client setup in init test #6571

Remove v1alpha1 Runs Client setup in init test #6571

Conversation

JeromeJu commented Apr 21, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

JeromeJu commented Apr 21, 2023

wlynch Apr 21, 2023

Choose a reason for hiding this comment

JeromeJu Apr 21, 2023

Choose a reason for hiding this comment

JeromeJu Apr 21, 2023

Choose a reason for hiding this comment

lbernick Apr 21, 2023

Choose a reason for hiding this comment

lbernick commented Apr 24, 2023

JeromeJu commented Apr 24, 2023 • edited Loading

lbernick commented Apr 24, 2023

JeromeJu commented Apr 24, 2023

lbernick left a comment

Choose a reason for hiding this comment

tekton-robot commented Apr 24, 2023

lbernick commented Apr 24, 2023

jerop left a comment

Choose a reason for hiding this comment

JeromeJu commented Apr 21, 2023 •

edited

Loading

JeromeJu commented Apr 24, 2023 •

edited

Loading