fix: debug scripts are not mounted to steps with no scripts #4776

yuzp1996 · 2022-04-19T10:06:28Z

Changes

Update debug script mount logic

When the step script is empty then the logic which mounts the debug
script will skip for the container and the signal of placeScripts
will be the default value false which causes no debug script volume
will be mounted.

Now it will check the breakpoint first, if the breakpoint is not null
then will add volume Mount for container no matter the script
is empty or not.

And if the breakpoint is not null then will set the signal placeScripts
to be true while it used to be true only when the script is not empty.

related issue: #4613

/kind bug

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in
(if there are no user facing changes, use release note "NONE")

Release Notes

NONE

tekton-robot · 2022-04-19T10:06:42Z

Hi @yuzp1996. 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.

vdemeester · 2022-04-20T08:09:57Z

/ok-to-test

yuzp1996 · 2022-04-20T22:58:57Z

/retest

yuzp1996 · 2022-04-21T04:15:47Z

/retest

vdemeester

/cc @waveywaves

yuzp1996 · 2022-04-28T10:05:55Z

@vdemeester @bobcatfish @jerop @waveywaves I am sorry to bother you guys. If you have time to help review this PR I will be very appreciative! Thank you!

If there is anything I need to do please tell me. Thank you again!

waveywaves · 2022-04-28T10:10:26Z

Hey, thanks for the PR ! I'll take a look at it today.

waveywaves · 2022-05-03T14:58:54Z

Again, thanks for the PR.

I can see that debug is mounted if scripts are not present as well. I have tested this with the kn task.

kubectl exec -it kn-pod -- /bin/sh
Defaulted container "step-kn" out of: step-kn, place-tools (init), step-init (init), place-scripts (init)
/ # ls /tekton
bin          debug        home         run          termination
creds        downward     results      steps

waveywaves · 2022-05-03T15:01:03Z

pkg/pod/script.go

+		// If breakpoint is not nil then should add the init container
+		// to write debug script files


waveywaves · 2022-05-03T15:12:07Z

pkg/pod/pod_test.go

