Skip to content

Commit

Permalink
Wrap nop final step with entrypoint
Browse files Browse the repository at this point in the history 
`nop` image/step used to be executed *only* when all the previous step
were done sucessfully. As we are not using init containers anymore,
this is not true, `nop` will always be executed even in case of
previous failure (after the others though). This happens because we
don't use the `entrypoint` wrapper for this final step.

This fixes it by wrapping `nop` with entrypoint — just like other
steps. That way, it will only print `build successful` in case of
actual successful run 💃

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
  • Loading branch information
@vdemeester
vdemeester committed Apr 10, 2019
1 parent 395bc32 commit 1ea0766
Show file tree
Hide file tree
Showing 5 changed files with 82 additions and 26 deletions.
33 changes: 22 additions & 11 deletions pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,21 +109,32 @@ func AddCopyStep(spec *v1alpha1.TaskSpec) {
func RedirectSteps(cache *Cache, steps []corev1.Container, kubeclient kubernetes.Interface, taskRun *v1alpha1.TaskRun, logger *zap.SugaredLogger) error {
for i := range steps {
step := &steps[i]
if len(step.Command) == 0 {
logger.Infof("Getting Cmd from remote entrypoint for step: %s", step.Name)
var err error
step.Command, err = GetRemoteEntrypoint(cache, step.Image, kubeclient, taskRun)
if err != nil {
logger.Errorf("Error getting entry point image", err.Error())
return err
}
if err := RedirectStep(cache, i, step, kubeclient, taskRun, logger); err != nil {
return err
}
}

return nil
}

step.Args = GetArgs(i, step.Command, step.Args)
step.Command = []string{BinaryLocation}
step.VolumeMounts = append(step.VolumeMounts, toolsMount)
// RedirectStep will modify a step/container such that
// the binary being run is no longer the one specified by the Command
// and the Args, but is instead the entrypoint binary, which will
// itself invoke the Command and Args, but also capture logs.
func RedirectStep(cache *Cache, stepNum int, step *corev1.Container, kubeclient kubernetes.Interface, taskRun *v1alpha1.TaskRun, logger *zap.SugaredLogger) error {
if len(step.Command) == 0 {
logger.Infof("Getting Cmd from remote entrypoint for step: %s", step.Name)
var err error
step.Command, err = GetRemoteEntrypoint(cache, step.Image, kubeclient, taskRun)
if err != nil {
logger.Errorf("Error getting entry point image", err.Error())
return err
}
}

step.Args = GetArgs(stepNum, step.Command, step.Args)
step.Command = []string{BinaryLocation}
step.VolumeMounts = append(step.VolumeMounts, toolsMount)
return nil
}

Expand Down
7 changes: 5 additions & 2 deletions pkg/reconciler/v1alpha1/taskrun/resources/pod.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"io/ioutil"
"path/filepath"

"go.uber.org/zap"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
Expand Down Expand Up @@ -157,7 +158,7 @@ func makeCredentialInitializer(serviceAccountName, namespace string, kubeclient

// MakePod converts TaskRun and TaskSpec objects to a Pod which implements the taskrun specified
// by the supplied CRD.
func MakePod(taskRun *v1alpha1.TaskRun, taskSpec v1alpha1.TaskSpec, kubeclient kubernetes.Interface) (*corev1.Pod, error) {
func MakePod(taskRun *v1alpha1.TaskRun, taskSpec v1alpha1.TaskSpec, kubeclient kubernetes.Interface, cache *entrypoint.Cache, logger *zap.SugaredLogger) (*corev1.Pod, error) {
// Copy annotations on the build through to the underlying pod to allow users
// to specify pod annotations.
annotations := map[string]string{}
Expand Down Expand Up @@ -220,7 +221,9 @@ func MakePod(taskRun *v1alpha1.TaskRun, taskSpec v1alpha1.TaskSpec, kubeclient k
}
gibberish := hex.EncodeToString(b)

podContainers = append(podContainers, corev1.Container{Name: "nop", Image: *nopImage, Command: []string{"/ko-app/nop"}})
nopContainer := &corev1.Container{Name: "nop", Image: *nopImage, Command: []string{"/ko-app/nop"}}
entrypoint.RedirectStep(cache, len(podContainers), nopContainer, kubeclient, taskRun, logger)
podContainers = append(podContainers, *nopContainer)

return &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Expand Down
11 changes: 9 additions & 2 deletions pkg/reconciler/v1alpha1/taskrun/resources/pod_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
fakek8s "k8s.io/client-go/kubernetes/fake"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/reconciler/v1alpha1/taskrun/entrypoint"
"github.com/tektoncd/pipeline/test/names"
)

Expand All @@ -37,7 +38,12 @@ var (
nopContainer = corev1.Container{
Name: "nop",
Image: *nopImage,
Command: []string{"/ko-app/nop"},
Command: []string{"/builder/tools/entrypoint"},
Args: []string{"-wait_file", "/builder/tools/0", "-post_file", "/builder/tools/1", "-entrypoint", "/ko-app/nop", "--"},
VolumeMounts: []corev1.VolumeMount{{
Name: entrypoint.MountName,
MountPath: entrypoint.MountPoint,
}},
}
)

Expand Down Expand Up @@ -246,7 +252,8 @@ func TestMakePod(t *testing.T) {
},
Spec: c.trs,
}
got, err := MakePod(tr, c.ts, cs)
cache, _ := entrypoint.NewCache()
got, err := MakePod(tr, c.ts, cs, cache, logger)
if err != c.wantErr {
t.Fatalf("MakePod: %v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/v1alpha1/taskrun/taskrun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ func (c *Reconciler) createPod(tr *v1alpha1.TaskRun, ts *v1alpha1.TaskSpec, task
return nil, fmt.Errorf("couldnt apply output resource templating: %s", err)
}

pod, err := resources.MakePod(tr, *ts, c.KubeClientSet)
pod, err := resources.MakePod(tr, *ts, c.KubeClientSet, c.cache, c.Logger)
if err != nil {
return nil, fmt.Errorf("translating Build to Pod: %v", err)
}
Expand Down
55 changes: 45 additions & 10 deletions pkg/reconciler/v1alpha1/taskrun/taskrun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/knative/pkg/configmap"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/logging"
"github.com/tektoncd/pipeline/pkg/reconciler"
"github.com/tektoncd/pipeline/pkg/reconciler/v1alpha1/taskrun/entrypoint"
"github.com/tektoncd/pipeline/pkg/reconciler/v1alpha1/taskrun/resources"
Expand Down Expand Up @@ -312,7 +313,11 @@ func TestReconcile(t *testing.T) {
tb.VolumeMount("workspace", workspaceDir),
tb.VolumeMount("home", "/builder/home"),
),
tb.PodContainer("nop", "override-with-nop:latest", tb.Command("/ko-app/nop")),
tb.PodContainer("nop", "override-with-nop:latest",
tb.Command("/builder/tools/entrypoint"),
tb.Args("-wait_file", "/builder/tools/0", "-post_file", "/builder/tools/1", "-entrypoint", "/ko-app/nop", "--"),
tb.VolumeMount(entrypoint.MountName, entrypoint.MountPoint),
),
),
),
}, {
Expand All @@ -339,7 +344,11 @@ func TestReconcile(t *testing.T) {
tb.VolumeMount("workspace", workspaceDir),
tb.VolumeMount("home", "/builder/home"),
),
tb.PodContainer("nop", "override-with-nop:latest", tb.Command("/ko-app/nop")),
tb.PodContainer("nop", "override-with-nop:latest",
tb.Command("/builder/tools/entrypoint"),
tb.Args("-wait_file", "/builder/tools/0", "-post_file", "/builder/tools/1", "-entrypoint", "/ko-app/nop", "--"),
tb.VolumeMount(entrypoint.MountName, entrypoint.MountPoint),
),
),
),
}, {
Expand Down Expand Up @@ -393,7 +402,11 @@ func TestReconcile(t *testing.T) {
tb.VolumeMount("workspace", workspaceDir),
tb.VolumeMount("home", "/builder/home"),
),
tb.PodContainer("nop", "override-with-nop:latest", tb.Command("/ko-app/nop")),
tb.PodContainer("nop", "override-with-nop:latest",
tb.Command("/builder/tools/entrypoint"),
tb.Args("-wait_file", "/builder/tools/2", "-post_file", "/builder/tools/3", "-entrypoint", "/ko-app/nop", "--"),
tb.VolumeMount(entrypoint.MountName, entrypoint.MountPoint),
),
),
),
}, {
Expand Down Expand Up @@ -491,7 +504,11 @@ func TestReconcile(t *testing.T) {
tb.VolumeMount("workspace", workspaceDir),
tb.VolumeMount("home", "/builder/home"),
),
tb.PodContainer("nop", "override-with-nop:latest", tb.Command("/ko-app/nop")),
tb.PodContainer("nop", "override-with-nop:latest",
tb.Command("/builder/tools/entrypoint"),
tb.Args("-wait_file", "/builder/tools/6", "-post_file", "/builder/tools/7", "-entrypoint", "/ko-app/nop", "--"),
tb.VolumeMount(entrypoint.MountName, entrypoint.MountPoint),
),
),
),
}, {
Expand All @@ -506,6 +523,7 @@ func TestReconcile(t *testing.T) {
tb.PodVolumes(toolsVolume, workspaceVolume, homeVolume),
tb.PodRestartPolicy(corev1.RestartPolicyNever),
getCredentialsInitContainer("mz4c7"),
placeToolsInitContainer,
tb.PodContainer("build-step-git-source-git-resource-9l9zj", "override-with-git:latest",
tb.Command(entrypointLocation),
tb.Args("-wait_file", "", "-post_file", "/builder/tools/0", "-entrypoint", "/ko-app/git-init", "--",
Expand All @@ -516,7 +534,6 @@ func TestReconcile(t *testing.T) {
tb.VolumeMount("workspace", workspaceDir),
tb.VolumeMount("home", "/builder/home"),
),
placeToolsInitContainer,
tb.PodContainer("build-step-mycontainer", "myimage",
tb.Command(entrypointLocation),
tb.WorkingDir(workspaceDir),
Expand All @@ -527,7 +544,11 @@ func TestReconcile(t *testing.T) {
tb.VolumeMount("workspace", workspaceDir),
tb.VolumeMount("home", "/builder/home"),
),
tb.PodContainer("nop", "override-with-nop:latest", tb.Command("/ko-app/nop")),
tb.PodContainer("nop", "override-with-nop:latest",
tb.Command("/builder/tools/entrypoint"),
tb.Args("-wait_file", "/builder/tools/1", "-post_file", "/builder/tools/2", "-entrypoint", "/ko-app/nop", "--"),
tb.VolumeMount(entrypoint.MountName, entrypoint.MountPoint),
),
),
),
}, {
Expand All @@ -553,7 +574,11 @@ func TestReconcile(t *testing.T) {
tb.VolumeMount("workspace", workspaceDir),
tb.VolumeMount("home", "/builder/home"),
),
tb.PodContainer("nop", "override-with-nop:latest", tb.Command("/ko-app/nop")),
tb.PodContainer("nop", "override-with-nop:latest",
tb.Command("/builder/tools/entrypoint"),
tb.Args("-wait_file", "/builder/tools/0", "-post_file", "/builder/tools/1", "-entrypoint", "/ko-app/nop", "--"),
tb.VolumeMount(entrypoint.MountName, entrypoint.MountPoint),
),
),
),
}, {
Expand Down Expand Up @@ -589,7 +614,11 @@ func TestReconcile(t *testing.T) {
tb.VolumeMount("workspace", workspaceDir),
tb.VolumeMount("home", "/builder/home"),
),
tb.PodContainer("nop", "override-with-nop:latest", tb.Command("/ko-app/nop")),
tb.PodContainer("nop", "override-with-nop:latest",
tb.Command("/builder/tools/entrypoint"),
tb.Args("-wait_file", "/builder/tools/1", "-post_file", "/builder/tools/2", "-entrypoint", "/ko-app/nop", "--"),
tb.VolumeMount(entrypoint.MountName, entrypoint.MountPoint),
),
),
),
}, {
Expand All @@ -616,7 +645,11 @@ func TestReconcile(t *testing.T) {
tb.VolumeMount("workspace", workspaceDir),
tb.VolumeMount("home", "/builder/home"),
),
tb.PodContainer("nop", "override-with-nop:latest", tb.Command("/ko-app/nop")),
tb.PodContainer("nop", "override-with-nop:latest",
tb.Command("/builder/tools/entrypoint"),
tb.Args("-wait_file", "/builder/tools/0", "-post_file", "/builder/tools/1", "-entrypoint", "/ko-app/nop", "--"),
tb.VolumeMount(entrypoint.MountName, entrypoint.MountPoint),
),
),
),
}} {
Expand Down Expand Up @@ -774,6 +807,8 @@ func TestReconcilePodFetchError(t *testing.T) {
func TestReconcilePodUpdateStatus(t *testing.T) {
taskRun := tb.TaskRun("test-taskrun-run-success", "foo", tb.TaskRunSpec(tb.TaskRunTaskRef("test-task")))

logger, _ := logging.NewLogger("", "")
cache, _ := entrypoint.NewCache()
// TODO(jasonhall): This avoids a circular dependency where
// getTaskRunController takes a test.Data which must be populated with
// a pod created from MakePod which requires a (fake) Kube client. When
Expand All @@ -786,7 +821,7 @@ func TestReconcilePodUpdateStatus(t *testing.T) {
Name: "default",
Namespace: taskRun.Namespace,
},
}))
}), cache, logger)
if err != nil {
t.Fatalf("MakePod: %v", err)
}
Expand Down

0 comments on commit 1ea0766

Please sign in to comment.