Added entrypoint log grabber to taskrun controller #167

aaron-prindle · 2018-10-18T18:02:49Z

What is the problem being solved?
This PR addresses issue #143. This issue is that it when Build's complete, logs from the steps the Build ran are garbage collected by kubernetes and no longer available to the user.

Why is this the best approach?

No special kubernetes configuration (eg. changing garbage collector values)

What other approaches did you consider?

Changing kubernetes garbage collection for these containers so that they are not immediately deleted and log capture was possible

What side effects will this approach have?

With this approach, users will have to specify the "Command" value for the containers they which to run as the Entrypoint is not retrievable. This means that containers/flows setup to use only the Entrypoint will no longer be supported.

What future work remains to be done?

It is possible to have the PVCs changed to EmptyDir volumes once a log uploader is created. This will help with the issue that currently PVCs are being created and not cleaned up.

Co-authored-by: Christie Wilson [email protected]

knative-prow-robot · 2018-10-18T18:02:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aaron-prindle

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~~ [aaron-prindle]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tejal29

Can you add some more notes on what happens to the claimed space once the task is done?

tejal29 · 2018-10-18T18:40:11Z

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

+func GetCopyStep() corev1.Container {
+	return corev1.Container{
+		Name:         "place-tools",
+		Image:        "gcr.io/k8s-prow/entrypoint",


Lets pin this image to sha.

tejal29 · 2018-10-18T18:45:53Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+		},
+	)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create PVC %q: %s", tr.Name, err)


Can we explain it more like "failed to claim Persistent Volume due to error "

bobcatfish · 2018-10-18T20:28:08Z

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

+const (
+	// MountName is the name of the pvc being mounted (which
+	// will contain the entrypoint binary and eventually the logs)
+	MountName = "tools"


@aaron-prindle and I talked about some follow-up work to randomize the name of the mount and the volume so that they won't conflict with any of the user's config - I'll create a follow up ticket!

bobcatfish · 2018-10-18T20:28:37Z

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

+}
+
+// TODO: add more test cases after all, e.g. with existing env
+// var and volume mounts


you can remove this comment for now i think @aaron-prindle , we can cover this in the follow up task ill make

ping you can remove the TODO

lol okay we can remove these TODOs later, i dont want to block this any longer

bobcatfish · 2018-10-18T20:28:52Z

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

+// TODO: This will not work when a step uses an image that has its
+// own entrypoint, i.e. `Command` is a required field. In later iterations
+// we can update the controller to inspect the image's `Entrypoint`
+// and use that if required.


we'll need a follow up ticket for this too - could you write that one up @aaron-prindle ?

plz remove todo (or update with issue #)

bobcatfish · 2018-10-18T20:29:22Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

-		return nil, fmt.Errorf("Task has nil BuildSpec")
+	// TODO: additional validation in Task webhook:
+	// No mounts with same names / path
+	// No env var with the name we'll be adding


you can remove these comments too, i think we can handle this with randomization instead

bobcatfish · 2018-10-18T20:29:49Z

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

+			}
+			// 3. check that volume was created
+			pvc := kubeclient.Actions()[0].(ktesting.CreateAction).GetObject().(*corev1.PersistentVolumeClaim)
+			// TODO(aaron-prindle) make these tests verify exact pvc spec, not just name


you should probably add this before we merge @aaron-prindle

tanner-bruce · 2018-10-19T03:20:19Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+					ClaimName: volumeName,
+				},
+			},
+		},


is bSpec.Volumes guaranteed to be empty at this point or should this new volume be appended instead?

Great point @tanner-bruce , we should definitely fix this before merging @aaron-prindle

nice catch, done!

bobcatfish · 2018-10-22T22:00:49Z

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

+			// panic: cannot handle unexported field: {v1.ResourceRequirements}.Requests["storage"].i
+			// consider using AllowUnexported or cmpopts.IgnoreUnexported [recovered]
+
+			// if d := cmp.Diff(pvc.Spec.Resources, expectedVolume.Spec.Resources); d != "" {


you could assert on the individual fields in Resources - cmp.Diff is probably trying to assert on every single field in the object, including fields which aren't exposed.

you just need to look at the values we set in:

bobcatfish

@dlorenc found some big issues we need to address before merging, I added a couple comments but he can update if he finds more

Thanks for being our guinea pig @dlorenc 🙏 ❤️ 😰

bobcatfish · 2018-10-23T13:46:58Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

