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

WIP: TaskRun Debug #2331

Closed
wants to merge 2 commits into from
Closed

Conversation

waveywaves
Copy link
Member

@waveywaves waveywaves commented Apr 5, 2020

.spec.debug bool, is added to TaskRun API which enables
debug mode for the particular TaskRun.

Co-authored-by: Vibhav Bobade vbobade@redhat.com
Co-authored-by: Nikhil Thomas nikthoma@redhat.com

Changes

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:

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

Describe any user facing changes here, or delete this block.

Examples of user facing changes:
- API changes
- Bug fixes
- Any changes in behavior

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2020
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign imjasonh
You can assign the PR to them by writing /assign @imjasonh in a comment when ready.

The full list of commands accepted by this bot can be found 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

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 5, 2020
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/taskrun_conversion.go 95.2% 95.4% 0.1

@imjasonh
Copy link
Member

imjasonh commented Apr 5, 2020

Can you add more details to the PR description and commit message to describe what debug mode does? Is there any supporting design doc with more info?

@waveywaves
Copy link
Member Author

@imjasonh
Copy link
Member

imjasonh commented Apr 6, 2020

@imjasonh https://docs.google.com/document/d/1SWtGfkGOaeggqj3zY4pyJo1cJz0hBw9wStbIWkCPnL0/edit#

Thanks, that's helpful. Adding this to the PR description and commit message will make this design more discoverable to future spelunkers.

Update Entrypoint Runner to propogate Signals by having a
chan being notified for them, and reading the chan at the
end of the Run to propogate all caught signals to the
main process.
It is a difficult task to do a RCA (Root Cause Analysis) of a failed TaskRun or PipelineRun. We often find ourselves changing the inputs, outputs and result parameters in the YAML definitions which turns developers more into YAML engineers for the CI/CD System instead of the CI/CD as an auxiliary tool for empowering the developer. 

Technical details for this are given here
https://docs.google.com/document/d/1SWtGfkGOaeggqj3zY4pyJo1cJz0hBw9wStbIWkCPnL0/edit#

Co-authored-by: Vibhav Bobade vbobade@redhat.com
Co-authored-by: Nikhil Thomas nikthoma@redhat.com
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to determine that you authored the commits in this PR. Maybe you used a different email address in the git commits than was used to sign the CLA? If someone else authored these commits, then please add them to this pull request and have them confirm that they're okay with them being contributed to Google. If there are co-authors, make sure they're formatted properly.

In order to pass this check, please resolve this problem and then comment@googlebot I fixed it... If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/taskrun_conversion.go 95.2% 95.4% 0.1

@tekton-robot
Copy link
Collaborator

@waveywaves: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-tekton-pipeline-unit-tests daec24f link /test tekton-pipeline-unit-tests
pull-tekton-pipeline-build-tests daec24f link /test pull-tekton-pipeline-build-tests
pull-tekton-pipeline-integration-tests daec24f link /test pull-tekton-pipeline-integration-tests

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@imjasonh
Copy link
Member

imjasonh commented Apr 8, 2020

Some thoughts on this design, that I didn't see covered in the doc:

  1. The term "debug" means quite a lot of things, and might be confusing since this feature doesn't, for instance, cover stepping through/in/out of operations in the container. I realize there's historical context around the field in Openshift, but I don't think we should necessarily carry it over without thinking it through. This might make more sense as breakpoint: true, and ties into the second point:
  2. I think we should look toward a future where there might be more than two options for the field. That is, breakpoint: true and breakpoint: false might be overly limiting. What if we wanted to add a breakpoint: OnlyOnFailure or OnlyOnTimeout or something? Supporting an enum (string) today makes this an easier change in the future.
  3. Are we anticipating tkn CLI surfaces to interact with this feature? I didn't see them discussed in the doc. Things like tkn unpause to nudge past the breakpoint, or tkn connect to streamline connecting to the paused container? In the absence of these it can be a bit hard to really gauge how useful this would be, and therefore whether it's worth adopting and supporting.

@bobcatfish
Copy link
Collaborator

@waveywaves I'm going to close this PR for now, I think the next step here would be to expand the design proposal to include @imjasonh's recommendations, which I strongly 👍 (I'd love to see more specifically about the API this will expose to debugging tools, e.g. CLI, vscode plugin).

Please reopen or let me know if you disagree!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: no do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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.

5 participants