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.

Sign up for GitHub

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

Add the step name in taskrun status #657

Merged
merged 1 commit into from
Apr 23, 2019
Merged

Add the step name in taskrun status #657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/apis/pipeline/v1alpha1/taskrun_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ func (tr *TaskRunStatus) SetCondition(newCond *apis.Condition) {
// StepState reports the results of running a step in the Task.
type StepState struct {
corev1.ContainerState
Name string `json:"name,omitempty"`
}

// +genclient
Expand Down
6 changes: 6 additions & 0 deletions pkg/reconciler/v1alpha1/taskrun/resources/pod.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"io"
"io/ioutil"
"path/filepath"
"strings"

"go.uber.org/zap"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -311,3 +312,8 @@ func findMaxResourceRequest(containers []corev1.Container, resourceNames ...core
}
return maxIdxs
}

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@skeeey skeeey Apr 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

}
3 changes: 2 additions & 1 deletion pkg/reconciler/v1alpha1/taskrun/taskrun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,7 @@ func updateStatusFromPod(taskRun *v1alpha1.TaskRun, pod *corev1.Pod) {
for _, s := range pod.Status.ContainerStatuses {
taskRun.Status.Steps = append(taskRun.Status.Steps, v1alpha1.StepState{
ContainerState: *s.State.DeepCopy(),
Name: resources.TrimContainerNamePrefix(s.Name),
})
}

Expand Down Expand Up @@ -413,7 +414,7 @@ func getFailureMessage(pod *corev1.Pod) string {
for _, status := range pod.Status.ContainerStatuses {
term := status.State.Terminated
if term != nil && term.ExitCode != 0 {
return fmt.Sprintf("build step %q exited with code %d (image: %q); for logs run: kubectl -n %s logs %s -c %s",
return fmt.Sprintf("%q exited with code %d (image: %q); for logs run: kubectl -n %s logs %s -c %s",
status.Name, term.ExitCode, status.ImageID,
pod.Namespace, pod.Name, status.Name)
}
Expand Down
57 changes: 45 additions & 12 deletions pkg/reconciler/v1alpha1/taskrun/taskrun_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1269,10 +1269,11 @@ func TestUpdateStatusFromPod(t *testing.T) {
Conditions: []apis.Condition{conditionRunning},
},
Steps: []v1alpha1.StepState{{
corev1.ContainerState{
ContainerState: corev1.ContainerState{
Terminated: &corev1.ContainerStateTerminated{
ExitCode: 123,
}},
Name: "state-name",
}},
},
}, {
Expand All @@ -1297,31 +1298,61 @@ func TestUpdateStatusFromPod(t *testing.T) {
Conditions: []apis.Condition{conditionRunning},
},
Steps: []v1alpha1.StepState{{
corev1.ContainerState{
ContainerState: corev1.ContainerState{
Terminated: &corev1.ContainerStateTerminated{
ExitCode: 123,
}},
Name: "state-name",
}},
},
}, {
desc: "success",
podStatus: corev1.PodStatus{Phase: corev1.PodSucceeded},
desc: "success",
podStatus: corev1.PodStatus{
Phase: corev1.PodSucceeded,
ContainerStatuses: []corev1.ContainerStatus{{
Name: "build-step-build-step-push",
State: corev1.ContainerState{
Terminated: &corev1.ContainerStateTerminated{
ExitCode: 0,
},
},
}},
},
want: v1alpha1.TaskRunStatus{
Status: duckv1beta1.Status{
Conditions: []apis.Condition{conditionTrue},
},
Steps: []v1alpha1.StepState{},
Steps: []v1alpha1.StepState{{
ContainerState: corev1.ContainerState{
Terminated: &corev1.ContainerStateTerminated{
ExitCode: 0,
}},
Name: "build-step-push",
}},
// We don't actually care about the time, just that it's not nil
CompletionTime: &metav1.Time{Time: time.Now()},
},
}, {
desc: "running",
podStatus: corev1.PodStatus{Phase: corev1.PodRunning},
desc: "running",
podStatus: corev1.PodStatus{
Phase: corev1.PodRunning,
ContainerStatuses: []corev1.ContainerStatus{{
Name: "build-step-running-step",
State: corev1.ContainerState{
Running: &corev1.ContainerStateRunning{},
},
}},
},
want: v1alpha1.TaskRunStatus{
Status: duckv1beta1.Status{
Conditions: []apis.Condition{conditionBuilding},
},
Steps: []v1alpha1.StepState{},
Steps: []v1alpha1.StepState{{
ContainerState: corev1.ContainerState{
Running: &corev1.ContainerStateRunning{},
},
Name: "running-step",
}},
},
}, {
desc: "failure-terminated",
Expand All @@ -1331,7 +1362,7 @@ func TestUpdateStatusFromPod(t *testing.T) {
// creds-init status; ignored
}},
ContainerStatuses: []corev1.ContainerStatus{{
Name: "status-name",
Name: "build-step-failure",
ImageID: "image-id",
State: corev1.ContainerState{
Terminated: &corev1.ContainerStateTerminated{
Expand All @@ -1345,14 +1376,15 @@ func TestUpdateStatusFromPod(t *testing.T) {
Conditions: []apis.Condition{{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Message: `build step "status-name" exited with code 123 (image: "image-id"); for logs run: kubectl -n foo logs pod -c status-name`,
Message: `"build-step-failure" exited with code 123 (image: "image-id"); for logs run: kubectl -n foo logs pod -c build-step-failure`,
}},
},
Steps: []v1alpha1.StepState{{
corev1.ContainerState{
ContainerState: corev1.ContainerState{
Terminated: &corev1.ContainerStateTerminated{
ExitCode: 123,
}},
Name: "failure",
}},
// We don't actually care about the time, just that it's not nil
CompletionTime: &metav1.Time{Time: time.Now()},
Expand Down Expand Up @@ -1416,11 +1448,12 @@ func TestUpdateStatusFromPod(t *testing.T) {
}},
},
Steps: []v1alpha1.StepState{{
corev1.ContainerState{
ContainerState: corev1.ContainerState{
Waiting: &corev1.ContainerStateWaiting{
Message: "i'm pending",
},
},
Name: "status-name",
}},
},
}, {
Expand Down
4 changes: 4 additions & 0 deletions test/taskrun_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,27 +78,31 @@ func TestTaskRunFailure(t *testing.T) {
Reason: "Error",
},
},
Name: "exit",
}, {
ContainerState: corev1.ContainerState{
Terminated: &corev1.ContainerStateTerminated{
ExitCode: 0,
Reason: "Completed",
},
},
Name: "hello",
}, {
ContainerState: corev1.ContainerState{
Terminated: &corev1.ContainerStateTerminated{
ExitCode: 0,
Reason: "Completed",
},
},
Name: "world",
}, {
ContainerState: corev1.ContainerState{
Terminated: &corev1.ContainerStateTerminated{
ExitCode: 0,
Reason: "Completed",
},
},
Name: "nop",
}}
ignoreFields := cmpopts.IgnoreFields(corev1.ContainerStateTerminated{}, "StartedAt", "FinishedAt", "ContainerID")
if d := cmp.Diff(taskrun.Status.Steps, expectedStepState, ignoreFields); d != "" {
Expand Down