-			InitContainers: []corev1.Container{placeToolsInit, tektonDirInit(images.EntrypointImage, []v1beta1.Step{{Container: corev1.Container{Name: "name"}}})},
+			RestartPolicy: corev1.RestartPolicyNever,
+			InitContainers: []corev1.Container{placeToolsInit, tektonDirInit(images.EntrypointImage, []v1beta1.Step{{Container: corev1.Container{Name: "name"}}}),
+				{


Can you extract this Container as it's own variable called placeScriptsInit as you have done for containerVolumeMounts ? It would look much cleaner.

yuzp1996 · 2022-05-08T13:11:57Z

Again, thanks for the PR.

I can see that debug is mounted if scripts are not present as well. I have tested this with the kn task.
kubectl exec -it kn-pod -- /bin/sh
Defaulted container "step-kn" out of: step-kn, place-tools (init), step-init (init), place-scripts (init)
/ # ls /tekton
bin          debug        home         run          termination
creds        downward     results      steps

Hi, this is the task and taskrun I use to reproduce the error in a local deployment environment that is deployed with the latest commit from the master branch

Task with script 

apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: hello
spec:
  steps:
    - name: hello
      image: ubuntu
      env:
      - name: WORKSPACE_SOURCE_PATH
        value: $(workspaces.source.path)
      - name: WORKSPACE_ARTIFACTS_PATH
        value: $(workspaces.artifacts.path)
      script: |
        #!/usr/bin/env bash
        echodd "Hello from Bash!"

Task with args and command

apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: hello
spec:
  steps:
    - name: hello
      image: ubuntu
      env:
      - name: WORKSPACE_SOURCE_PATH
        value: $(workspaces.source.path)
      - name: WORKSPACE_ARTIFACTS_PATH
        value: $(workspaces.artifacts.path)
      command:
        - echodd
      args:
        - "Hello from Bash!"

Above tasks can be referred to by the same taskrun 

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  generateName: hello-run-arg-
spec:
  debug:
    breakpoint: [ "onFailure" ]
  taskRef:
    name: hello

this is the result that I get. @waveywaves

When the step script is empty then the logic which mounts the debug script will skip for the container and the signal of placeScripts will be the default value false which causes no debug script volume will be mounted. Now it will check the breakpoint first, if the breakpoint is not null then will add volume Mount for container no matter weather the script is empty or not. And if the breakpoint is not null then will set the signal placeScripts to be true while it used to be true only when the script is not empty. related issue: tektoncd#4613 Signed-off-by: yuzhipeng <[email protected]>

yuzp1996 · 2022-05-08T13:32:18Z

Because there were some conflict changes I spend some time fixing that. Now I have modified the code and the PR is ready to be merged. I will be very appreciative if you can review this PR.

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

Thanks again! @waveywaves

yuzp1996 · 2022-05-08T13:55:33Z

/retest

yuzp1996 · 2022-05-11T22:51:00Z

pkg/pod/pod.go

@@ -371,7 +371,7 @@ func makeLabels(s *v1beta1.TaskRun) map[string]string {
 	return labels
 }

-// shouldAddReadyAnnotationonPodCreate returns a bool indicating whether the
+// shouldAddReadyAnnotationOnPodCreate returns a bool indicating whether the


I find that the comment does not start with the function name, so I change it.

yuzp1996 · 2022-05-11T22:53:56Z

pkg/pod/script.go

@@ -130,9 +130,18 @@ func convertScripts(shellImageLinux string, shellImageWin string, steps []v1beta
 func convertListOfSteps(steps []v1beta1.Step, initContainer *corev1.Container, placeScripts *bool, breakpoints []string, namePrefix string) []corev1.Container {
 	containers := []corev1.Container{}
 	for i, s := range steps {
+		// Add debug mounts if breakpoints are present


I move the logic of adding debug script before judging the script make sure it can work well no matter s.Script is empty or not.

yuzp1996 · 2022-05-11T23:02:12Z

Hi @waveywaves, I am sorry to bother you again. I fixed the code conflict and add some comments to help understand the change I made. I would be very grateful if you could take the time to help me review this PR and give me your valuable suggestions. Thanks!

waveywaves · 2022-05-15T17:14:45Z

Hey @yuzp1996
This looks good to me ! I ran it once again and can confirm that it works. Thanks for fixing this one 💯

/lgtm

yuzp1996 · 2022-05-16T02:59:45Z

Hey @yuzp1996 This looks good to me ! I ran it once again and can confirm that it works. Thanks for fixing this one 💯

/lgtm

Thanks for your lgtm! It is important to me! @waveywaves

Is there anything else I need to do to merge this PR？ Maybe an approval? @waveywaves @vdemeester

Sorry, I don't know if I should be in such a hurry. If there is anything I did that made you feel not good please tell me and I will be happy to accept your suggestions. Thanks again!

abayer · 2022-05-17T16:25:00Z

/assign
/assign @vdemeester

tekton-robot · 2022-05-20T08:32:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 19, 2022

tekton-robot requested review from bobcatfish and jerop April 19, 2022 10:06

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 19, 2022

yuzp1996 changed the title ~~fix: debug scripts are not mounted to steps with no scripts~~ [WIP]fix: debug scripts are not mounted to steps with no scripts Apr 19, 2022

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 19, 2022

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 Apr 20, 2022

yuzp1996 force-pushed the fix/arg-taskrun-no-debug-script branch from b97c1cd to 41f2844 Compare April 20, 2022 22:44

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 20, 2022

yuzp1996 force-pushed the fix/arg-taskrun-no-debug-script branch from 41f2844 to adb43ec Compare April 20, 2022 22:47

yuzp1996 closed this Apr 20, 2022

yuzp1996 reopened this Apr 20, 2022

yuzp1996 changed the title ~~[WIP]fix: debug scripts are not mounted to steps with no scripts~~ fix: debug scripts are not mounted to steps with no scripts Apr 20, 2022

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 20, 2022

yuzp1996 force-pushed the fix/arg-taskrun-no-debug-script branch from adb43ec to e7d18df Compare April 21, 2022 03:02

vdemeester reviewed Apr 21, 2022

View reviewed changes

tekton-robot requested a review from waveywaves April 21, 2022 08:29

waveywaves reviewed May 3, 2022

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2022

yuzp1996 force-pushed the fix/arg-taskrun-no-debug-script branch from e7d18df to 87a4d99 Compare May 8, 2022 13:25

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2022

yuzp1996 commented May 11, 2022

View reviewed changes

tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label May 12, 2022

yuzp1996 requested review from vdemeester and waveywaves and removed request for waveywaves May 12, 2022 23:01

tekton-robot assigned waveywaves May 15, 2022

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

tekton-robot assigned abayer and vdemeester May 17, 2022

vdemeester approved these changes May 20, 2022

View reviewed changes

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

tekton-robot merged commit e6c7eb0 into tektoncd:main May 20, 2022

lbernick mentioned this pull request May 24, 2022

Debug scripts are not mounted to steps with no scripts #4613

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: debug scripts are not mounted to steps with no scripts #4776

fix: debug scripts are not mounted to steps with no scripts #4776

yuzp1996 commented Apr 19, 2022 •

edited

Loading

tekton-robot commented Apr 19, 2022

vdemeester commented Apr 20, 2022

yuzp1996 commented Apr 20, 2022

yuzp1996 commented Apr 21, 2022

vdemeester left a comment

yuzp1996 commented Apr 28, 2022

waveywaves commented Apr 28, 2022

waveywaves commented May 3, 2022

waveywaves May 3, 2022

waveywaves May 3, 2022

yuzp1996 May 5, 2022

yuzp1996 commented May 8, 2022 •

edited

Loading

yuzp1996 commented May 8, 2022

yuzp1996 commented May 8, 2022

yuzp1996 May 11, 2022 •

edited

Loading

yuzp1996 May 11, 2022

yuzp1996 commented May 11, 2022

waveywaves commented May 15, 2022

yuzp1996 commented May 16, 2022

abayer commented May 17, 2022

tekton-robot commented May 20, 2022

		// If breakpoint is not nil then should add the init container
		// to write debug script files

fix: debug scripts are not mounted to steps with no scripts #4776

fix: debug scripts are not mounted to steps with no scripts #4776

Conversation

yuzp1996 commented Apr 19, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Apr 19, 2022

vdemeester commented Apr 20, 2022

yuzp1996 commented Apr 20, 2022

yuzp1996 commented Apr 21, 2022

vdemeester left a comment

Choose a reason for hiding this comment

yuzp1996 commented Apr 28, 2022

waveywaves commented Apr 28, 2022

waveywaves commented May 3, 2022

waveywaves May 3, 2022

Choose a reason for hiding this comment

waveywaves May 3, 2022

Choose a reason for hiding this comment

yuzp1996 May 5, 2022

Choose a reason for hiding this comment

yuzp1996 commented May 8, 2022 • edited Loading

yuzp1996 commented May 8, 2022

yuzp1996 commented May 8, 2022

yuzp1996 May 11, 2022 • edited Loading

Choose a reason for hiding this comment

yuzp1996 May 11, 2022

Choose a reason for hiding this comment

yuzp1996 commented May 11, 2022

waveywaves commented May 15, 2022

yuzp1996 commented May 16, 2022

abayer commented May 17, 2022

tekton-robot commented May 20, 2022

yuzp1996 commented Apr 19, 2022 •

edited

Loading

yuzp1996 commented May 8, 2022 •

edited

Loading

yuzp1996 May 11, 2022 •

edited

Loading