Add the step name in taskrun status #657

skeeey · 2019-03-22T09:59:00Z

Changes

refer to #549

Add a step name to taskrun status to help user to trace their steps status

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

See the contribution guide
for more details.

Test

apply the following task

...
kind: Task
...
spec:
  steps:
    - name: step1
      image: ubuntu
      command:
        - echo
      args:
        - "hello world 1"
    - name: step2
      image: ubuntu
      command:
        - echo
      args:
        - "hello world 2"

Then apply a taskrun to trigger this task, we can watch the each step status, like:

...

status:
  conditions:
  - lastTransitionTime: 2019-03-22T08:53:26Z
    status: "True"
    type: Succeeded
  podName: steps-task-run-pod-354cda
  startTime: 2019-03-22T08:52:58Z
  steps:
  - name: step1
    terminated:
      containerID: docker://85f1c1df656df87a3eb15bf68a9ff0776ee0b399a9a6e41abcebcc459e981282
      exitCode: 0
      finishedAt: 2019-03-22T08:53:03Z
      reason: Completed
      startedAt: 2019-03-22T08:53:03Z
  - name: step2
    terminated:
      containerID: docker://2b70e22616b43cf0cf407d95180bc3bfb69fd1d29809beee1e27a7e3e000574b
      exitCode: 0
      finishedAt: 2019-03-22T08:53:05Z
      reason: Completed
      startedAt: 2019-03-22T08:53:05Z
  - name: nop
    terminated:
      containerID: docker://b7af1e688c434489535a43772e5ed3608c4e3a0ce7580c0b460dda38517d2689
      exitCode: 0
      finishedAt: 2019-03-22T08:53:05Z
      reason: Completed
      startedAt: 2019-03-22T08:53:05Z

nader-ziada · 2019-03-23T19:57:17Z

Thanks for the PR @skeeey, but would you mind adding some test to cover your new change

nader-ziada · 2019-03-23T19:57:50Z

/ok-to-test

skeeey · 2019-03-25T06:05:58Z

/retest

skeeey · 2019-03-25T07:28:06Z

@pivotal-nader-ziada thanks for your review, I add test case to make sure the step name in the step state, but the integration test failed, I cannot locate where the error occurs, it seems to be an env problem. would you help me to have a look?

vdemeester · 2019-03-25T07:50:17Z

@skeeey yeah there is some weirdness on the CI / integration tests, I'm looking into it 😉

vdemeester · 2019-03-25T13:21:58Z

/test pull-tekton-pipeline-integration-tests

vdemeester

Can you add some tests cases ? TestUpdateStatusFromPod with containers's name being named with the different prefixes

skeeey · 2019-03-27T05:01:38Z

@vdemeester I enhance the test cases: success, running and failure, and find a small problem, if a step failed, there are will be a message, e.g.

message: 'build step "build-step-build-and-push" exited with code 1 ...'

the message build step "build-step-build-and-push is verbose and not clear, I also trim the prefix build-step in this message, the fixed message will be

message: 'build step "build-and-push" exited with code 1 ...'

nader-ziada · 2019-03-27T13:56:12Z

The "build-step-build-and-push is the actual name of the step which will be similar to the name of the container, so I suggest you remove the build step from the error message and keep the build step name the same so its less confusing.
Also if the standard of adding the words build step to the name of the container ever changes, this won't break

Thanks

vdemeester

@skeeey ah looking at the build failure, it's kinda my fault 😅

taskrun_test.go:105: -got, +want: {[]v1alpha1.StepState}[0].Name:
-: "exit"
+: ""
    {[]v1alpha1.StepState}[1].Name:
-: "hello"
+: ""
    {[]v1alpha1.StepState}[2].Name:
-: "world"
+: ""
    {[]v1alpha1.StepState}[3].Name:
-: "nop"
+: ""

You're gonna have to update the test/taskrun_test.go expectedState array to add the Name field in there 🙇‍♂️

Also, can you squash your commit ? 👼

nader-ziada · 2019-03-28T12:50:56Z

/check-cla

skeeey · 2019-03-28T14:08:32Z

@vdemeester I try to squash my commits, but it failed 😅, so I reset my branch and force push it ...

I correct the e2e test case, but the integration test TestTaskRunTimeout is failure, do you have any suggestions

vdemeester · 2019-03-28T15:13:57Z

@skeeey sorry for being a pita but there is stiil a need to squash 😅 Ping me on knative's slack (#build-pipeline) if you need help for squashing them 😉 🤗

skeeey · 2019-03-29T04:46:29Z

@vdemeester thanks, I make all commits to one commit

hrishin · 2019-04-01T14:51:04Z

@vdemeester @pivotal-nader-ziada is anything pending in this PR?

nader-ziada · 2019-04-01T17:52:29Z

pkg/reconciler/v1alpha1/taskrun/resources/pod.go

+
+// TrimContainerNamePrefix trim the container name prefix to get the corresponding step name
+func TrimContainerNamePrefix(containerName string) string {
+	return strings.TrimPrefix(containerName, containerPrefix)


are you removing the prefix "build-step" but keeping the random string in the end of the name?

@pivotal-nader-ziada indeed, the random string in the end is still there, we should trim it too

@pivotal-nader-ziada @vdemeester yes, this method only trims the build-step prefix, for this case, the method is used to trim the build step name, the build step name consists of build-step prefix and the task step name, the task step name is defined by user, there should be no random string in it unless user defined it, right?

@skeeey the pod actually get a random suffix at the end of the name — and are limited in size. I think we should actually use the pod labels to get the task name (see below an extract of a describe on a pod)

Name: build-push-run-pod-04cde6 Namespace: default Priority: 0 PriorityClassName: <none> Node: minikube/192.168.122.229 Start Time: Tue, 02 Apr 2019 12:17:01 +0200 Labels: tekton.dev/task=build-push-kaniko tekton.dev/taskRun=build-push-run

@vdemeester I'm confused :-), why we need a task name？in this case, we want to add the step name to each step that is in the taskrun status, and in fact, each task step corresponds to a container in a pod, we can get the container name from ContainerStatus directly, and the container name consists of build-step prefix and the task step name, I think we can trim the build-step prefix to get the actual step name, is my understand right?

@skeeey lol sorry, I messed up 😂 You're right 😉

@skeeey right so we just need to trim the last 7 characters of the pod name then (to get a clean step name 👼 )

nader-ziada · 2019-04-01T17:53:28Z

added a question about the name trimming? but @vdemeester had comments after I reviewed so will wait for him to take a look and decide

skeeey · 2019-04-04T01:43:11Z

@vdemeester any more comments?

vdemeester

@skeeey two points

The PR needs a rebase, sorry 🙇‍♂️
There is some case if the name of the step is too long (see here) — I am not entirely sure what to do in that case 😓 cc @abayer

skeeey · 2019-04-10T04:24:29Z

@vdemeester I rebased the PR, for second problem, Can we limit the length of the step name when users apply their task (additional, we may also need to valid the step name, the name should be consisted of ^[a-zA-Z0-9]+$, right?), currently, the max length is 63, then we subtract the length of build-step- prefix (11), the validate step name length is 52, I think the length is enough for a user to define his/her step name

bobcatfish · 2019-04-16T23:10:56Z

I think/hope we no longer will have step names that are too long but I might be missing something @vdemeester !

I also want to throw out two wildcard suggestions:

what if we didn't add build-step to any of the container names
i think we should use the taskrun names exactly as is - the mapping b/w the Pipeline and the name isn't 100% clear BUT it has the advantage of actually being the real name of the TaskRun which ppl can use to look up the TaskRun if they want more info

(BUT I think we should also merge this as-is and iterate so @skeeey isn't stuck for too much longer. @skeeey it looks like a rebase is required again 😓 )

bobcatfish · 2019-04-22T18:45:11Z

I think this is looking pretty good - will leave for @vdemeester to make the final call on! (I think today is a holiday for him tho... 😇 )

vdemeester

/lgtm

tekton-robot · 2019-04-23T07:21:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: skeeey, 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 Mar 22, 2019

tekton-robot requested review from imjasonh and vdemeester March 22, 2019 09:59

tekton-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 22, 2019

tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 23, 2019

vdemeester requested changes Mar 26, 2019

View reviewed changes

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 27, 2019

vdemeester reviewed Mar 28, 2019

View reviewed changes

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 28, 2019

skeeey closed this Mar 28, 2019

tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 28, 2019

skeeey reopened this Mar 28, 2019

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 28, 2019

nader-ziada reviewed Apr 1, 2019

View reviewed changes

vdemeester reviewed Apr 8, 2019

View reviewed changes

Add the step name in taskrun status

d4d299e

vdemeester approved these changes Apr 23, 2019

View reviewed changes

tekton-robot assigned vdemeester Apr 23, 2019

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

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

tekton-robot merged commit 3415c18 into tektoncd:master Apr 23, 2019

AlanGreene mentioned this pull request Apr 23, 2019

Task's step(s) showing as success even if they are not tektoncd/dashboard#56

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the step name in taskrun status #657

Add the step name in taskrun status #657

skeeey commented Mar 22, 2019 •

edited

Loading

nader-ziada commented Mar 23, 2019

nader-ziada commented Mar 23, 2019

skeeey commented Mar 25, 2019

skeeey commented Mar 25, 2019

vdemeester commented Mar 25, 2019

vdemeester commented Mar 25, 2019

vdemeester left a comment

skeeey commented Mar 27, 2019 •

edited

Loading

nader-ziada commented Mar 27, 2019

vdemeester left a comment

nader-ziada commented Mar 28, 2019

skeeey commented Mar 28, 2019

vdemeester commented Mar 28, 2019

skeeey commented Mar 29, 2019

hrishin commented Apr 1, 2019

nader-ziada Apr 1, 2019

vdemeester Apr 2, 2019

skeeey Apr 2, 2019 •

edited

Loading

vdemeester Apr 2, 2019

skeeey Apr 2, 2019

vdemeester Apr 2, 2019

vdemeester Apr 3, 2019

nader-ziada commented Apr 1, 2019

skeeey commented Apr 4, 2019

vdemeester left a comment •

edited

Loading

skeeey commented Apr 10, 2019

bobcatfish commented Apr 16, 2019

bobcatfish commented Apr 22, 2019

vdemeester left a comment

tekton-robot commented Apr 23, 2019

Add the step name in taskrun status #657

Add the step name in taskrun status #657

Conversation

skeeey commented Mar 22, 2019 • edited Loading

Changes

Submitter Checklist

Test

nader-ziada commented Mar 23, 2019

nader-ziada commented Mar 23, 2019

skeeey commented Mar 25, 2019

skeeey commented Mar 25, 2019

vdemeester commented Mar 25, 2019

vdemeester commented Mar 25, 2019

vdemeester left a comment

Choose a reason for hiding this comment

skeeey commented Mar 27, 2019 • edited Loading

nader-ziada commented Mar 27, 2019

vdemeester left a comment

Choose a reason for hiding this comment

nader-ziada commented Mar 28, 2019

skeeey commented Mar 28, 2019

vdemeester commented Mar 28, 2019

skeeey commented Mar 29, 2019

hrishin commented Apr 1, 2019

nader-ziada Apr 1, 2019

Choose a reason for hiding this comment

vdemeester Apr 2, 2019

Choose a reason for hiding this comment

skeeey Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

vdemeester Apr 2, 2019

Choose a reason for hiding this comment

skeeey Apr 2, 2019

Choose a reason for hiding this comment

vdemeester Apr 2, 2019

Choose a reason for hiding this comment

vdemeester Apr 3, 2019

Choose a reason for hiding this comment

nader-ziada commented Apr 1, 2019

skeeey commented Apr 4, 2019

vdemeester left a comment • edited Loading

Choose a reason for hiding this comment

skeeey commented Apr 10, 2019

bobcatfish commented Apr 16, 2019

bobcatfish commented Apr 22, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Apr 23, 2019

skeeey commented Mar 22, 2019 •

edited

Loading

skeeey commented Mar 27, 2019 •

edited

Loading

skeeey Apr 2, 2019 •

edited

Loading

vdemeester left a comment •

edited

Loading