Unit Tests for Custom Task Retries

[lbernick]

Custom Task restries were added in #4327; however, some parts of the code base were missed and addressed in #4625 and #4647. We should test Custom Task retries E2E, fix any bugs if necessary, and increase our test coverage of this functionality.

[yuzp1996]

/assign

[yuzp1996]

Hi @lbernick it is my first time writing an e2e test. I went through the custom task document and the e2e test file existing in the code base. And I want to sync with you about what I want to do and looking forward to your suggestion!

I read the document Tekton runs and IBM introducing about custom task as well as the e2e retry test. I want to write an e2e test like e2e retry test and will first create the custom task controller and pipeline with ko and then check the run's status.

I am not good at English now if there is any offence please tell me and forgive me. Thanks!

[lbernick]

Thanks so much @yuzp1996, I appreciate the enthusiasm!

I know I said "test E2E in the original issue", but I think the best place here to start would be to add some more test coverage of custom task retries to the pipelinerun reconciler. This existing test case is the closest thing we currently have, but we could really use a test case that reconciles a pipelinerun with a custom task with retries, and verify that if the most recent attempt failed, the pipelinerun reconciler restarts the custom task if it has retries remaining (and does not restart the custom task if there are no retries remaining).

If you'd like to also add E2E tests that run on a cluster, I would recommend against using the task-loops custom task controller you've linked. Instead, I think it would be useful to have a custom task controller that is meant just for testing. The controller could set the custom task status based on the custom task spec (or just set the status to succeeded to start). I think a controller like this would be generally useful for developers adding features to runs who want to test things out on their cluster.

I'm at a conference this week but I am happy to help more next week if needed! Also feel free to contact me on the tekton slack.

[yuzp1996]

Thanks so much @yuzp1996, I appreciate the enthusiasm!

I know I said "test E2E in the original issue", but I think the best place here to start would be to add some more test coverage of custom task retries to the pipelinerun reconciler. This existing test case is the closest thing we currently have, but we could really use a test case that reconciles a pipelinerun with a custom task with retries, and verify that if the most recent attempt failed, the pipelinerun reconciler restarts the custom task if it has retries remaining (and does not restart the custom task if there are no retries remaining).

If you'd like to also add E2E tests that run on a cluster, I would recommend against using the task-loops custom task controller you've linked. Instead, I think it would be useful to have a custom task controller that is meant just for testing. The controller could set the custom task status based on the custom task spec (or just set the status to succeeded to start). I think a controller like this would be generally useful for developers adding features to runs who want to test things out on their cluster.

I'm at a conference this week but I am happy to help more next week if needed! Also feel free to contact me on the tekton slack.

Thanks for your reply, your suggestion is very important to me. I will do as you suggest. If I need any help, I will try to contact you. Thanks again!

[pritidesai]

All pipeline e2e tests and examples run against a common configuration of the tekton controller. This will help a lot if we can enable custom task e2e tests or examples in our CI (dogfooding cluster).

One option is to work with @afrittoli to enable custom configurations for these kind of tests where an alpha feature can be enabled (custom task is still alpha) and a custom controller is deployed. See: tektoncd/plumbing#1017

Until we have e2e tests, will be great to add more unit tests for the controller.

[pritidesai]

@afrittoli @imjasonh Can we treat the tekton controller as a custom task controller instead of writing and deploying a test custom controller? In a long run, it will be great to have a dedicated test custom controller but just to unblock the testing for now.

[imjasonh]

@afrittoli @imjasonh Can we treat the tekton controller as a custom task controller instead of writing and deploying a test custom controller? In a long run, it will be great to have a dedicated test custom controller but just to unblock the testing for now.

Do you mean have the standard Tekton controller also watch for Runs of a particular type, and operate on them? That might be a path forward, but I would want to make sure that users know this isn't the way the Tekton controller should be used in normal operation. The e2e tests would setup Tekton configured to also operate in that test mode.

At that point it might be easier just to write a simple external controller that watches Runs and marks them as failed/successful or waits indefinitely for timeout, and only install that controller during e2e tests. That would probably be easier to mark as not intended for external use.

[vdemeester]

@pritidesai I tend to agree with @imjasonh, and I don't really think it would make sense to make the main controller act as a custom task controller just for the tests (if we want to provide "default built-in" custom task, maybe we could do that, but even, it would probably make sense to extract it in another deployment).

[lbernick]

Created #5120 to track a custom Task controller; this issue can just be to track unit tests.

[JeromeJu]

/assign

[JeromeJu]

This comment summarizes the findings after digging into TEP-69 and checks the possibilities of test coverage improvement, suggesting that this can be closed.

• TEP69 design related:
  The implementation of TEP-69: Support retries for custom task in a pipeline passes down the retries field and let the custom-task controller deal with the actual retries logics.

  Thanks to the discussions of @bobcatfish and @ScrapCodes:

  Reason why not trying to use the same approach used for retrying TaskRuns (using [retriesstatus] (https://github.com/tektoncd/pipeline/blob/f041ef5d644c72e88689e079a8e3ea96718891fc/pkg/apis/pipeline/v1beta1/taskr un_types.go#L201-L204)) - it would be a bit weird b/c it would mean the PIipelines controller would be changing the status of the Run which I think up until this point we've avoided (i.e. it's the custom task controller's responsibility to deal with status)

  The test plan changed according to the above reference for controllers' cohesiveness. And therefore, the current test cases regarding retries are sufficient on the level of unit-tests.

• Test Coverage:
  getRetryableTask is worth notifying and it has been covered by its caller and tested in pipelinerunstate
  For possible improvements, this function could be decoupled as retries-related. Further refactorization would be required.

• Follow up:
  Custom controller behaviour with no retries support #5182 as a follow up issue, addresses the cases where the custom controller does not support retries and it could be further improved.

Thanks to @lbernick for the guidance!