From 7afd613a76ccb909ec8adfc36c407510dd22ced1 Mon Sep 17 00:00:00 2001 From: Billy Lynch Date: Wed, 13 Jan 2021 15:30:19 -0500 Subject: [PATCH] Use Object UIDs as Result/Record names. This uses Object UIDs as the Record name in order to accomplish a few different things: - Fixes an issue where k8s object names (<= 253 chars) didn't fit in to Result name parts (<= 63 chars). UIDs - Sidesteps the original reason why we tried prepending the type in the first place to distinguish TaskRuns/PipelineRuns with the same name. (UIDs are unique for the cluster). - Allows us to detect and distinguish cases where users delete then recreate objects with the same name. Using UIDs will cause us to create new Results/Records for each new instance, instead of trying to update the existing data. The downside of this change means that Result/Record names are no longer human readable, which means that users will need to rely more on Record data to distinguish entries. --- docs/watcher/README.md | 10 +- .../reconciler/pipelinerun/controller.go | 2 +- pkg/watcher/reconciler/reconciler_test.go | 13 ++- pkg/watcher/reconciler/taskrun/controller.go | 2 +- pkg/watcher/results/results.go | 42 ++++--- pkg/watcher/results/results_test.go | 103 +++++++++++------- test/e2e/02-test.sh | 16 +-- test/e2e/pipelinerun.yaml | 13 +++ 8 files changed, 117 insertions(+), 84 deletions(-) create mode 100644 test/e2e/pipelinerun.yaml diff --git a/docs/watcher/README.md b/docs/watcher/README.md index 6b69ba9db..841e5f38b 100644 --- a/docs/watcher/README.md +++ b/docs/watcher/README.md @@ -11,10 +11,10 @@ The Watcher currently supports the following types: ## Result Grouping -The Watcher uses annotations to automatically detect and group related Records into the same Result. The following annotations are recognized (in order of precedence): +The Watcher uses Object data to automatically detect and group related Records into the same Result. The following data is checked (listed in order of precedence): -- `results.tekton.dev/result` -- `triggers.tekton.dev/triggers-eventid` -- `tekton.dev/pipelineRun` +- `results.tekton.dev/result` annotation. This should correspond to the full `Result.name` identifier (e.g. `foo/results/bar`). +- `triggers.tekton.dev/triggers-eventid` label (this is generated from Objects created via [Tekton Triggers](https://github.com/tektoncd/triggers)) +- An [OwnerReference](https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/#owners-and-dependents) to a PipelineRun. -If no annotation is detected, the Watcher will automatically generate a new Result name for the object. \ No newline at end of file +If no annotation is detected, the Watcher will automatically generate a new Result name for the Object. \ No newline at end of file diff --git a/pkg/watcher/reconciler/pipelinerun/controller.go b/pkg/watcher/reconciler/pipelinerun/controller.go index 610eed307..80b65b769 100644 --- a/pkg/watcher/reconciler/pipelinerun/controller.go +++ b/pkg/watcher/reconciler/pipelinerun/controller.go @@ -37,7 +37,7 @@ func NewControllerWithConfig(ctx context.Context, client pb.ResultsClient, cfg * pipelineRunInformer := pipelineruninformer.Get(ctx) pipelineclientset := pipelineclient.Get(ctx) c := &Reconciler{ - client: results.NewClient(client, "pipelinerun"), + client: results.NewClient(client), pipelineclientset: pipelineclientset, cfg: cfg, } diff --git a/pkg/watcher/reconciler/reconciler_test.go b/pkg/watcher/reconciler/reconciler_test.go index 05158c350..78aa89c15 100644 --- a/pkg/watcher/reconciler/reconciler_test.go +++ b/pkg/watcher/reconciler/reconciler_test.go @@ -80,7 +80,12 @@ func reconcileTaskRun(ctx context.Context, t *testing.T, client *fake.Clientset) // be associated with the PipelineRun result. "tekton.dev/pipelineRun": "pr", }, - UID: "12345", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "tekton.dev/v1beta1", + Kind: "PipelineRun", + UID: "pr-id", + }}, + UID: "tr-id", }, }) if err != nil { @@ -97,7 +102,7 @@ func reconcileTaskRun(ctx context.Context, t *testing.T, client *fake.Clientset) t.Log(err) } if got := tr.Annotations[annotation.Result]; err == nil && got != "" { - want := "ns/results/pipelinerun-pr" + want := "ns/results/pr-id" if got != want { t.Fatalf("want result ID %s, got %s", want, got) } @@ -120,7 +125,7 @@ func reconcilePipelineRun(ctx context.Context, t *testing.T, client *fake.Client Name: "pr", Namespace: "ns", Annotations: map[string]string{"demo": "demo"}, - UID: "12345", + UID: "pr-id", }, }) if err != nil { @@ -138,7 +143,7 @@ func reconcilePipelineRun(ctx context.Context, t *testing.T, client *fake.Client } if err == nil && pr.Annotations[annotation.Result] != "" { if got := pr.Annotations[annotation.Result]; err == nil && got != "" { - want := "ns/results/pipelinerun-pr" + want := "ns/results/pr-id" if got != want { t.Fatalf("want result ID %s, got %s", want, got) } diff --git a/pkg/watcher/reconciler/taskrun/controller.go b/pkg/watcher/reconciler/taskrun/controller.go index db59d6d7a..ea8955527 100644 --- a/pkg/watcher/reconciler/taskrun/controller.go +++ b/pkg/watcher/reconciler/taskrun/controller.go @@ -37,7 +37,7 @@ func NewControllerWithConfig(ctx context.Context, client pb.ResultsClient, cfg * pipelineclientset := pipelineclient.Get(ctx) c := &Reconciler{ - client: results.NewClient(client, "taskrun"), + client: results.NewClient(client), pipelineclientset: pipelineclientset, cfg: cfg, } diff --git a/pkg/watcher/results/results.go b/pkg/watcher/results/results.go index cd41cb546..ed19a0f09 100644 --- a/pkg/watcher/results/results.go +++ b/pkg/watcher/results/results.go @@ -16,7 +16,6 @@ package results import ( "context" - "fmt" "strings" "github.com/tektoncd/results/pkg/api/server/v1alpha2/record" @@ -35,19 +34,12 @@ import ( // operations. type Client struct { pb.ResultsClient - - // We need to know what kind of type we're working with, since this - // information is not returned back in Get requests. - // See https://github.com/kubernetes/kubernetes/issues/3030 for more details. - // We might be able to do something clever with schemes in the future. - kind string } // NewClient returns a new results client for the particular kind. -func NewClient(client pb.ResultsClient, kind string) *Client { +func NewClient(client pb.ResultsClient) *Client { return &Client{ ResultsClient: client, - kind: kind, } } @@ -75,7 +67,7 @@ func (c *Client) Put(ctx context.Context, o metav1.Object, opts ...grpc.CallOpti // ensureResult gets the Result corresponding to the Object, or creates a new // one. func (c *Client) ensureResult(ctx context.Context, o metav1.Object, opts ...grpc.CallOption) (*pb.Result, error) { - name := c.resultName(o) + name := resultName(o) res, err := c.ResultsClient.GetResult(ctx, &pb.GetResultRequest{Name: name}, opts...) if err != nil && status.Code(err) != codes.NotFound { return nil, status.Errorf(status.Code(err), "GetResult(%s): %v", name, err) @@ -97,27 +89,31 @@ func (c *Client) ensureResult(ctx context.Context, o metav1.Object, opts ...grpc // resultName gets the result name to use for the given object. // The name is derived from a known Tekton annotation if available, else // the object's name is used. -func (c *Client) resultName(o metav1.Object) string { - a := o.GetAnnotations() +func resultName(o metav1.Object) string { // Special case result annotations, since this should already be the // full result identifier. - if v, ok := a[annotation.Result]; ok { + if v, ok := o.GetAnnotations()[annotation.Result]; ok { return v } var part string - if v, ok := a["triggers.tekton.dev/triggers-eventid"]; ok { + if v, ok := o.GetLabels()["triggers.tekton.dev/triggers-eventid"]; ok { // Don't prefix trigger events. These are 1) not CRD types, 2) are // intended to be unique identifiers already, and 3) should be applied // to all objects created via trigger templates, so there's no need to // prefix these to avoid collision. part = v - } else if v, ok := a["tekton.dev/pipelineRun"]; ok { - // Prefix found pipelineruns with the kind to match the defaultName - // output for pipelineruns. - part = fmt.Sprintf("pipelinerun-%s", v) - } else { - part = c.defaultName(o) + } else if len(o.GetOwnerReferences()) > 0 { + for _, owner := range o.GetOwnerReferences() { + if strings.EqualFold(owner.Kind, "pipelinerun") { + part = string(owner.UID) + break + } + } + } + + if part == "" { + part = defaultName(o) } return result.FormatName(o.GetNamespace(), part) } @@ -126,7 +122,7 @@ func (c *Client) resultName(o metav1.Object) string { func (c *Client) upsertRecord(ctx context.Context, parent string, o metav1.Object, opts ...grpc.CallOption) (*pb.Record, error) { name, ok := o.GetAnnotations()[annotation.Record] if !ok { - name = record.FormatName(parent, c.defaultName(o)) + name = record.FormatName(parent, defaultName(o)) } data, err := convert.ToProto(o) @@ -159,6 +155,6 @@ func (c *Client) upsertRecord(ctx context.Context, parent string, o metav1.Objec // defaultName is the default Result/Record name that should be used if one is // not already associated to the Object. -func (c *Client) defaultName(o metav1.Object) string { - return strings.ToLower(fmt.Sprintf("%s-%s", c.kind, o.GetName())) +func defaultName(o metav1.Object) string { + return string(o.GetUID()) } diff --git a/pkg/watcher/results/results_test.go b/pkg/watcher/results/results_test.go index 011044bab..c925fd73b 100644 --- a/pkg/watcher/results/results_test.go +++ b/pkg/watcher/results/results_test.go @@ -32,27 +32,27 @@ import ( ) func TestDefaultName(t *testing.T) { - // No need to create a full client - we're only testing a utility method. - client := &Client{kind: "test"} - want := "test-name" + want := "id" objs := []metav1.Object{ &v1beta1.TaskRun{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "test", + UID: "id", }, }, &v1beta1.PipelineRun{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "test", + UID: "id", }, }, } for _, o := range objs { t.Run(fmt.Sprintf("%T", o), func(t *testing.T) { - if got := client.defaultName(o); want != got { + if got := defaultName(o); want != got { t.Errorf("want %s, got %s", want, got) } }) @@ -60,39 +60,48 @@ func TestDefaultName(t *testing.T) { } func TestResultName(t *testing.T) { - // No need to create a full client - we're only testing a utility method. - client := &Client{kind: "test"} + ownerRef := []metav1.OwnerReference{{ + Kind: "PipelineRun", + UID: "pipelinerun", + }} for _, tc := range []struct { name string + modify func(o metav1.Object) annotations map[string]string want string }{ { name: "object name", - want: "test/results/test-object", + want: "test/results/id", }, { name: "pipeline run", - annotations: map[string]string{ - "tekton.dev/pipelineRun": "pipelinerun", + modify: func(o metav1.Object) { + o.SetOwnerReferences(ownerRef) }, - want: "test/results/pipelinerun-pipelinerun", + want: "test/results/pipelinerun", }, { name: "trigger event", - annotations: map[string]string{ - "triggers.tekton.dev/triggers-eventid": "trigger", - "tekton.dev/pipelineRun": "pipelinerun", + modify: func(o metav1.Object) { + o.SetOwnerReferences(ownerRef) + o.SetLabels(map[string]string{ + "triggers.tekton.dev/triggers-eventid": "trigger", + }) }, want: "test/results/trigger", }, { name: "result", - annotations: map[string]string{ - annotation.Result: "result", - "triggers.tekton.dev/triggers-eventid": "trigger", - "tekton.dev/pipelineRun": "pipelinerun", + modify: func(o metav1.Object) { + o.SetOwnerReferences(ownerRef) + o.SetLabels(map[string]string{ + "triggers.tekton.dev/triggers-eventid": "trigger", + }) + o.SetAnnotations(map[string]string{ + annotation.Result: "result", + }) }, // This is not modified, since we assume that results are referred // to by the full name already. @@ -102,12 +111,15 @@ func TestResultName(t *testing.T) { t.Run(tc.name, func(t *testing.T) { o := &v1beta1.TaskRun{ ObjectMeta: metav1.ObjectMeta{ - Name: "object", - Namespace: "test", - Annotations: tc.annotations, + Name: "object", + Namespace: "test", + UID: "id", }, } - if got := client.resultName(o); tc.want != got { + if tc.modify != nil { + tc.modify(o) + } + if got := resultName(o); tc.want != got { t.Errorf("want %s, got %s", tc.want, got) } }) @@ -123,17 +135,19 @@ func TestEnsureResult(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "taskrun", Namespace: "test", + UID: "taskrun-id", }, }, &v1beta1.PipelineRun{ ObjectMeta: metav1.ObjectMeta{ Name: "pipelinerun", Namespace: "test", + UID: "pipelinerun-id", }, }, } for _, o := range objs { - name := fmt.Sprintf("test/results/test-%s", o.GetName()) + name := fmt.Sprintf("test/results/%s", o.GetUID()) // Sanity check Result doesn't exist. if r, err := client.GetResult(ctx, &pb.GetResultRequest{Name: name}); status.Code(err) != codes.NotFound { @@ -168,12 +182,14 @@ func TestUpsertRecord(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "taskrun", Namespace: "test", + UID: "taskrun-id", }, }, &v1beta1.PipelineRun{ ObjectMeta: metav1.ObjectMeta{ Name: "pipelinerun", Namespace: "test", + UID: "pipelinerun-id", }, }, } @@ -183,7 +199,7 @@ func TestUpsertRecord(t *testing.T) { t.Fatal(err) } - name := fmt.Sprintf("%s/records/test-%s", result.GetName(), o.GetName()) + name := fmt.Sprintf("%s/records/%s", result.GetName(), o.GetUID()) // Sanity check Record doesn't exist if r, err := client.GetRecord(ctx, &pb.GetRecordRequest{Name: name}); status.Code(err) != codes.NotFound { @@ -226,12 +242,14 @@ func TestPut(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "taskrun", Namespace: "test", + UID: "taskrun-id", }, }, &v1beta1.PipelineRun{ ObjectMeta: metav1.ObjectMeta{ Name: "pipelinerun", Namespace: "test", + UID: "pipelinerun-id", }, }, } @@ -240,25 +258,27 @@ func TestPut(t *testing.T) { // simulate an update. // This is less exhaustive than the other tests, since Put is a wrapper // around ensureResult/upsertRecord. - for _, tc := range []string{"create", "update"} { - t.Run(tc, func(t *testing.T) { - if _, _, err := client.Put(ctx, o); err != nil { - t.Fatal(err) - } - }) - } + t.Run(o.GetName(), func(t *testing.T) { + for _, tc := range []string{"create", "update"} { + t.Run(tc, func(t *testing.T) { + if _, _, err := client.Put(ctx, o); err != nil { + t.Fatal(err) + } + }) + } - // Verify Result/Record exist. - if _, err := client.GetResult(ctx, &pb.GetResultRequest{ - Name: fmt.Sprintf("test/results/test-%s", o.GetName()), - }); err != nil { - t.Fatalf("GetResult: %v", err) - } - if _, err := client.GetRecord(ctx, &pb.GetRecordRequest{ - Name: fmt.Sprintf("test/results/test-%s/records/test-%s", o.GetName(), o.GetName()), - }); err != nil { - t.Fatalf("GetRecord: %v", err) - } + // Verify Result/Record exist. + if _, err := client.GetResult(ctx, &pb.GetResultRequest{ + Name: fmt.Sprintf("test/results/%s", o.GetUID()), + }); err != nil { + t.Fatalf("GetResult: %v", err) + } + if _, err := client.GetRecord(ctx, &pb.GetRecordRequest{ + Name: fmt.Sprintf("test/results/%s/records/%s", o.GetUID(), o.GetUID()), + }); err != nil { + t.Fatalf("GetRecord: %v", err) + } + }) } } @@ -280,6 +300,5 @@ func client(t *testing.T) *Client { return &Client{ ResultsClient: test.NewResultsClient(t), - kind: "test", } } diff --git a/test/e2e/02-test.sh b/test/e2e/02-test.sh index 5c4d7c0ed..2d1164bf7 100755 --- a/test/e2e/02-test.sh +++ b/test/e2e/02-test.sh @@ -16,19 +16,19 @@ set -e ROOT="$(git rev-parse --show-toplevel)" -TASKRUN="${ROOT}/test/e2e/taskrun.yaml" +CONFIG="${ROOT}/test/e2e/pipelinerun.yaml" -kubectl delete -f "${TASKRUN}" || true -kubectl apply -f "${TASKRUN}" -echo "Waiting for TaskRun to complete..." -kubectl wait -f "${TASKRUN}" --for=condition=Succeeded +kubectl delete -f "${CONFIG}" || true +kubectl apply -f "${CONFIG}" +echo "Waiting for runs to complete..." +kubectl wait -f "${CONFIG}" --for=condition=Succeeded # Try a few times to get the result, since we might query before the reconciler # picks it up. for n in $(seq 10); do - result_id=$(kubectl get -f "${TASKRUN}" -o json | jq -r '.metadata.annotations."results.tekton.dev/result"') + result_id=$(kubectl get -f "${CONFIG}" -o json | jq -r '.metadata.annotations."results.tekton.dev/result"') if [[ "${result_id}" == "null" ]]; then - echo "Attempt #${n}: Could not find 'results.tekton.dev/result' for ${TASKRUN}" + echo "Attempt #${n}: Could not find 'results.tekton.dev/result' for ${CONFIG}" sleep 1 fi done @@ -39,4 +39,4 @@ if [[ "${result_id}" == "null" ]]; then fi echo "Found result ${result_id}" -echo "Success!" \ No newline at end of file +echo "Success!" diff --git a/test/e2e/pipelinerun.yaml b/test/e2e/pipelinerun.yaml new file mode 100644 index 000000000..78a294481 --- /dev/null +++ b/test/e2e/pipelinerun.yaml @@ -0,0 +1,13 @@ +apiVersion: tekton.dev/v1beta1 +kind: PipelineRun +metadata: + name: hello +spec: + pipelineSpec: + tasks: + - name: hello + taskSpec: + steps: + - name: hello + image: ubuntu + script: "echo hello world!"