Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Pending PipelineRun status (TEP-0015) #3522

Merged
merged 1 commit into from
Jan 27, 2021

Conversation

jbarrick-mesosphere
Copy link
Contributor

@jbarrick-mesosphere jbarrick-mesosphere commented Nov 13, 2020

Changes

Implements the Pending PipelineRun status as described in TEP-0015. (based on #3223)

/kind feature

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
  • Release notes block has been filled in or deleted (only if no user facing changes)

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Added PipelineRunPending setting to PipelineRun Spec Status to allow creating PipelineRuns in a Pending state.

@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 13, 2020
@tekton-robot tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 13, 2020
@tekton-robot
Copy link
Collaborator

Hi @jbarrick-mesosphere. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jbarrick-mesosphere jbarrick-mesosphere changed the title Pause Implement Pending PipelineRun status (TEP-0015) Nov 13, 2020
Copy link
Collaborator

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jbarrick-mesosphere !!

What do you think about maybe including an end to end test as well? https://github.com/tektoncd/pipeline/tree/master/test#end-to-end-tests

that would give you some flexibility to do something like: create a pipelinerun with a quick timeout, but pending, make sure it doesn't time out, then remove pending and verify it succeeds or times out or something (i.e. will make sure more pieces work together)

@bobcatfish
Copy link
Collaborator

btw this is a relatively small PR so what im suggesting might be overkill but probably the fastest way to get this merged might be to break it up a little, e.g. if you had a PR with just the types and the validation, that could probably get merged a lot faster than this entire PR

@jbarrick-mesosphere
Copy link
Contributor Author

btw this is a relatively small PR so what im suggesting might be overkill but probably the fastest way to get this merged might be to break it up a little, e.g. if you had a PR with just the types and the validation, that could probably get merged a lot faster than this entire PR

Thanks, if this makes it significantly easier for you as a reviewer I'm happy to do this, but I'm in no rush to get the API changes merged in without the implementation.

@pritidesai
Copy link
Member

/ok-to-test

Logistics 😄 - please squash the commits

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 17, 2020
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
internal/builder/v1beta1/pipeline.go 86.1% 85.6% -0.5
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 75.4% 75.8% 0.4

@FogDong
Copy link
Member

FogDong commented Nov 17, 2020

Thanks! I'll close my original PR after this is merged😸

status: "PipelineRunPending"
```

To start the PipelineRun, delete the `.spec.status` field.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say "unset the .spec.status field, or update its value cancel it"

@@ -114,11 +114,16 @@ func PipelineDescription(desc string) PipelineSpecOp {
}
}

// PipelineRunCancelled sets the status to cancel to the TaskRunSpec.
// PipelineRunCancelled sets the status to cancel to the PielineRunSpec.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo here: PipelineRunSpec

@imjasonh
Copy link
Member

I notice that the TEP says it's a non-goal to support pending TaskRuns, "as that would requiring pausing the Kubernetes pods themselves" -- I think that's actually a bit misstated -- pending TaskRuns could just not create Pods, the same way that pending PipelineRuns just don't create TaskRuns. I think it's worth reconsidering implementing pending TaskRuns for consistency, or at least coming up with a better rationale for not doing so.

@bobcatfish
Copy link
Collaborator

Thanks, if this makes it significantly easier for you as a reviewer I'm happy to do this, but I'm in no rush to get the API changes merged in without the implementation.

I think I'm just over-applying this principle and this PR is pretty easy to review as it is 👍

@jbarrick-mesosphere
Copy link
Contributor Author

I notice that the TEP says it's a non-goal to support pending TaskRuns, "as that would requiring pausing the Kubernetes pods themselves" -- I think that's actually a bit misstated -- pending TaskRuns could just not create Pods, the same way that pending PipelineRuns just don't create TaskRuns. I think it's worth reconsidering implementing pending TaskRuns for consistency, or at least coming up with a better rationale for not doing so.

Nice catch, yeah this is left over from when the proposal was to actually pause a running pipeline.

@jbarrick-mesosphere
Copy link
Contributor Author

Thanks @jbarrick-mesosphere !!

What do you think about maybe including an end to end test as well? https://github.com/tektoncd/pipeline/tree/master/test#end-to-end-tests

Added an e2e test. 👍

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
internal/builder/v1beta1/pipeline.go 86.1% 85.6% -0.5
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 75.4% 75.8% 0.4
pkg/reconciler/pipelinerun/pipelinerun.go 84.4% 84.8% 0.4

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
internal/builder/v1beta1/pipeline.go 86.1% 85.6% -0.5
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 75.4% 75.8% 0.4
pkg/reconciler/pipelinerun/pipelinerun.go 84.4% 84.8% 0.4

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
internal/builder/v1beta1/pipeline.go 86.1% 85.6% -0.5
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 75.4% 75.8% 0.4
pkg/reconciler/pipelinerun/pipelinerun.go 84.4% 84.8% 0.4

Allows creation of Pending PipelineRuns which are not started until
the pending status is cleared.

Co-authored-by: Tianxin Dong <[email protected]>
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
internal/builder/v1beta1/pipeline.go 86.1% 85.6% -0.5
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 75.4% 75.8% 0.4
pkg/reconciler/pipelinerun/pipelinerun.go 84.4% 84.8% 0.4

@imjasonh
Copy link
Member

Nice catch, yeah this is left over from when the proposal was to actually pause a running pipeline.

I see. Is the intention to support creating pending TaskRuns as well then? Either way, can you update the TEP with this?

@jbarrick-mesosphere
Copy link
Contributor Author

I see. Is the intention to support creating pending TaskRuns as well then?

I don't currently have a use-case for this.

Either way, can you update the TEP with this?

I can update the TEP, but that is outside of the scope of this PR.

@imjasonh
Copy link
Member

I don't currently have a use-case for this.

The initial TEP proposal PR says it will cover TaskRuns: tektoncd/community#203

Another recent TEP, tektoncd/community#228, assumes pending TaskRuns to limit concurrency during PipelineRun execution.

@jbarrick-mesosphere
Copy link
Contributor Author

The initial TEP proposal PR says it will cover TaskRuns: tektoncd/community#203

I wrote the TEP, it doesn't say that.

Another recent TEP, tektoncd/community#228, assumes pending TaskRuns to limit concurrency during PipelineRun execution.

That would be a discussion to have on that TEP's PR.

@imjasonh
Copy link
Member

I wrote the TEP, it doesn't say that.

Hey, sorry, I think I spoke imprecisely. I just meant the PR's title as merged was "Add a pending setting to Tekton PipelineRun and TaskRuns", so it seemed like we had intended to support it at some point. It's fine that the TEP's scope eventually narrowed -- better that than the opposite! -- and that the actually-merged TEP didn't cover pending TaskRuns at all.

Your previous comment had been about lacking a use case, and I found the pipeline-concurrency TEP as a use case which was depending on pending TaskRuns, seemingly assuming they'd be added as part of your TEP. You're absolutely right that the pipeline-concurrency TEP is the place to discuss the addition of pending TaskRuns as a requirement for concurrency-limited pipelines.

(To be clear, none of this discussion should block this PR, which otherwise looks great to me.)

If you felt so inclined to extend this work in another PR to include pending TaskRuns, being as you have recent experience implementing such a change, I'm sure the authors of the pipeline-concurrency TEP would appreciate it. If that's outside your needs and you'd prefer the author of that TEP or the community at large to support pending TaskRuns, that's fine too. Thanks for your contributions of all kinds throughout this process. :)

@jbarrick-mesosphere
Copy link
Contributor Author

This should be ready for review.

@FogDong
Copy link
Member

FogDong commented Jan 4, 2021

Do we have any plans to move forward this PR? 🤔

@vincent-pli
Copy link
Member

Any process of the PR, here we have a case and expect to leverage this feature:

There is a specific task named terminator and when arrive the task, the pipeline should be pause, and before cancel need to do some cleanup work...
@imjasonh @bobcatfish

Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't realize this PR was still pending (pun intended), it looks great, and thanks for adding tests!

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 26, 2021
@@ -202,6 +207,8 @@ const (
// PipelineRunSpecStatusCancelled indicates that the user wants to cancel the task,
// if not already cancelled or terminated
PipelineRunSpecStatusCancelled = "PipelineRunCancelled"

PipelineRunSpecStatusPending = "PipelineRunPending"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think it's worth adding a comment here similar to PipelineRunSpecStatusCancelled describing its purpose. Something like:

// PipelineRunSpecStatusPending indicates that the user wants to postpone starting a PipelineRun
// until some condition is met

@ghost
Copy link

ghost commented Jan 27, 2021

Given how long this PR has been open and that I only have a nit about a comment, I'm happy to add that comment in a follow-up PR.

/approve

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants