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.

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

Check labels in e2e PipelineRun tests and document label propagation #500

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 20 additions & 0 deletions docs/pipelineruns.md
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,26 @@ of the `TaskRun` resource object.
For examples and more information about specifying service accounts, see the
[`ServiceAccount`](./auth.md) reference topic.

## Labels
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seemed like the least bad place for the docs, but I don't know if it really fits. Any suggestions are welcome!

Copy link
Collaborator

Choose a reason for hiding this comment

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

i had trouble thinking of a good place myself! the only other alternative i can think of is to create a new doc like troubleshooting.md or faq.md or tips.md or something but none of those options seem great either 🤔 this seems fine for now tho if you don't like any of those ideas!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it seems kind of weird to create a new file just for this information. My preference would be to leave it for now, and then if we end up with other troubleshooting-esque docs we can refactor them into a single place later. Does that seem ok?


Any labels specified in the metadata field of a `PipelineRun` will be
propagated to the `TaskRuns` created automatically for each `Task` in the
`Pipeline` and then to the `Pods` created for those `TaskRuns`. In addition,
the following labels will be added automatically:

* `pipeline.knative.dev/pipeline` will contain the name of the `Pipeline`
* `pipeline.knative.dev/pipelineRun` will contain the name of the `PipelineRun`

These labels make it easier to find the resources that are associated with a
given pipeline.

For example, to find all `Pods` created by a `Pipeline` named test-pipeline,
you could use the following command:

```shell
kubectl get pods --all-namespaces -l pipeline.knative.dev/pipeline=test-pipeline
Copy link
Collaborator

Choose a reason for hiding this comment

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

niiiice

```

## Cancelling a PipelineRun

In order to cancel a running pipeline (`PipelineRun`), you need to update its
Expand Down
24 changes: 24 additions & 0 deletions docs/taskruns.md
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,30 @@ of the `TaskRun` resource object.
For examples and more information about specifying service accounts, see the
[`ServiceAccount`](./auth.md) reference topic.

## Labels

Any labels specified in the metadata field of a `TaskRun` will be
propagated to the `Pod` created to execute the `Task`. In addition,
the following label will be added automatically:

* `pipeline.knative.dev/taskRun` will contain the name of the `TaskRun`

If the `TaskRun` was created automatically by a `PipelineRun`, then the
following two labels will also be added to the `TaskRun` and `Pod`:

* `pipeline.knative.dev/pipeline` will contain the name of the `Pipeline`
* `pipeline.knative.dev/pipelineRun` will contain the name of the `PipelineRun`

These labels make it easier to find the resources that are associated with a
given `TaskRun`.

For example, to find all `Pods` created by a `TaskRun` named test-taskrun,
you could use the following command:

```shell
kubectl get pods --all-namespaces -l pipeline.knative.dev/taskRun=test-taskrun
```

## Cancelling a TaskRun

