Poll multiple times that the pvc is deleted in TestPipelineRun #916

chmouel · 2019-05-28T13:42:01Z

Let's poll for up to 10mn that the PersistentVolumeClaims has the
DeletionTimestamp set.

Fixes #915

Signed-off-by: Chmouel Boudjnah [email protected]

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

[ ✔️ ] Includes tests (if functionality changed/added)
[ 🤷🏼‍♂️ ] Includes docs (if user facing)
[ ✔️ ] Commit messages follow commit message best practices

vdemeester

Sounds good to me
cc @abayer

abayer · 2019-05-28T13:45:33Z

/lgtm

Thanks!

chmouel · 2019-05-28T13:48:09Z

/hold

abayer · 2019-05-28T14:13:13Z

/lgtm

chmouel · 2019-05-28T14:28:57Z

/hold cancel

Should work know, not 100% sure but the future will hold truth

abayer · 2019-05-28T14:33:02Z

...I really wrote a lousy test, didn't I. =)

chmouel · 2019-05-28T14:50:42Z

@abayer arf not it's fine, there is way worse things :)

vdemeester · 2019-05-28T15:47:11Z

test/pipelinerun_test.go

+					return true, fmt.Errorf("Error looking up PVC %s for PipelineRun %s: %s", artifacts.GetPVCName(pipelineRun), prName, errWait)
+				}
+				return pvc.DeletionTimestamp == nil, errWait
+			}); err != nil {
 				if !errors.IsNotFound(err) {


🤔 shouldn't that check be in the PollImmediate ? (and returning true + nil as error) so that we don't have to do the check twice ?

vdemeester · 2019-05-28T15:47:51Z

test/pipelinerun_test.go

+				if errWait != nil && !errors.IsNotFound(errWait) {
+					return true, fmt.Errorf("Error looking up PVC %s for PipelineRun %s: %s", artifacts.GetPVCName(pipelineRun), prName, errWait)
+				}
+				return pvc.DeletionTimestamp == nil, errWait


I don't think we need to return errWait here, as it's only be IsNotFound as an error, and in this case, we don't want it to fail (as it's already gone)

chmouel · 2019-05-29T08:39:10Z

Seems like it works when testing the test locally

% go test -failfast -v -count=1 -tags=e2e -ldflags '-X github.com/tektoncd/pipeline/test.missingKoFatal=false' ./test -timeout=20m --kubeconfig $KUBECONFIG -run TestPipelineRun/service_account_propagation 2>&1 |tee /tmp/log
Using kubeconfig at `/Users/chmouel/.kube/config` with cluster ``
=== RUN   TestPipelineRun
=== PAUSE TestPipelineRun
=== RUN   TestPipelineRunTimeout
2019-05-29T10:35:01.537+0200	info	logging/config.go:39	Successfully created the logger.	{"knative.dev/jsonconfig": "{\n\t  \"level\": \"info\",\n\t  \"encoding\": \"console\",\n\t  \"outputPaths\": [\"stdout\"],\n\t  \"errorOutputPaths\": [\"stderr\"],\n\t  \"encoderConfig\": {\n\t    \"timeKey\": \"ts\",\n\t    \"messageKey\": \"message\",\n\t    \"levelKey\": \"level\",\n\t    \"nameKey\": \"logger\",\n\t    \"callerKey\": \"caller\",\n\t    \"messageKey\": \"msg\",\n\t    \"stacktraceKey\": \"stacktrace\",\n\t    \"lineEnding\": \"\",\n\t    \"levelEncoder\": \"\",\n\t    \"timeEncoder\": \"iso8601\",\n\t    \"durationEncoder\": \"\",\n\t    \"callerEncoder\": \"\"\n\t  }\n\t}"}
2019-05-29T10:35:01.537+0200	info	logging/config.go:39	Logging level set to info
2019-05-29T10:35:01.537+0200	warn	logging/config.go:41	Fetch GitHub commit ID from kodata failed: "KO_DATA_PATH" does not exist or is empty
=== PAUSE TestPipelineRunTimeout
=== RUN   TestPipelineRunFailedAndRetry
=== PAUSE TestPipelineRunFailedAndRetry
=== CONT  TestPipelineRun
=== CONT  TestPipelineRunTimeout
=== CONT  TestPipelineRunFailedAndRetry
=== RUN   TestPipelineRun/service_account_propagation
=== PAUSE TestPipelineRun/service_account_propagation
=== CONT  TestPipelineRun/service_account_propagation
--- PASS: TestPipelineRun (0.00s)
    --- PASS: TestPipelineRun/service_account_propagation (35.85s)
        init_test.go:100: Create namespace arendelle-lqhsw to deploy to
        init_test.go:112: Verify SA "default" is created in namespace "arendelle-lqhsw"
        pipelinerun_test.go:127: Setting up test resources for "service account propagation" test in namespace arendelle-lqhsw
        pipelinerun_test.go:136: Waiting for PipelineRun pipelinerun1 in namespace arendelle-lqhsw to complete
        pipelinerun_test.go:141: Making sure the expected TaskRuns [task1] were created
        pipelinerun_test.go:164: Checking that labels were propagated correctly for TaskRun pipelinerun1-task1-wq69c
        pipelinerun_test.go:170: Making sure 2 events were created from taskrun and pipelinerun with kinds map[PipelineRun:[pipelinerun1] TaskRun:[pipelinerun1-task1-wq69c]]
        pipelinerun_test.go:194: Successfully finished test "service account propagation"
        pipelinerun_test.go:195: Deleting namespace arendelle-lqhsw
--- PASS: TestPipelineRunTimeout (39.58s)
    init_test.go:100: Create namespace arendelle-zttbd to deploy to
    init_test.go:112: Verify SA "default" is created in namespace "arendelle-zttbd"
    timeout_test.go:43: Creating Task in namespace arendelle-zttbd
    timeout_test.go:63: Waiting for Pipelinerun pear in namespace arendelle-zttbd to be started
    timeout_test.go:83: Waiting for TaskRuns from PipelineRun pear in namespace arendelle-zttbd to be running
    timeout_test.go:114: Waiting for PipelineRun pear in namespace arendelle-zttbd to be timed out
    timeout_test.go:132: Waiting for TaskRuns from PipelineRun pear in namespace arendelle-zttbd to be cancelled
    timeout_test.go:175: Waiting for PipelineRun kiwi in namespace arendelle-zttbd to complete
    timeout_test.go:179: Deleting namespace arendelle-zttbd
--- PASS: TestPipelineRunFailedAndRetry (47.15s)
    init_test.go:100: Create namespace arendelle-7zhwf to deploy to
    init_test.go:112: Verify SA "default" is created in namespace "arendelle-7zhwf"
    timeout_test.go:189: Creating Task in namespace arendelle-7zhwf
    timeout_test.go:208: Waiting for Pipelinerun pear in namespace arendelle-7zhwf to be started
    timeout_test.go:231: Deleting namespace arendelle-7zhwf
PASS
ok  	github.com/tektoncd/pipeline/test	47.777s

I am going to increase the timeout and see if that changes things

chmouel · 2019-05-29T09:27:57Z

/hold

still working on this,

vdemeester · 2019-05-29T10:27:35Z

/test pull-tekton-pipeline-integration-tests

chmouel · 2019-05-29T12:01:57Z

/test pull-tekton-pipeline-integration-tests

chmouel · 2019-05-30T09:45:35Z

/hold cancel

yay this should now!! it took me a while to figure out because between my local dev env on openshift4 and on CI/k8 the behavior seems to be different, the volume are around a longer time after the test in OS4

Let's poll for up to 10mn that the PersistentVolumeClaims has the DeletionTimestamp set. Fixes tektoncd#915 Signed-off-by: Chmouel Boudjnah <[email protected]>

vdemeester

/lgtm

tekton-robot · 2019-05-30T10:14:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chmouel, vdemeester

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~~ [vdemeester]

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

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label May 28, 2019

tekton-robot requested review from bobcatfish and imjasonh May 28, 2019 13:42

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 28, 2019

vdemeester approved these changes May 28, 2019

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 28, 2019

tekton-robot assigned abayer May 28, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 28, 2019

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 28, 2019

chmouel force-pushed the testpipeline-wait-for branch from de2e035 to 5a86d48 Compare May 28, 2019 14:03

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label May 28, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 28, 2019

chmouel force-pushed the testpipeline-wait-for branch from 5a86d48 to cde21fc Compare May 28, 2019 15:14

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label May 28, 2019

vdemeester reviewed May 28, 2019

View reviewed changes

chmouel force-pushed the testpipeline-wait-for branch 2 times, most recently from 4a41fe5 to 30e2725 Compare May 29, 2019 09:27

chmouel force-pushed the testpipeline-wait-for branch 3 times, most recently from 66b3db8 to 8b5c155 Compare May 30, 2019 09:22

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 30, 2019

chmouel force-pushed the testpipeline-wait-for branch from 8b5c155 to 3c65e1e Compare May 30, 2019 09:57

Poll multiple times that the pvc is deleted in TestPipelineRun

c68128c

Let's poll for up to 10mn that the PersistentVolumeClaims has the DeletionTimestamp set. Fixes tektoncd#915 Signed-off-by: Chmouel Boudjnah <[email protected]>

chmouel force-pushed the testpipeline-wait-for branch from 3c65e1e to c68128c Compare May 30, 2019 09:58

tekton-robot assigned vdemeester May 30, 2019

vdemeester approved these changes May 30, 2019

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 30, 2019

tekton-robot merged commit f456845 into tektoncd:master May 30, 2019

chmouel deleted the testpipeline-wait-for branch December 19, 2019 10:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Poll multiple times that the pvc is deleted in TestPipelineRun #916

Poll multiple times that the pvc is deleted in TestPipelineRun #916

chmouel commented May 28, 2019 •

edited by vdemeester

Loading

vdemeester left a comment

abayer commented May 28, 2019

chmouel commented May 28, 2019

abayer commented May 28, 2019

chmouel commented May 28, 2019 •

edited

Loading

abayer commented May 28, 2019

chmouel commented May 28, 2019

vdemeester May 28, 2019

vdemeester May 28, 2019

chmouel May 29, 2019

chmouel commented May 29, 2019

chmouel commented May 29, 2019

vdemeester commented May 29, 2019

chmouel commented May 29, 2019

chmouel commented May 30, 2019

vdemeester left a comment

tekton-robot commented May 30, 2019

Poll multiple times that the pvc is deleted in TestPipelineRun #916

Poll multiple times that the pvc is deleted in TestPipelineRun #916

Conversation

chmouel commented May 28, 2019 • edited by vdemeester Loading

Changes

Submitter Checklist

vdemeester left a comment

Choose a reason for hiding this comment

abayer commented May 28, 2019

chmouel commented May 28, 2019

abayer commented May 28, 2019

chmouel commented May 28, 2019 • edited Loading

abayer commented May 28, 2019

chmouel commented May 28, 2019

vdemeester May 28, 2019

Choose a reason for hiding this comment

vdemeester May 28, 2019

Choose a reason for hiding this comment

chmouel May 29, 2019

Choose a reason for hiding this comment

chmouel commented May 29, 2019

chmouel commented May 29, 2019

vdemeester commented May 29, 2019

chmouel commented May 29, 2019

chmouel commented May 30, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented May 30, 2019

chmouel commented May 28, 2019 •

edited by vdemeester

Loading

chmouel commented May 28, 2019 •

edited

Loading