-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
Added entrypoint log grabber to taskrun controller #167
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
/* | ||
Copyright 2018 The Knative Authors | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package resources | ||
|
||
import ( | ||
"encoding/json" | ||
"fmt" | ||
|
||
corev1 "k8s.io/api/core/v1" | ||
) | ||
|
||
const ( | ||
// MountName is the name of the pvc being mounted (which | ||
// will contain the entrypoint binary and eventually the logs) | ||
MountName = "tools" | ||
|
||
mountPoint = "/tools" | ||
entrypointBin = mountPoint + "/entrypoint" | ||
entrypointJSONConfigEnvVar = "ENTRYPOINT_OPTIONS" | ||
EntrypointImage = "gcr.io/k8s-prow/entrypoint@sha256:7c7cd8906ce4982ffee326218e9fc75da2d4896d53cabc9833b9cc8d2d6b2b8f" | ||
) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we probably should not be using an image from Prow for our running pipeline system, maybe have out own? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
var toolsMount = corev1.VolumeMount{ | ||
Name: MountName, | ||
MountPath: mountPoint, | ||
} | ||
|
||
// GetCopyStep will return a Build Step (Container) that will | ||
// copy the entrypoint binary from the entrypoint image into the | ||
// volume mounted at mountPoint, so that it can be mounted by | ||
// subsequent steps and used to capture logs. | ||
func GetCopyStep() corev1.Container { | ||
return corev1.Container{ | ||
Name: "place-tools", | ||
Image: EntrypointImage, | ||
Command: []string{"/bin/cp"}, | ||
Args: []string{"/entrypoint", entrypointBin}, | ||
VolumeMounts: []corev1.VolumeMount{toolsMount}, | ||
} | ||
} | ||
|
||
type entrypointArgs struct { | ||
Args []string `json:"args"` | ||
ProcessLog string `json:"process_log"` | ||
MarkerFile string `json:"marker_file"` | ||
} | ||
|
||
func getEnvVar(cmd, args []string) (string, error) { | ||
entrypointArgs := entrypointArgs{ | ||
Args: append(cmd, args...), | ||
ProcessLog: "/tools/process-log.txt", | ||
MarkerFile: "/tools/marker-file.txt", | ||
} | ||
j, err := json.Marshal(entrypointArgs) | ||
if err != nil { | ||
return "", fmt.Errorf("couldn't marshal arguments %q for entrypoint env var: %s", entrypointArgs, err) | ||
} | ||
return string(j), nil | ||
} | ||
|
||
// TODO: add more test cases after all, e.g. with existing env | ||
// var and volume mounts | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ping you can remove the TODO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lol okay we can remove these TODOs later, i dont want to block this any longer |
||
|
||
// AddEntrypoint will modify each of the steps/containers 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. | ||
// 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we'll need a follow up ticket for this too - could you write that one up @aaron-prindle ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done: #175 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. plz remove todo (or update with issue #) |
||
func AddEntrypoint(steps []corev1.Container) error { | ||
for i := range steps { | ||
step := &steps[i] | ||
e, err := getEnvVar(step.Command, step.Args) | ||
if err != nil { | ||
return fmt.Errorf("couldn't get env var for entrypoint: %s", err) | ||
} | ||
step.Command = []string{entrypointBin} | ||
step.Args = []string{} | ||
|
||
step.Env = append(step.Env, corev1.EnvVar{ | ||
Name: entrypointJSONConfigEnvVar, | ||
Value: e, | ||
}) | ||
step.VolumeMounts = append(step.VolumeMounts, toolsMount) | ||
} | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,7 @@ import ( | |
corev1 "k8s.io/api/core/v1" | ||
"k8s.io/apimachinery/pkg/api/equality" | ||
"k8s.io/apimachinery/pkg/api/errors" | ||
"k8s.io/apimachinery/pkg/api/resource" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/runtime/schema" | ||
"k8s.io/client-go/tools/cache" | ||
|
@@ -55,6 +56,8 @@ const ( | |
// taskRunControllerName defines name for TaskRun Controller | ||
taskRunControllerName = "TaskRun" | ||
taskRunNameLabelKey = "taskrun.knative.dev/taskName" | ||
|
||
pvcSizeBytes = 5 * 1024 * 1024 * 1024 // 5 GBs | ||
) | ||
|
||
var ( | ||
|
@@ -102,13 +105,6 @@ func NewController( | |
UpdateFunc: controller.PassNew(impl.Enqueue), | ||
}) | ||
|
||
// TODO(aaron-prindle) what to do if a task is deleted? | ||
// taskInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ | ||
// AddFunc: impl.Enqueue, | ||
// UpdateFunc: controller.PassNew(impl.Enqueue), | ||
// DeleteFunc: impl.Enqueue, | ||
// }) | ||
|
||
c.tracker = tracker.New(impl.EnqueueKey, opt.GetTrackerLease()) | ||
buildInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ | ||
AddFunc: c.tracker.OnChanged, | ||
|
@@ -166,8 +162,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{}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we move L154 to This block is getting longer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can do this another PR |
||
if errors.IsNotFound(err) { | ||
// Create a persistent volume claim to hold Build logs | ||
pvc, err = c.createPVC(tr) | ||
if err != nil { | ||
return fmt.Errorf("Failed to create persistent volume claim %s for task %q: %v", tr.Name, err, tr.Name) | ||
} | ||
} else if err != nil { | ||
c.Logger.Errorf("Failed to reconcile taskrun: %q, failed to get pvc %q; %v", tr.Name, tr.Name, err) | ||
return err | ||
} | ||
|
||
// Build is not present, create build | ||
build, err = c.createBuild(tr) | ||
build, err = c.createBuild(tr, pvc.Name) | ||
if err != nil { | ||
// This Run has failed, so we need to mark it as failed and stop reconciling it | ||
tr.Status.SetCondition(&duckv1alpha1.Condition{ | ||
|
@@ -224,8 +232,40 @@ func (c *Reconciler) updateStatus(taskrun *v1alpha1.TaskRun) (*v1alpha1.TaskRun, | |
return newtaskrun, nil | ||
} | ||
|
||
// createBuild creates a build from the task, using the task's buildspec. | ||
func (c *Reconciler) createBuild(tr *v1alpha1.TaskRun) (*buildv1alpha1.Build, error) { | ||
// createVolume will create a persistent volume mount for tr which | ||
// will be used to gather logs using the entrypoint wrapper | ||
func (c *Reconciler) createPVC(tr *v1alpha1.TaskRun) (*corev1.PersistentVolumeClaim, error) { | ||
v, err := c.KubeClientSet.CoreV1().PersistentVolumeClaims(tr.Namespace).Create( | ||
&corev1.PersistentVolumeClaim{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Namespace: tr.Namespace, | ||
// This pvc is specific to this TaskRun, so we'll use the same name | ||
Name: tr.Name, | ||
OwnerReferences: []metav1.OwnerReference{ | ||
*metav1.NewControllerRef(tr, groupVersionKind), | ||
}, | ||
}, | ||
Spec: corev1.PersistentVolumeClaimSpec{ | ||
AccessModes: []corev1.PersistentVolumeAccessMode{ | ||
corev1.ReadWriteOnce, | ||
}, | ||
Resources: corev1.ResourceRequirements{ | ||
Requests: map[corev1.ResourceName]resource.Quantity{ | ||
corev1.ResourceStorage: *resource.NewQuantity(pvcSizeBytes, resource.BinarySI), | ||
}, | ||
}, | ||
}, | ||
}, | ||
) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to claim Persistent Volume %q due to error: %s", tr.Name, err) | ||
} | ||
return v, nil | ||
} | ||
|
||
// createBuild creates a build from the task, using the task's buildspec | ||
// with pvcName as a volumeMount | ||
func (c *Reconciler) createBuild(tr *v1alpha1.TaskRun, pvcName string) (*buildv1alpha1.Build, error) { | ||
// Get related task for taskrun | ||
t, err := c.taskLister.Tasks(tr.Namespace).Get(tr.Spec.TaskRef.Name) | ||
if err != nil { | ||
|
@@ -237,6 +277,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 = append(bSpec.Volumes, corev1.Volume{ | ||
Name: resources.MountName, | ||
VolumeSource: corev1.VolumeSource{ | ||
PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ | ||
ClaimName: pvcName, | ||
}, | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great point @tanner-bruce , we should definitely fix this before merging @aaron-prindle There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice catch, done! |
||
}) | ||
|
||
// Override the entrypoint so that we can use our custom | ||
// entrypoint which copies logs | ||
err = resources.AddEntrypoint(bSpec.Steps) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we change the Concepts.doc to mention we no longer depend on a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've update this doc with entrypoint info There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created some docs but I'm having a hard time adding them to this PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm failing at this, I'll just make a new PR with the doc changes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
if err != nil { | ||
return nil, fmt.Errorf("Failed to add entrypoint to steps of Build: %s", err) | ||
} | ||
|
||
// Add the step which will copy the entrypoint into the volume | ||
// we are going to be using, so that all of the steps will have | ||
// access to it. | ||
bSpec.Steps = append([]corev1.Container{resources.GetCopyStep()}, bSpec.Steps...) | ||
|
||
b := &buildv1alpha1.Build{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: tr.Name, | ||
|
@@ -247,7 +309,7 @@ func (c *Reconciler) createBuild(tr *v1alpha1.TaskRun) (*buildv1alpha1.Build, er | |
// Attach new label and pass taskrun labels to build | ||
Labels: makeLabels(tr), | ||
}, | ||
Spec: *t.Spec.BuildSpec, | ||
Spec: *bSpec, | ||
} | ||
// Pass service account name from taskrun to build | ||
// if task specifies service account name override with taskrun SA | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!