Repository: knative/client
Pull request #1019 (2020-09-18 10:25:05)

Print logs from PipelineRun pods when Tekton tests fail
## Description

It often happens that the tests fail and there's no error message. This is very helpful when analyzing the failures.

<!-- Please add a short summary about what the pull request is going to bring on the table -->

## Changes

<!-- Please add list of more detailed changes. These changes should be reflected also in the commit messages -->

*
*
*

## Reference

<!-- Please add the corresponding issue number which this pull request is about to fix -->
Fixes #

<!--
Please add an entrty to CHANGELOG.adoc file, too, as part of your Pull Request.

In the following cases, add a short description of PR to the unreleased section in CHANGELOG.adoc:

- 🎁 New feature
- 🐛 Bug fix
- ✨ Feature Update
- 🐣 Refactoring
- 🗑️ Remove feature or internal logic

See other entries in CHANGELOG.adoc as an example for how to add the entry, including a reference to the corresponding issue/PR

PLEASE DON'T ADD THAT LINE HERE IN THE PULL-REQUEST DESCRIPTION BUT DIRECTLY IN CHANGELOG.ADOC AND ADD CHANGELOG.ADOC AS PART OF YOUR PULL-REQUEST.
-->

<!--
To automatically lint go code in this pull request uncomment the line below. You get feedback as comments on your pull request then -->

<!--
/lint
-->


knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/1019#" title="Author self-approved">mgencur</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **evankanderson**
You can assign the PR to them by writing `/assign @evankanderson` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative/client/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["evankanderson"]} -->
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/1019#" title="Author self-approved">mgencur</a>*, *<a href="https://github.com/knative/client/pull/1019#pullrequestreview-491350895" title="Approved">navidshaikh</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/client/blob/master/OWNERS)~~ [navidshaikh]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
knative-prow-robot: @mgencur: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-client-integration-tests | f95c3ca94b40d4079cdc6e487b2cce2a6369582c | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_client/1019/pull-knative-client-integration-tests/1306902318045728771) | `/test pull-knative-client-integration-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_client/1019). [Your PR dashboard](https://gubernator.knative.dev/pr/mgencur).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go
index d76d2381..8050e146 100644
--- a/pkg/kn/commands/service/configuration_edit_flags.go
+++ b/pkg/kn/commands/service/configuration_edit_flags.go
@@ -412,10 +412,23 @@ func (p *ConfigurationEditFlags) Apply(
 		}
 
 		annotationsToRemove := util.ParseMinusSuffix(annotationsMap)
-		err = servinglib.UpdateAnnotations(service, template, annotationsMap, annotationsToRemove)
+		// Service Annotations can't contain Autoscaling ones
+		serviceAnnotations := make(map[string]string)
+		for key, value := range annotationsMap {
+			if !strings.HasPrefix(key, autoscaling.GroupName) {
+				serviceAnnotations[key] = value
+			}
+		}
+		// Add all user provided annotations to RevisionTemplate
+		err = servinglib.UpdateRevisionTemplateAnnotations(template, annotationsMap, annotationsToRemove)
+		if err != nil {
+			return err
+		}
+		err = servinglib.UpdateServiceAnnotations(service, serviceAnnotations, annotationsToRemove)
 		if err != nil {
 			return err
 		}
+
 	}
 
 	if cmd.Flags().Changed("service-account") {
@@ -446,11 +459,8 @@ func (p *ConfigurationEditFlags) Apply(
 		if cmd.Flags().Changed("annotation") && containsAnnotation(p.Annotations, autoscaling.InitialScaleAnnotationKey) {
 			return fmt.Errorf("only one of the --scale-init or --annotation %s can be specified", autoscaling.InitialScaleAnnotationKey)
 		}
-		annotationsMap := map[string]string{
-			autoscaling.InitialScaleAnnotationKey: strconv.Itoa(p.ScaleInit),
-		}
-
-		err = servinglib.UpdateAnnotations(service, template, annotationsMap, []string{})
+		// Autoscaling annotations are only applicable on Revision Template, not Service
+		err = servinglib.UpdateRevisionTemplateAnnotation(template, autoscaling.InitialScaleAnnotationKey, strconv.Itoa(p.ScaleInit))
 		if err != nil {
 			return err
 		}
diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go
index 057fc32a..74fcb32b 100644
--- a/pkg/serving/config_changes.go
+++ b/pkg/serving/config_changes.go
@@ -216,30 +216,6 @@ func UpdateConcurrencyLimit(template *servingv1.RevisionTemplateSpec, limit int6
 	return nil
 }
 
-// UpdateRevisionTemplateAnnotation updates an annotation for the given Revision Template.
-// Also validates the autoscaling annotation values
-func UpdateRevisionTemplateAnnotation(template *servingv1.RevisionTemplateSpec, annotation string, value string) error {
-	annoMap := template.Annotations
-	if annoMap == nil {
-		annoMap = make(map[string]string)
-		template.Annotations = annoMap
-	}
-
-	// Validate autoscaling annotations and returns error if invalid input provided
-	// without changing the existing spec
-	in := make(map[string]string)
-	in[annotation] = value
-	// The boolean indicates whether or not the init-scale annotation can be set to 0.
-	// Since we don't have the config handy, err towards allowing it. The API will
-	// correctly fail the request if it's forbidden.
-	if err := autoscaling.ValidateAnnotations(true, in); err != nil {
-		return err
-	}
-
-	annoMap[annotation] = value
-	return nil
-}
-
 // EnvToMap is an utility function to translate between the API list form of env vars, and the
 // more convenient map form.
 func EnvToMap(vars []corev1.EnvVar) (map[string]string, error) {
@@ -452,33 +428,30 @@ func UpdateLabels(labelsMap map[string]string, add map[string]string, remove []s
 	return labelsMap
 }
 
-// UpdateAnnotations updates the annotations identically on a service and template.
-// Does not overwrite the entire Annotations field, only makes the requested updates.
-func UpdateAnnotations(
-	service *servingv1.Service,
-	template *servingv1.RevisionTemplateSpec,
-	toUpdate map[string]string,
-	toRemove []string) error {
-
-	if service.ObjectMeta.Annotations == nil {
-		service.ObjectMeta.Annotations = make(map[string]string)
-	}
-
-	if template.ObjectMeta.Annotations == nil {
-		template.ObjectMeta.Annotations = make(map[string]string)
+// UpdateServiceAnnotations updates annotations for the given Service Metadata.
+func UpdateServiceAnnotations(service *servingv1.Service, toUpdate map[string]string, toRemove []string) error {
+	if service.Annotations == nil && len(toUpdate) > 0 {
+		service.Annotations = make(map[string]string)
 	}
+	return updateAnnotations(service.Annotations, toUpdate, toRemove)
+}
 
-	for key, value := range toUpdate {
-		service.ObjectMeta.Annotations[key] = value
-		template.ObjectMeta.Annotations[key] = value
+// UpdateRevisionTemplateAnnotations updates annotations for the given Revision Template.
+// Also validates the autoscaling annotation values
+func UpdateRevisionTemplateAnnotations(template *servingv1.RevisionTemplateSpec, toUpdate map[string]string, toRemove []string) error {
+	if err := autoscaling.ValidateAnnotations(true, toUpdate); err != nil {
+		return err
 	}
-
-	for _, key := range toRemove {
-		delete(service.ObjectMeta.Annotations, key)
-		delete(template.ObjectMeta.Annotations, key)
+	if template.Annotations == nil {
+		template.Annotations = make(map[string]string)
 	}
+	return updateAnnotations(template.Annotations, toUpdate, toRemove)
+}
 
-	return nil
+// UpdateRevisionTemplateAnnotation updates an annotation for the given Revision Template.
+// Also validates the autoscaling annotation values
+func UpdateRevisionTemplateAnnotation(template *servingv1.RevisionTemplateSpec, annotation string, value string) error {
+	return UpdateRevisionTemplateAnnotations(template, map[string]string{annotation: value}, []string{})
 }
 
 // UpdateServiceAccountName updates the service account name used for the corresponding knative service
@@ -543,6 +516,16 @@ func GenerateVolumeName(path string) string {
 
 // =======================================================================================
 
+func updateAnnotations(annotations map[string]string, toUpdate map[string]string, toRemove []string) error {
+	for key, value := range toUpdate {
+		annotations[key] = value
+	}
+	for _, key := range toRemove {
+		delete(annotations, key)
+	}
+	return nil
+}
+
 func updateEnvVarsFromMap(env []corev1.EnvVar, toUpdate map[string]string) []corev1.EnvVar {
 	set := sets.NewString()
 	for i := range env {


Test patch:
diff --git a/pkg/kn/commands/service/create_mock_test.go b/pkg/kn/commands/service/create_mock_test.go
index 627c86fd..0c919ee7 100644
--- a/pkg/kn/commands/service/create_mock_test.go
+++ b/pkg/kn/commands/service/create_mock_test.go
@@ -18,6 +18,8 @@ import (
 	"testing"
 	"time"
 
+	"knative.dev/serving/pkg/apis/autoscaling"
+
 	"gotest.tools/assert"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -545,6 +547,38 @@ func TestServiceCreateWithBothAnnotationAndInitScaleAsOption(t *testing.T) {
 	r.Validate()
 }
 
+func TestServiceCreateWithAnnotations(t *testing.T) {
+	client := knclient.NewMockKnServiceClient(t)
+
+	r := client.Recorder()
+	r.GetService("foo", nil, errors.NewNotFound(servingv1.Resource("service"), "foo"))
+
+	service := getService("foo")
+	template := &service.Spec.Template
+
+	service.ObjectMeta.Annotations = map[string]string{
+		"foo": "bar",
+	}
+
+	template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz"
+	template.ObjectMeta.Annotations = map[string]string{
+		autoscaling.InitialScaleAnnotationKey: "1", // autoscaling in only added Revision Template
+		"foo":                                 "bar",
+		servinglib.UserImageAnnotationKey:     "gcr.io/foo/bar:baz",
+	}
+
+	r.CreateService(service, nil)
+
+	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz",
+		"--annotation", "foo=bar",
+		"--annotation", autoscaling.InitialScaleAnnotationKey+"=1",
+		"--no-wait", "--revision-name=")
+	assert.NilError(t, err)
+	assert.Assert(t, util.ContainsAll(output, "created", "foo", "default"))
+
+	r.Validate()
+}
+
 func getServiceWithUrl(name string, urlName string) *servingv1.Service {
 	service := servingv1.Service{}
 	url, _ := apis.ParseURL(urlName)
diff --git a/pkg/kn/commands/service/service_update_mock_test.go b/pkg/kn/commands/service/service_update_mock_test.go
index 783ac7c8..885647f6 100644
--- a/pkg/kn/commands/service/service_update_mock_test.go
+++ b/pkg/kn/commands/service/service_update_mock_test.go
@@ -17,6 +17,8 @@ package service
 import (
 	"testing"
 
+	"knative.dev/serving/pkg/apis/autoscaling"
+
 	"gotest.tools/assert"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -77,10 +79,11 @@ func TestServiceUpdateAnnotationsMock(t *testing.T) {
 		"an3": "getsRemoved",
 	}
 	template.ObjectMeta.Annotations = map[string]string{
-		"an1":                                "staysConstant",
-		"an2":                                "getsUpdated",
-		"an3":                                "getsRemoved",
-		clientserving.UserImageAnnotationKey: "gcr.io/foo/bar:baz",
+		"an1":                                 "staysConstant",
+		"an2":                                 "getsUpdated",
+		"an3":                                 "getsRemoved",
+		clientserving.UserImageAnnotationKey:  "gcr.io/foo/bar:baz",
+		autoscaling.InitialScaleAnnotationKey: "1",
 	}
 
 	updatedService := getService(svcName)
@@ -91,9 +94,10 @@ func TestServiceUpdateAnnotationsMock(t *testing.T) {
 		"an2": "isUpdated",
 	}
 	template.ObjectMeta.Annotations = map[string]string{
-		"an1":                                "staysConstant",
-		"an2":                                "isUpdated",
-		clientserving.UserImageAnnotationKey: "gcr.io/foo/bar:baz",
+		"an1":                                 "staysConstant",
+		"an2":                                 "isUpdated",
+		clientserving.UserImageAnnotationKey:  "gcr.io/foo/bar:baz",
+		autoscaling.InitialScaleAnnotationKey: "2",
 	}
 
 	r := client.Recorder()
@@ -104,6 +108,7 @@ func TestServiceUpdateAnnotationsMock(t *testing.T) {
 		"--annotation", "an1=staysConstant",
 		"--annotation", "an2=getsUpdated",
 		"--annotation", "an3=getsRemoved",
+		"--annotation", autoscaling.InitialScaleAnnotationKey+"=1",
 		"--no-wait", "--revision-name=",
 	)
 	assert.NilError(t, err)
@@ -113,6 +118,7 @@ func TestServiceUpdateAnnotationsMock(t *testing.T) {
 		"update", svcName,
 		"--annotation", "an2=isUpdated",
 		"--annotation", "an3-",
+		"--annotation", autoscaling.InitialScaleAnnotationKey+"=2",
 		"--no-wait", "--revision-name=",
 	)
 	assert.NilError(t, err)
@@ -1480,9 +1486,6 @@ func TestServiceUpdateInitialScaleMock(t *testing.T) {
 	newService := getService(svcName)
 	template := &newService.Spec.Template
 	template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz"
-	newService.ObjectMeta.Annotations = map[string]string{
-		"autoscaling.knative.dev/initialScale": "1",
-	}
 	template.ObjectMeta.Annotations = map[string]string{
 		"autoscaling.knative.dev/initialScale": "1",
 		clientserving.UserImageAnnotationKey:   "gcr.io/foo/bar:baz",
@@ -1491,9 +1494,6 @@ func TestServiceUpdateInitialScaleMock(t *testing.T) {
 	updatedService := getService(svcName)
 	template = &updatedService.Spec.Template
 	template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz"
-	updatedService.ObjectMeta.Annotations = map[string]string{
-		"autoscaling.knative.dev/initialScale": "2",
-	}
 	template.ObjectMeta.Annotations = map[string]string{
 		"autoscaling.knative.dev/initialScale": "2",
 		clientserving.UserImageAnnotationKey:   "gcr.io/foo/bar:baz",
diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go
index bd985e00..eed1b75c 100644
--- a/pkg/serving/config_changes_test.go
+++ b/pkg/serving/config_changes_test.go
@@ -579,38 +579,81 @@ func TestUpdateImagePullSecrets(t *testing.T) {
 	assert.Check(t, template.Spec.ImagePullSecrets == nil)
 }
 
+func TestUpdateRevisionTemplateAnnotationsNew(t *testing.T) {
+	_, template, _ := getService()
+
+	annotations := map[string]string{
+		autoscaling.InitialScaleAnnotationKey: "1",
+		autoscaling.MaxScaleAnnotationKey:     "2",
+	}
+	err := UpdateRevisionTemplateAnnotations(template, annotations, []string{})
+	assert.NilError(t, err)
+
+	actual := template.ObjectMeta.Annotations
+	assert.DeepEqual(t, annotations, actual)
+}
+
+func TestUpdateRevisionTemplateAnnotationsExisting(t *testing.T) {
+	_, template, _ := getService()
+	template.ObjectMeta.Annotations = map[string]string{
+		autoscaling.InitialScaleAnnotationKey: "1",
+		autoscaling.MaxScaleAnnotationKey:     "2",
+	}
+
+	annotations := map[string]string{
+		autoscaling.InitialScaleAnnotationKey: "5",
+		autoscaling.MaxScaleAnnotationKey:     "10",
+	}
+	err := UpdateRevisionTemplateAnnotations(template, annotations, []string{})
+	assert.NilError(t, err)
+
+	actual := template.ObjectMeta.Annotations
+	assert.DeepEqual(t, annotations, actual)
+}
+
+func TestUpdateRevisionTemplateAnnotationsRemoveExisting(t *testing.T) {
+	_, template, _ := getService()
+	template.ObjectMeta.Annotations = map[string]string{
+		autoscaling.InitialScaleAnnotationKey: "1",
+		autoscaling.MaxScaleAnnotationKey:     "2",
+	}
+	expectedAnnotations := map[string]string{
+		autoscaling.InitialScaleAnnotationKey: "1",
+	}
+	remove := []string{autoscaling.MaxScaleAnnotationKey}
+	err := UpdateRevisionTemplateAnnotations(template, map[string]string{}, remove)
+	assert.NilError(t, err)
+
+	actual := template.ObjectMeta.Annotations
+	assert.DeepEqual(t, expectedAnnotations, actual)
+}
+
 func TestUpdateAnnotationsNew(t *testing.T) {
-	service, template, _ := getService()
+	service, _, _ := getService()
 
 	annotations := map[string]string{
 		"a": "foo",
 		"b": "bar",
 	}
-	err := UpdateAnnotations(service, template, annotations, []string{})
+	err := UpdateServiceAnnotations(service, annotations, []string{})
 	assert.NilError(t, err)
 
 	actual := service.ObjectMeta.Annotations
 	if !reflect.DeepEqual(annotations, actual) {
 		t.Fatalf("Service annotations did not match expected %v found %v", annotations, actual)
 	}
-
-	actual = template.ObjectMeta.Annotations
-	if !reflect.DeepEqual(annotations, actual) {
-		t.Fatalf("Template annotations did not match expected %v found %v", annotations, actual)
-	}
 }
 
 func TestUpdateAnnotationsExisting(t *testing.T) {
-	service, template, _ := getService()
+	service, _, _ := getService()
 	service.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"}
-	template.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"}
 
 	annotations := map[string]string{
 		"a": "notfoo",
 		"c": "bat",
 		"d": "",
 	}
-	err := UpdateAnnotations(service, template, annotations, []string{})
+	err := UpdateServiceAnnotations(service, annotations, []string{})
 	assert.NilError(t, err)
 	expected := map[string]string{
 		"a": "notfoo",
@@ -621,18 +664,14 @@ func TestUpdateAnnotationsExisting(t *testing.T) {
 
 	actual := service.ObjectMeta.Annotations
 	assert.DeepEqual(t, expected, actual)
-
-	actual = template.ObjectMeta.Annotations
-	assert.DeepEqual(t, expected, actual)
 }
 
 func TestUpdateAnnotationsRemoveExisting(t *testing.T) {
-	service, template, _ := getService()
+	service, _, _ := getService()
 	service.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"}
-	template.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"}
 
 	remove := []string{"b"}
-	err := UpdateAnnotations(service, template, map[string]string{}, remove)
+	err := UpdateServiceAnnotations(service, map[string]string{}, remove)
 	assert.NilError(t, err)
 	expected := map[string]string{
 		"a": "foo",
@@ -640,9 +679,6 @@ func TestUpdateAnnotationsRemoveExisting(t *testing.T) {
 
 	actual := service.ObjectMeta.Annotations
 	assert.DeepEqual(t, expected, actual)
-
-	actual = template.ObjectMeta.Annotations
-	assert.DeepEqual(t, expected, actual)
 }
 
 func TestGenerateVolumeName(t *testing.T) {
diff --git a/test/e2e/service_options_test.go b/test/e2e/service_options_test.go
index 586eb430..c0f5ffdb 100644
--- a/test/e2e/service_options_test.go
+++ b/test/e2e/service_options_test.go
@@ -25,6 +25,8 @@ import (
 	"strings"
 	"testing"
 
+	"knative.dev/serving/pkg/apis/autoscaling"
+
 	"gotest.tools/assert"
 
 	"knative.dev/client/lib/test"
@@ -156,6 +158,11 @@ func TestServiceOptions(t *testing.T) {
 	t.Log("delete service")
 	test.ServiceDelete(r, "svc10")
 
+	t.Log("create and validate service with scale init option via --annotation flag")
+	serviceCreateWithOptions(r, "svc11", "--annotation", autoscaling.InitialScaleAnnotationKey+"=2")
+	validateServiceInitScale(r, "svc11", "2")
+	t.Log("delete service")
+	test.ServiceDelete(r, "svc11")
 }
 
 func serviceCreateWithOptions(r *test.KnRunResultCollector, serviceName string, options ...string) {
diff --git a/test/e2e/tekton_test.go b/test/e2e/tekton_test.go
index a4951ec2..228cc835 100644
--- a/test/e2e/tekton_test.go
+++ b/test/e2e/tekton_test.go
@@ -17,6 +17,7 @@
 package e2e
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 	"testing"
@@ -75,7 +76,11 @@ func TestTektonPipeline(t *testing.T) {
 	assert.NilError(t, err)
 
 	err = waitForPipelineSuccess(kubectl)
-	assert.NilError(t, err)
+	if err != nil {
+		logs, logsErr := kubectl.Run("logs", "-l", "tekton.dev/pipeline=buildah-build-kn-create", "--all-containers", "--prefix=true")
+		assert.NilError(t, logsErr, "Unable to gather logs from pipeline pods")
+		t.Fatalf("PipelineRun failed with %v, printing logs from pipeline pods:\n%s", err, logs)
+	}
 
 	r := test.NewKnRunResultCollector(t, it)
 
@@ -89,7 +94,14 @@ func TestTektonPipeline(t *testing.T) {
 func waitForPipelineSuccess(k test.Kubectl) error {
 	return wait.PollImmediate(Interval, Timeout, func() (bool, error) {
 		out, err := k.Run("get", "pipelinerun", "-o=jsonpath='{.items[0].status.conditions[?(@.type==\"Succeeded\")].status}'")
-		return strings.Contains(out, "True"), err
+		if err != nil {
+			return false, err
+		}
+		// Return early if the run failed.
+		if strings.Contains(out, "False") {
+			return false, errors.New("pipelinerun failure")
+		}
+		return strings.Contains(out, "True"), nil
 	})
 }