Restore periodic prow job for upgrade tests

[lbernick]

Recently, we have had a few issues of regressions that were not detected by CI.

For example, #4834 introduced changes that caused a new client to be incompatible with an old server.

In addition, some breaking changes were introduced where resources already stored on a cluster became invalid when the server version was upgraded. #4779 added new defaults via the mutating webhook, and then #4818 validated the existence of these defaults. However, existing resources didn't receive these defaults, so they failed validation.

We already have some upgrade tests.
These tests have two parts:

1. Install a new version on a cluster with an older server version, and run integration tests (using the newer version client) on this newer version server
2. Create some resources on a cluster with an older server version, then upgrade the cluster to use a newer server version. Run integration tests on this cluster.

We should add a step where we run integration tests on the older server version (before upgrading the server). This will address the first type of regression detailed here. When we do make a breaking change purposefully (after deprecating a field), I would imagine we'd remove tests for this functionality, so I don't think adding this check would stop us from removing deprecated fields.

The second type of regression (existing resources becoming invalid) seems like it should have been caught by the upgrade tests. I'm not sure why these tests didn't catch this issue-- maybe upgrading the server reapplies the webhook defaults.

[yuzp1996]

Hello! I run e2e on my machine and try to figure out how the e2e test works. Now I think I am a little familiar with this e2e test. But when I run the e2e test I found some weird things and was not sure if it is a bug.

First, there is some unnormal log and I find the related issue #4777 but seems like the bug is not resolved. But I think it is a good chance to fix it and happy to do this.

Second I did not find the function apply_resources in the repository tektoncd/pipeline or tektoncd/plumbing, I guess maybe the function is deleted but forget to delete the reference here ?

pipeline/test/e2e-tests-upgrade.sh

Line 69 in c1d4702

apply_resources ${test}

[yuzp1996]

I noticed that in the e2e-tests-upgrade.sh the older version is v0.5.2.

pipeline/test/e2e-tests-upgrade.sh

Line 28 in c1d4702

PREVIOUS_PIPELINE_VERSION=v0.5.2

I think maybe v0.5.2 is too old that is not suitable to be the older version in the upgrade e2e test? I guess maybe no one will upgrade the server to the latest from the oldest version in their production environment?

The above are some of my humble opinions, not sure if it is reasonable. 😂

[yuzp1996]

I find that the e2e-tests-upgrade.sh script is referenced here as a cron job but I don't find the related running job in tektoncd/pipeline github actions.

https://github.com/tektoncd/plumbing/blob/c5b0f73a1b5e2ca0025d21ac9b7d69a3f7843506/prow/config.yaml#L1829

Could you help show me where I can get the relevant running job? @lbernick Thanks very much!

[afrittoli]

Upgrade testing is something we've been discussing with @abayer in the productivity WG.
It's definitely something that we need to implement, we should have solid tests before v1.0.

We also need to make sure we have enough visibility on periodic jobs. If an upgrade job is running it has not been maintained or seen by anyone for a long time afaik.

We where discussing perhaps we should include it in the release process.

[dibyom]

@yuzp1996 Nice detective work! I agree pipeline v0.5.2 is too old and we should definitely make sure the cron job is 1) running and 2) sending notifications somewhere when it fails

[lbernick]

Looking at our prow dashboard, upgrade tests have been failing with fatal: destination path '/home/prow/go/src/github.com/google/licenseclassifier' already exists and is not an empty directory. I'm guessing they've been broken for some time and we haven't noticed.

Since these tests appear to be broken anyway, might be a good opportunity to move them to Tekton :)

[yuzp1996]

Looking at our prow dashboard, upgrade tests have been failing with fatal: destination path '/home/prow/go/src/github.com/google/licenseclassifier' already exists and is not an empty directory. I'm guessing they've been broken for some time and we haven't noticed.

Since these tests appear to be broken anyway, might be a good opportunity to move them to Tekton :)

I am sorry I don't understand what moving them to Tekton means. Is it something like dogfooding? Another CI system based on Tekton?

[yuzp1996]

Hi, @lbernick @afrittoli looks like there will be a large change for the e2e upgrade test. I am interested in e2e upgrade test. Is there anything I can do to help now? 😁

[lbernick]

I am sorry I don't understand what moving them to Tekton means. Is it something like dogfooding? Another CI system based on Tekton?

What I meant here is that instead of having this test run on prow, we can run it on our dogfooding kubernetes cluster via a Tekton TaskRun or PipelineRun. One of our medium-term goals is to move Tekton test infrastructure away from using prow and use Tekton to test Tekton instead.

I am interested in e2e upgrade test. Is there anything I can do to help now? 😁

A great first step would be seeing if you can get the existing scripts working on a k8s cluster! It looks like the tests are currently broken with the error message I've pasted above. Once we have a working script we can get started on adding this as a recurring TaskRun/PipelineRun in our dogfooding cluster.

[yuzp1996]

I am sorry I don't understand what moving them to Tekton means. Is it something like dogfooding? Another CI system based on Tekton?

What I meant here is that instead of having this test run on prow, we can run it on our dogfooding kubernetes cluster via a Tekton TaskRun or PipelineRun. One of our medium-term goals is to move Tekton test infrastructure away from using prow and use Tekton to test Tekton instead.

I am interested in e2e upgrade test. Is there anything I can do to help now? 😁

A great first step would be seeing if you can get the existing scripts working on a k8s cluster! It looks like the tests are currently broken with the error message I've pasted above. Once we have a working script we can get started on adding this as a recurring TaskRun/PipelineRun in our dogfooding cluster.

OK! I commented the initialize $@ to avoid creating GCP cluster when I run the script locally and found the e2e upgrade test worked well.

pipeline/test/e2e-tests-upgrade.sh

Line 32 in 3c5d33b

initialize $@

Looks like something went wrong in function initialize() I will try to find out why.
https://github.com/tektoncd/plumbing/blob/75f7d7e959de11479a64bb5a05f4f66217a22ef3/scripts/e2e-tests.sh#L357

[yuzp1996]

I find that Prow job will run the command /usr/local/bin/entrypoint.sh first

Then I pull the image to find out if the files /home/prow/go/src/github.com/google/licenseclassifier exist. Looks like we do not need to clone https://github.com/google/licenseclassifier to GOPATH anymore!

Related Issue:
google/licenseclassifier#20
tektoncd/plumbing#349

Related file:
entrypoint.sh
https://github.com/tektoncd/plumbing/blob/5dbf210387217e509550ce1c522f11a854058d10/tekton/images/test-runner/entrypoint.sh#L21

Solution:
I want to add a judgement before cloning the repository. This preserves the greatest possible compatibility.

[yuzp1996]

I made a change in tektoncd/plumbing#1175 and maybe it can resolve the problem when running the e2e-upgrade-test.sh.