In order to cancel a running task (`TaskRun`), you need to update its spec to
Expand Down
59 changes: 45 additions & 14 deletions test/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ func TestPipelineRun(t *testing.T) {
td.testSetup(t, c, namespace, i)

prName := fmt.Sprintf("%s%d", pipelineRunName, i)
if _, err := c.PipelineRunClient.Create(td.pipelineRunFunc(i, namespace)); err != nil {
pr, err := c.PipelineRunClient.Create(td.pipelineRunFunc(i, namespace))
if err != nil {
t.Fatalf("Failed to create PipelineRun `%s`: %s", prName, err)
}

Expand All @@ -160,16 +161,22 @@ func TestPipelineRun(t *testing.T) {
if !r.Status.GetCondition(duckv1alpha1.ConditionSucceeded).IsTrue() {
t.Fatalf("Expected TaskRun %s to have succeeded but Status is %v", taskRunName, r.Status)
}
for name, key := range map[string]string{
pipelineRunName: pipeline.PipelineRunLabelKey,
pipelineName: pipeline.PipelineLabelKey,
} {
expectedName := getName(name, i)
lbl := pipeline.GroupName + key
if val := r.Labels[lbl]; val != expectedName {
t.Errorf("Expected label %s=%s but got %s", lbl, expectedName, val)
}

// Check label propagation to Tasks. Presize map with len(pr.ObjectMeta.Labels)
// for custom labels, +2 for the labels added in the PipelineRun controller,
// and +1 for the label added in the TaskRun controller.
labels := make(map[string]string, len(pr.ObjectMeta.Labels)+3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why +3 here? (maybe explain with a comment or a const ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

len(pr.ObjectMeta.Labels) for custom labels, +2 for the static pipeline/pipelineRun label from the PipelineRun controller, and +1 for the taskRun label from the TaskRun controller. I'll add a comment 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment in 37ed488

for key, val := range pr.ObjectMeta.Labels {
labels[key] = val
}
// These labels are added to every TaskRun by the PipelineRun controller
labels[pipeline.GroupName+pipeline.PipelineLabelKey] = getName(pipelineName, i)
labels[pipeline.GroupName+pipeline.PipelineRunLabelKey] = getName(pipelineRunName, i)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the significance of pipeline.GroupName+pipeline.PipelineRunLabelKey and pipeline.GroupName+pipeline.PipelineLabelKey? (i assume these correspond to some label generation logic in the controller - maybe a comment here would help?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah those are static labels added by the controller, but definitely could use a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment two lines up in 37ed488

assertLabelsMatch(t, labels, r.ObjectMeta.Labels)

// Check label propagation to Pods. This label is added to every Pod by the TaskRun controller
labels[pipeline.GroupName + pipeline.TaskRunLabelKey] = taskRunName
assertLabelsMatch(t, labels, getPodForTaskRun(t, c.KubeClient, namespace, r).ObjectMeta.Labels)
}

matchKinds := map[string][]string{"PipelineRun": {prName}, "TaskRun": expectedTaskRunNames}
Expand Down Expand Up @@ -322,10 +329,12 @@ func getPipelineRunSecret(suffix int, namespace string) *corev1.Secret {
}

func getHelloWorldPipelineRun(suffix int, namespace string) *v1alpha1.PipelineRun {
return tb.PipelineRun(getName(pipelineRunName, suffix), namespace, tb.PipelineRunSpec(
getName(pipelineName, suffix),
tb.PipelineRunServiceAccount(fmt.Sprintf("%s%d", saName, suffix)),
))
return tb.PipelineRun(getName(pipelineRunName, suffix), namespace,
tb.PipelineRunLabel("hello-world-key", "hello-world-value"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This just adds a custom label to the service account propagation test. @shashwathi is that what you had in mind, or do you think I should create a separate test for labels specifically? (I wasn't sure how conservative we wanted to be about adding new e2e tests)

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good. Thank you @dwnusbaum

tb.PipelineRunSpec(getName(pipelineName, suffix),
tb.PipelineRunServiceAccount(fmt.Sprintf("%s%d", saName, suffix)),
),
)
}

func getName(namespace string, suffix int) string {
Expand Down Expand Up @@ -370,3 +379,25 @@ func collectMatchingEvents(kubeClient *knativetest.KubeClient, namespace string,
}
}
}

func getPodForTaskRun(t *testing.T, kubeClient *knativetest.KubeClient, namespace string, tr *v1alpha1.TaskRun) *corev1.Pod {
// The Pod name has a random suffix, so we filter by label to find the one we care about.
pods, err := kubeClient.Kube.CoreV1().Pods(namespace).List(metav1.ListOptions{
LabelSelector: pipeline.GroupName + pipeline.TaskRunLabelKey + " = " + tr.Name,
})
if err != nil {
t.Fatalf("Couldn't get expected Pod for %s: %s", tr.Name, err)
}
if numPods := len(pods.Items); numPods != 1 {
t.Fatalf("Expected 1 Pod for %s, but got %d Pods", tr.Name, numPods)
}
return &pods.Items[0]
}

func assertLabelsMatch(t *testing.T, expectedLabels, actualLabels map[string]string) {
for key, expectedVal := range expectedLabels {
if actualVal := actualLabels[key]; actualVal != expectedVal {
t.Errorf("Expected labels containing %s=%s but labels were %v", key, expectedVal, actualLabels)
}
}
}