-func (c *Reconciler) createBuild(tr *v1alpha1.TaskRun) (*buildv1alpha1.Build, error) {
+// GetExpectedVolume returns the volume template used by createVolume
+// this is exported for testing
+func GetExpectedVolume(tr *v1alpha1.TaskRun) *corev1.PersistentVolumeClaim {


We shouldn't use this in the tests - if we are using exactly the same code to test the code we are using, there is no reason to test it.

In the test we should only assert on the fields we care about. I'd rather have less coverage in the test than do this

can we plz rename this and not use it in the tests? assert only on fields we care about? i.e. access mode + resource size? or not assert on those at all?

bobcatfish · 2018-10-23T13:53:10Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

@@ -209,6 +250,29 @@ func (c *Reconciler) createBuild(tr *v1alpha1.TaskRun) (*buildv1alpha1.Build, er
 		return nil, fmt.Errorf("Task %s has nil BuildSpec", t.Name)
 	}

+	bSpec := t.Spec.BuildSpec
+	bSpec.Volumes = append(bSpec.Volumes,


@dlorenc tried this out and if there is an error creating the build, we end up appending this multiple times and trying to create the same PVC multiple times - and wrapping the entry point multiple times too!! 🤣

We need to:

not create the PVC if it already exists, maybe we can try to reuse it

not append this volume multiple times, - copy the buildspec before modifying it - we shouldn't mutate the task at all!!

We should add a test case somewhere to cover this, we can probably use the reconciler tests and avoid making this a full fledged system test

dlorenc · 2018-10-23T14:16:44Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

@@ -209,6 +250,29 @@ func (c *Reconciler) createBuild(tr *v1alpha1.TaskRun) (*buildv1alpha1.Build, er
 		return nil, fmt.Errorf("Task %s has nil BuildSpec", t.Name)
 	}

+	bSpec := t.Spec.BuildSpec


I think this needs to be DeepCopy()

bobcatfish · 2018-10-23T20:49:10Z

out.txt

+ok  	github.com/knative/build-pipeline/pkg/reconciler/v1alpha1/taskrun/resources	(cached)
+?   	github.com/knative/build-pipeline/pkg/system	[no test files]
+ok  	github.com/knative/build-pipeline/test	(cached)
+?   	github.com/knative/build-pipeline/test/gohelloworld	[no test files]


i think this was committed by accident

bobcatfish · 2018-10-23T20:53:40Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

@@ -209,6 +265,28 @@ func (c *Reconciler) createBuild(tr *v1alpha1.TaskRun) (*buildv1alpha1.Build, er
 		return nil, fmt.Errorf("Task %s has nil BuildSpec", t.Name)
 	}

+	bSpec := t.Spec.BuildSpec.DeepCopy()
+	bSpec.Volumes = appendIfMissing(bSpec.Volumes, corev1.Volume{


since we're copying the buildspec before modifying it, we should be able to append the volume every time? it should never be present

removed appendIfMissing( fxn and logic -> append( now

tejal29

lgtm except for 2 nits.

tejal29 · 2018-10-23T21:59:02Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

@@ -154,8 +151,20 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error
 	// get build the same as the taskrun, this is the value we use for 1:1 mapping and retrieval
 	build, err := c.BuildClientSet.BuildV1alpha1().Builds(tr.Namespace).Get(tr.Name, metav1.GetOptions{})
 	if errors.IsNotFound(err) {
+		pvc, err := c.KubeClientSet.CoreV1().PersistentVolumeClaims(tr.Namespace).Get(tr.Name, metav1.GetOptions{})


Can we move L154 to createPVC and name is getOrCreatePVC.

This block is getting longer.

I can do this another PR

tejal29 · 2018-10-23T22:04:11Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+
+	// Override the entrypoint so that we can use our custom
+	// entrypoint which copies logs
+	err = resources.AddEntrypoint(bSpec.Steps)


Can we change the Concepts.doc to mention we no longer depend on a BuilderImage ?
You can also add a note for it and then we can decide what this image should be.

I'm not sure I understand, I can update the doc to explain the entrypoint log change but we still depend on a BuilderImage if my understanding is correct

I've update this doc with entrypoint info

I'm guessing @tejal29 is talking about the fact that the builder image contract actually says to do the opposite of what we want - it says we want ppl to define an entrypoint when in fact we definitely do not want that

I created some docs but I'm having a hard time adding them to this PR

I'm failing at this, I'll just make a new PR with the doc changes

What is the problem being solved? This PR addresses issue #143. This issue is that it when Build's complete, logs from the steps the Build ran are garbage collected by kubernetes and no longer available to the user. Why is this the best approach? 1) No special kubernetes configuration (eg. changing garbage collector values) What other approaches did you consider? 1) Changing kubernetes garbage collection for these containers so that they are not immediately deleted and log capture was possible What side effects will this approach have? 1) With this approach, users will have to specify the "Command" value for the containers they which to run as the Entrypoint is not retrievable. This means that containers/flows setup to use only the Entrypoint will no longer be supported. What future work remains to be done? 1) It is possible to have the PVCs changed to EmptyDir volumes once a log uploader is created. This will help with the issue that currently PVCs are being created and not cleaned up. Co-authored-by: Christie Wilson <[email protected]>

nader-ziada · 2018-10-24T14:48:01Z

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

+	entrypointJSONConfigEnvVar = "ENTRYPOINT_OPTIONS"
+	EntrypointImage            = "gcr.io/k8s-prow/entrypoint@sha256:7c7cd8906ce4982ffee326218e9fc75da2d4896d53cabc9833b9cc8d2d6b2b8f"
+)
+


we probably should not be using an image from Prow for our running pipeline system, maybe have out own?

we could, esp. if we feel we can't rely on this image.

but in the meantime, i think it's easier if they manage it's lifecycle? one less thing we have to worry about! but once we start having a release process i guess we should probably release this image too 🤔

I think we'll want our own anyway, to add features like being able to specify the entrypoint. BUilding the image ourself also makes it easy to build into the config YAMLs with ko, and makes it easy to make sure it's cached on every node

for now its fine, but once we have a release like you said, we should be using our own to make sure it does't change on us

The Builder image contract is actually the opposite of what we now support - it specifies that users should define an `entrypoint` and rely on that, however the new behaviour in #167 will completely ignore this `entrypoint`. These docs are a copy of the Builder image docs at https://github.com/knative/docs/blob/master/build/builder-contract.md but modified to describe the contract required by Task.

bobcatfish · 2018-10-24T15:48:04Z

/lgtm

The Builder image contract is actually the opposite of what we now support - it specifies that users should define an `entrypoint` and rely on that, however the new behaviour in tektoncd#167 will completely ignore this `entrypoint`. These docs are a copy of the Builder image docs at https://github.com/knative/docs/blob/master/build/builder-contract.md but modified to describe the contract required by Task.

The Builder image contract is actually the opposite of what we now support - it specifies that users should define an `entrypoint` and rely on that, however the new behaviour in tektoncd#167 will completely ignore this `entrypoint`. These docs are a copy of the Builder image docs at https://github.com/knative/docs/blob/master/build/builder-contract.md but modified to describe the contract required by Task. This is for tektoncd#143

The Builder image contract is actually the opposite of what we now support - it specifies that users should define an `entrypoint` and rely on that, however the new behaviour in #167 will completely ignore this `entrypoint`. These docs are a copy of the Builder image docs at https://github.com/knative/docs/blob/master/build/builder-contract.md but modified to describe the contract required by Task. This is for #143

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2018

knative-prow-robot requested review from bobcatfish and dlorenc October 18, 2018 18:03

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 18, 2018

tejal29 reviewed Oct 18, 2018

View reviewed changes

bobcatfish reviewed Oct 18, 2018

View reviewed changes

tanner-bruce reviewed Oct 19, 2018

View reviewed changes

bobcatfish reviewed Oct 22, 2018

View reviewed changes

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 22, 2018

bobcatfish reviewed Oct 23, 2018

View reviewed changes

dlorenc reviewed Oct 23, 2018

View reviewed changes

bobcatfish reviewed Oct 23, 2018

View reviewed changes

tejal29 reviewed Oct 23, 2018

View reviewed changes

tanner-bruce mentioned this pull request Oct 24, 2018

Load entrypoint from remote #183

Merged

nader-ziada reviewed Oct 24, 2018

View reviewed changes

knative-prow-robot assigned bobcatfish Oct 24, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2018

knative-prow-robot merged commit 6f6b071 into tektoncd:master Oct 24, 2018

bobcatfish mentioned this pull request Oct 24, 2018

Update docs about Builder image #184

Merged

tejal29 mentioned this pull request Oct 24, 2018

Implement reference Unstructured store API to upload TaskRun logs GCS store #107

Closed

bobcatfish mentioned this pull request Oct 25, 2018

Create solution for making logs available when running Tasks #143

Closed

Added entrypoint log grabber to taskrun controller #167

Added entrypoint log grabber to taskrun controller #167

Conversation

aaron-prindle commented Oct 18, 2018

knative-prow-robot commented Oct 18, 2018

tejal29 left a comment

Choose a reason for hiding this comment

tejal29 Oct 18, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

tejal29 Oct 18, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish Oct 18, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-prindle Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish Oct 18, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlorenc Oct 23, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

bobcatfish Oct 23, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish Oct 23, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejal29 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish commented Oct 24, 2018

tejal29 Oct 18, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 18, 2018 •

edited by aaron-prindle

Loading

bobcatfish Oct 18, 2018 •

edited by aaron-prindle

Loading

aaron-prindle Oct 22, 2018 •

edited

Loading

bobcatfish Oct 18, 2018 •

edited by aaron-prindle

Loading

dlorenc Oct 23, 2018 •

edited by aaron-prindle

Loading

bobcatfish Oct 23, 2018 •

edited by aaron-prindle

Loading

bobcatfish Oct 23, 2018 •

edited by aaron-prindle

Loading