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 1 commit
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
51 changes: 38 additions & 13 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,14 +161,36 @@ 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
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
}
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

for key, pipelineRunVal := range labels {
if taskRunVal := r.ObjectMeta.Labels[key]; taskRunVal != pipelineRunVal {
t.Errorf("Expected label %s=%s but got %s", key, pipelineRunVal, taskRunVal)
}
}

// Check label propagation to Pods
taskRunLabelKey := pipeline.GroupName + pipeline.TaskRunLabelKey
labels[taskRunLabelKey] = taskRunName
pods, err := c.KubeClient.Kube.CoreV1().Pods(namespace).List(metav1.ListOptions{
LabelSelector: taskRunLabelKey + " = " + taskRunName,
})
if err != nil {
t.Fatalf("Couldn't get expected Pod for %s: %s", taskRunName, err)
}
if numPods := len(pods.Items); numPods != 1 {
t.Fatalf("Expected 1 Pod for %s, but got %d Pods", taskRunName, numPods)
}
pod := pods.Items[0]
for key, taskRunVal := range labels {
if podVal := pod.ObjectMeta.Labels[key]; podVal != taskRunVal {
t.Errorf("Expected label %s=%s but got %s", key, taskRunVal, podVal)
Copy link
Collaborator

Choose a reason for hiding this comment

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

total nitpick: could these be in a separate function called by the test? for the same of readability (test is getting long!)

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 good point, I'll separate it out!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did some refactoring in 37ed488, let me know if that seems good or if you'd like to see a totally separate assertLabels function with all the label-related stuff to simplify further.

}
}
}
Expand Down Expand Up @@ -322,10 +345,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