Verify if pipeline works after upgrading from previous release to current release 🧗♂️ #1162
Conversation
googlebot
added
the
cla: yes
tekton-robot
added
the
size/M
houshengbo
changed the title
houshengbo
changed the title
houshengbo
changed the title
tekton-robot
added
the
do-not-merge/work-in-progress
|
/test all |
hmmmm /test pull-tekton-pipeline-integration-tests |
There was a problem hiding this comment.
@houshengbo thx for starting work on this one 👼
I'll add a prow job for this 👼
tekton-robot
added
size/L
houshengbo
changed the title
tekton-robot
removed
the
do-not-merge/work-in-progress
|
/retest |
There was a problem hiding this comment.
Thank you for this, it's an important test to have!
My ideal solution would be to have this test running as a Tekton pipeline, but since we do not have the facility to do that yet, starting with a bash script is ok.
If I got it right, the current sequence is:
- install previous release
- upgrade to master
- run E2E and YAML tests
- uninstall
- install previous release
- run YAML tests (but not result check)
- install master
- run E2E and YAML tests
My opinion on this is that we should have upgrade targeted tests, as this whole sequence may take too long and not necessarily test all that we need to test - i.e. I'm not convinced this would have caught the issues we encountered until now with upgrade; however I'd like to see what other maintainers think too.
I'm not sure I fully understand the need to go through the upgrade twice.
It is fine to start with E2E or YAML tests until we develop more targeted tests, but I would like to have dedicated functions that wrap things to be done before and after the the upgrade, so that it's easy to add / remove things from there and migrate to a Tasks once we're ready for that.
Could you please change the commit message to match the recommended format?
|
|
||
| for test in taskrun pipelinerun; do | ||
| header "Running YAML e2e tests for ${test}s" | ||
| if ! run_yaml_tests ${test}; then |
There was a problem hiding this comment.
I wonder if we really need to run all YAML tests here?
There was a problem hiding this comment.
yes, there are two scenarios we covered here. This is the first one.
| done | ||
|
|
||
| # Remove all the pipeline CRDs | ||
| uninstall_pipeline_crd |
There was a problem hiding this comment.
Why do we need to install and update two times?
There was a problem hiding this comment.
@afrittoli It is two cases actually as identified in: #1114
They are slightly different, not exactly the same.
There was a problem hiding this comment.
When we finish the first one, remove and clean up everything. Run the second scenario.
There was a problem hiding this comment.
makes sense @houshengbo - i had the same question tho! maybe there could be some comments that explain these scenarios in a bit more detail so it would be clear to someone who just sees this script?
There was a problem hiding this comment.
I will add more comments on these two scenarios.
|
|
||
| for test in taskrun pipelinerun; do | ||
| header "Applying the resources ${test}s" | ||
| apply_resources ${test} |
There was a problem hiding this comment.
This runs all YAML tests again, but it does not check the result, right?
I think it would be nice to create a targeted set of resource of different type (e.g. PipelineResources, a Task, a Pipeline and perhaps long running TaskRuns and PipelineRuns) and to verify them after the upgrade.
There was a problem hiding this comment.
NO, this is a different case as I stated before.
We do apply_resources only here.
Then after install the crd, doing the upgrading, run_tests ${test}.
There was a problem hiding this comment.
You may have noticed that apply_resources and run_tests used to be in one function, but I split them specially for the two scenarios for upgrade.
There was a problem hiding this comment.
My opinion on this is that we should have upgrade targeted tests
Could do! This might be an okay starting point in the meantime? It'll also be very hard for folks writing features to know whether they should add upgrade tests (or not! if we can provide some clear guidelines. its not obvious to me at the moment what the guidelines would be tho)
as this whole sequence may take too long and not necessarily test all that we need to test
In tektoncd/plumbing#56 the job is being added as presubmit - why don't we try a nightly periodic run instead? (e.g. like https://github.com/knative/test-infra/blob/master/ci/prow/config.yaml#L2821)
The downside is we'd need to keep an eye on the prow dashboard (https://prow.tekton.dev) - but we could at the very least check it before making a release?
test/e2e-tests-upgrade.sh
Outdated
| # and deploy Tekton Pipelines to it for running integration tests. | ||
|
|
||
| source $(dirname $0)/e2e-common.sh | ||
| PREVIOUS_PIPELINE_VERSION=0.5.2 |
There was a problem hiding this comment.
this is pretty minor, but since the v is actually part of the tag, maybe it would make sense to include here vs. appending it whereever the version variable gets used?
| done | ||
|
|
||
| # Remove all the pipeline CRDs | ||
| uninstall_pipeline_crd |
There was a problem hiding this comment.
makes sense @houshengbo - i had the same question tho! maybe there could be some comments that explain these scenarios in a bit more detail so it would be clear to someone who just sees this script?
|
@bobcatfish Regarding what type of prow job we should create for the upgrade tests, there have already been pros and cons to have a PR-based job or a periodic job. Once we have a PR-based job, it is true that it runs on a PR basis, which consumes more infrastructure resources, but it can better guard the quality, since we know immediately whether the upgrade fails or not. Once we have a periodic job, we need at least someone to watch the job at least on a daily, weekly, or whatever frequency to see if there is a problem, and furthermore we have to find out and locate which commit brings in the issue. Even if we find it, we need to commit new PR, and manually launch this upgrade tests to verify. Well if there is no problem, that's fine. However, if we have an issue, it will be very time-consuming to fix it. I still tend to have a new prow job dedicated to do the upgrade tests. It is true it consume infrastructure resources, but the benefit to be automatic will pay back. |
|
@afrittoli There are two scenarios at least now for us to cover as I tried to tackle in the issue #1114. That is why you see install, test, uninstall... install again... I will write more comments in the the script for better understanding. |
|
/retest all |
|
/test pull-tekton-pipeline-build-tests |
|
/test pull-tekton-pipeline-integration-tests |
tekton-robot
added
size/M
tekton-robot
added
size/L
I'm less concerned about the infrastructure resources and more about turnaround time for contributors - the end to end tests are already the slowest check on each PR, and this would make them even slower :S What do you think if we start out with making this a periodic job and then see how often it fails? If it's often enough that we want to catch it on PRs we can add it as a presubmit job. I'm assuming this would be infrequent enough that it wouldn't be too bad. Another idea: Anyone else have other ideas @afrittoli @vdemeester @abayer @imjasonh @sbwsg @dibyom ? |
I do agree, we could have that as a periodic build for now so, not on all PRs. |
kk - I'll move that discussion over to tektoncd/plumbing#56 In the meantime I think we can merge this. @houshengbo I think that "scenario 1" and "scenario 2" in the comments in the script could use some more detail but I also think we can iterate and merge this as is now: /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bobcatfish, houshengbo 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:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
|
/test pull-tekton-pipeline-build-tests |
Changes
This PR adds the upgrade test case to install the previous release, upgrade to the current release, and run the integration tests.
Partially-closes: #1114
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task and TaskRun to build and release this image
Release Notes