Repository: knative/serving
Pull request #9837 (2020-10-15 14:41:35)

Run v1alpha1 tests (and deploy DomainMapping component) in github actions
Since we'll be [adding some tests](https://github.com/knative/serving/pull/9780) to this suite gated by the enable-alpha flag as part of the [DomainMapping work](https://github.com/knative/serving/issues/9713).

I think I'm holding GitHub actions right, but it's my first time trying so 🤷.

Example running on my fork here: https://github.com/julz/serving/runs/1258795911?check_suite_focus=true.

/assign @mattmoor @markusthoemmes 

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/9837#" title="Author self-approved">julz</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **tcnghia** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @tcnghia` 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%2Fserving).

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

- **[OWNERS](https://github.com/knative/serving/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":["tcnghia"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/9837?src=pr&el=h1) Report
> Merging [#9837](https://codecov.io/gh/knative/serving/pull/9837?src=pr&el=desc) into [master](https://codecov.io/gh/knative/serving/commit/65b83e6005b6592fb8d9ca9b7827db3ddb505f7d?el=desc) will **decrease** coverage by `0.05%`.
> The diff coverage is `0.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/serving/pull/9837/graphs/tree.svg?width=650&height=150&src=pr&token=MPpDfAj4Ui)](https://codecov.io/gh/knative/serving/pull/9837?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #9837      +/-   ##
==========================================
- Coverage   87.78%   87.72%   -0.06%     
==========================================
  Files         180      180              
  Lines        8570     8571       +1     
==========================================
- Hits         7523     7519       -4     
- Misses        799      803       +4     
- Partials      248      249       +1     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/9837?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/revision/revision.go](https://codecov.io/gh/knative/serving/pull/9837/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcmV2aXNpb24vcmV2aXNpb24uZ28=) | `84.21% <0.00%> (-1.51%)` | :arrow_down: |
| [pkg/reconciler/configuration/configuration.go](https://codecov.io/gh/knative/serving/pull/9837/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvY29uZmlndXJhdGlvbi9jb25maWd1cmF0aW9uLmdv) | `87.70% <0.00%> (-1.64%)` | :arrow_down: |
| [pkg/activator/net/revision\_backends.go](https://codecov.io/gh/knative/serving/pull/9837/diff?src=pr&el=tree#diff-cGtnL2FjdGl2YXRvci9uZXQvcmV2aXNpb25fYmFja2VuZHMuZ28=) | `90.49% <0.00%> (-0.91%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/9837?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/serving/pull/9837?src=pr&el=footer). Last update [65b83e6...04f6455](https://codecov.io/gh/knative/serving/pull/9837?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

knative-prow-robot: @julz: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-serving-istio-stable-no-mesh | 04f645589ff0d1991960e13966b43c2fa3fc160e | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/9837/pull-knative-serving-istio-stable-no-mesh/1316751245364432897) | `/test pull-knative-serving-istio-stable-no-mesh`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/9837). [Your PR dashboard](https://gubernator.knative.dev/pr/julz).

<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 -->
julz: /retest
knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/9837#" title="Author self-approved">julz</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **markusthoemmes** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @markusthoemmes` 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%2Fserving).

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

- **[OWNERS](https://github.com/knative/serving/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":["markusthoemmes"]} -->
coryrc: v1alpha1 was deleted in #9545 
coryrc: oh, you have a new CRD with v1alpha1 version

I guess the infra should not have been ripped out if that was a possibility.
knative-prow-robot: @julz: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-serving-istio-stable-no-mesh | 59561adf55e434e63c7b76085d88acc931752fa1 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/9837/pull-knative-serving-istio-stable-no-mesh/1316774901616480260) | `/test pull-knative-serving-istio-stable-no-mesh`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/9837). [Your PR dashboard](https://gubernator.knative.dev/pr/julz).

<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 -->
knative-prow-robot: @julz: PR needs rebase.

<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.
</details>

Patch:
diff --git a/pkg/reconciler/route/traffic/traffic.go b/pkg/reconciler/route/traffic/traffic.go
index eb5033bb4..6425675c6 100644
--- a/pkg/reconciler/route/traffic/traffic.go
+++ b/pkg/reconciler/route/traffic/traffic.go
@@ -325,13 +325,21 @@ func (cb *configBuilder) addRevisionTarget(tt *v1.TrafficTarget) error {
 	return nil
 }
 
+// valIfNil returns `val` if `ptr==nil`, or `*ptr` otherwise.
+func valIfNil(val int64, ptr *int64) int64 {
+	if ptr == nil {
+		return val
+	}
+	return *ptr
+}
+
 // This find the exact revision+tag pair and if so, just adds the percentages.
 // This expects single digit lists, so just does an O(N) search.
 func mergeIfNecessary(rts RevisionTargets, rt RevisionTarget) RevisionTargets {
 	for i := range rts {
 		if rts[i].Tag == rt.Tag && rts[i].RevisionName == rt.RevisionName &&
 			*rt.LatestRevision == *rts[i].LatestRevision {
-			rts[i].Percent = ptr.Int64(*rts[i].Percent + *rt.Percent)
+			rts[i].Percent = ptr.Int64(valIfNil(0, rts[i].Percent) + valIfNil(0, rt.Percent))
 			return rts
 		}
 	}


Test patch:
diff --git a/.github/workflows/kind-e2e.yaml b/.github/workflows/kind-e2e.yaml
index 4cc352d7b..bbbfa8115 100644
--- a/.github/workflows/kind-e2e.yaml
+++ b/.github/workflows/kind-e2e.yaml
@@ -25,12 +25,13 @@ jobs:
         test-suite:
         - ./test/conformance/runtime
         - ./test/conformance/api/v1
+        - ./test/conformance/api/v1alpha1
         - ./test/e2e
 
-        # Map between K8s and KinD versions.
-        # This is attempting to make it a bit clearer what's being tested.
-        # See: https://github.com/kubernetes-sigs/kind/releases/tag/v0.9.0
         include:
+          # Map between K8s and KinD versions.
+          # This is attempting to make it a bit clearer what's being tested.
+          # See: https://github.com/kubernetes-sigs/kind/releases/tag/v0.9.0
         - k8s-version: v1.17.11
           kind-version: v0.9.0
           kind-image-sha: sha256:5240a7a2c34bf241afb54ac05669f8a46661912eab05705d660971eeb12f6555
@@ -44,6 +45,11 @@ jobs:
           kind-image-sha: sha256:98cf5288864662e37115e362b23e4369c8c4a408f99cbc06e58ac30ddc721600
           kingress: contour
 
+          # The tests in api/v1alpha1 require the --enable-alpha flag to be set,
+          # and alpha components to be deployed.
+        - test-suite: ./test/conformance/api/v1alpha1
+          test-flags: "--enable-alpha"
+
     env:
       GOPATH: ${{ github.workspace }}
       GO111MODULE: off
@@ -155,6 +161,9 @@ jobs:
         sudo echo "127.0.0.1 $REGISTRY_NAME" | sudo tee -a /etc/hosts
 
     - name: Install Knative Serving
+      env:
+        GO111MODULE: on
+        GOFLAGS: -mod=vendor
       run: |
         set -o pipefail
 
@@ -162,8 +171,6 @@ jobs:
         kubectl apply -f config/core/300-imagecache.yaml
 
         # Build and Publish our containers to the docker daemon (including test assets)
-        export GO111MODULE=on
-        export GOFLAGS=-mod=vendor
         ko apply --platform=all -Pf test/config/
         ko apply --platform=all -PRf config/core
 
@@ -176,6 +183,14 @@ jobs:
         # Be KinD to these tests.
         kubectl scale -nknative-serving deployment/chaosduck --replicas=0
 
+    - name: Install Alpha Components
+      if: contains(matrix.test-flags, '--enable-alpha')
+      env:
+        GO111MODULE: on
+        GOFLAGS: -mod=vendor
+      run: |
+        ko apply --platform=all -PRf config/domain-mapping
+
     - name: Wait for Webhook to be up
       run: |
         # We need the webhook to be up
@@ -251,4 +266,5 @@ jobs:
 
         # Run the tests tagged as e2e on the KinD cluster.
         go test -race -count=1 -timeout=20m -tags=e2e ${{ matrix.test-suite }} \
-           --ingressendpoint="${IPS[0]}"
+           --ingressendpoint="${IPS[0]}" \
+           ${{ matrix.test-flags }}
diff --git a/pkg/reconciler/route/traffic/traffic_test.go b/pkg/reconciler/route/traffic/traffic_test.go
index 0ad298c1b..50d378a15 100644
--- a/pkg/reconciler/route/traffic/traffic_test.go
+++ b/pkg/reconciler/route/traffic/traffic_test.go
@@ -389,13 +389,39 @@ func TestBuildTrafficConfigurationTwoEntriesSameConfig(t *testing.T) {
 			niceNewRev.Name: niceNewRev,
 		},
 	}
-	if tc, err := BuildTrafficConfiguration(configLister, revLister, testRouteWithTrafficTargets(WithSpecTraffic(v1.TrafficTarget{
-		ConfigurationName: niceConfig.Name,
-		Percent:           ptr.Int64(90),
-	}, v1.TrafficTarget{
-		ConfigurationName: niceConfig.Name,
-		Percent:           ptr.Int64(10),
-	}))); err != nil {
+
+	// Test 42%/42%/16% split.
+	tc, err := BuildTrafficConfiguration(
+		configLister, revLister, testRouteWithTrafficTargets(
+			WithSpecTraffic([]v1.TrafficTarget{{
+				ConfigurationName: niceConfig.Name,
+				Percent:           ptr.Int64(42),
+			}, {
+				ConfigurationName: niceConfig.Name,
+				Percent:           ptr.Int64(16),
+			}, {
+				ConfigurationName: niceConfig.Name,
+				Percent:           ptr.Int64(42),
+			}}...)))
+	if err != nil {
+		t.Error("Unexpected error", err)
+	} else if got, want := tc, expected; !cmp.Equal(want, got, cmpOpts...) {
+		fmt.Println(spew.Sdump(got))
+		t.Error("Unexpected traffic diff (-want +got):", cmp.Diff(want, got, cmpOpts...))
+	}
+
+	// Test 100%/nil case.
+	tc, err = BuildTrafficConfiguration(
+		configLister, revLister,
+		testRouteWithTrafficTargets(WithSpecTraffic([]v1.TrafficTarget{{
+			ConfigurationName: niceConfig.Name,
+			Percent:           ptr.Int64(100),
+		}, {
+			ConfigurationName: niceConfig.Name,
+		}, {
+			ConfigurationName: niceConfig.Name, // let's throw in 2 nils! ;-)
+		}}...)))
+	if err != nil {
 		t.Error("Unexpected error", err)
 	} else if got, want := tc, expected; !cmp.Equal(want, got, cmpOpts...) {
 		fmt.Println(spew.Sdump(got))
diff --git a/test/conformance/api/v1/service_test.go b/test/conformance/api/v1/service_test.go
index e2803f9c4..718ba3aeb 100644
--- a/test/conformance/api/v1/service_test.go
+++ b/test/conformance/api/v1/service_test.go
@@ -31,6 +31,7 @@ import (
 	"knative.dev/serving/test"
 	v1test "knative.dev/serving/test/v1"
 
+	corev1 "k8s.io/api/core/v1"
 	rtesting "knative.dev/serving/pkg/testing/v1"
 )
 
@@ -581,3 +582,51 @@ func TestAnnotationPropagation(t *testing.T) {
 		t.Error("Route still has `juicy` annotation")
 	}
 }
+
+// TestServiceCreateWithMultipleContainers tests both Creation paths for a service.
+// The test performs a series of Validate steps to ensure that the service transitions as expected during each step.
+func TestServiceCreateWithMultipleContainers(t *testing.T) {
+	if !test.ServingFlags.EnableAlphaFeatures {
+		t.Skip()
+	}
+	t.Parallel()
+	clients := test.Setup(t)
+
+	names := test.ResourceNames{
+		Service: test.ObjectNameForTest(t),
+	}
+
+	// Clean up on test failure or interrupt
+	test.EnsureTearDown(t, clients, &names)
+	// images are used to validate digest in validateControlPlane function
+	images := map[string]string{
+		"serving-container": test.ServingContainer,
+		"sidecar-container": test.SidecarContainer,
+	}
+	containers := []corev1.Container{{
+		Name:  "serving-container",
+		Image: pkgTest.ImagePath(test.ServingContainer),
+		Ports: []corev1.ContainerPort{{
+			ContainerPort: 8881,
+		}},
+	}, {
+		Name:  "sidecar-container",
+		Image: pkgTest.ImagePath(test.SidecarContainer),
+	}}
+
+	// Setup initial Service
+	if _, err := v1test.CreateServiceReadyForMultiContainer(t, clients, &names, func(svc *v1.Service) {
+		svc.Spec.Template.Spec.Containers = containers
+	}); err != nil {
+		t.Fatalf("Failed to create initial Service %v: %v", names.Service, err)
+	}
+
+	// Validate State after Creation
+	if err := validateControlPlane(t, clients, names, "1" /*1 is the expected generation value*/, images); err != nil {
+		t.Error(err)
+	}
+
+	if err := validateDataPlane(t, clients, names, test.MultiContainerResponse); err != nil {
+		t.Error(err)
+	}
+}
diff --git a/test/conformance/api/v1/util.go b/test/conformance/api/v1/util.go
index 07d48e189..a622569da 100644
--- a/test/conformance/api/v1/util.go
+++ b/test/conformance/api/v1/util.go
@@ -121,14 +121,28 @@ func validateDataPlane(t pkgTest.TLegacy, clients *test.Clients, names test.Reso
 // Validates the state of Configuration, Revision, and Route objects for a runLatest Service.
 // The checks in this method should be able to be performed at any point in a
 // runLatest Service's lifecycle so long as the service is in a "Ready" state.
-func validateControlPlane(t *testing.T, clients *test.Clients, names test.ResourceNames, expectedGeneration string) error {
+func validateControlPlane(t *testing.T, clients *test.Clients, names test.ResourceNames, expectedGeneration string, imagesForMultipleContainers ...map[string]string) error {
 	t.Log("Checking to ensure Revision is in desired state with", "generation", expectedGeneration)
 	err := v1test.CheckRevisionState(clients.ServingClient, names.Revision, func(r *v1.Revision) (bool, error) {
 		if ready, err := v1test.IsRevisionReady(r); !ready {
 			return false, fmt.Errorf("revision %s did not become ready to serve traffic: %w", names.Revision, err)
 		}
-		if validDigest, err := shared.ValidateImageDigest(t, names.Image, r.Status.DeprecatedImageDigest); !validDigest {
-			return false, fmt.Errorf("imageDigest %s is not valid for imageName %s: %w", r.Status.DeprecatedImageDigest, names.Image, err)
+		// for multi containers there will be more than one container and names.Image is a string,
+		// so in order to validate imageDigest for each images validateControlPlane accepting optional field called imagesForMultipleContainers
+		if names.Image == "" {
+			for _, value := range imagesForMultipleContainers {
+				for _, v := range r.Status.ContainerStatuses {
+					if image, ok := value[v.Name]; ok {
+						if validDigest, err := shared.ValidateImageDigest(t, image, v.ImageDigest); !validDigest {
+							return false, fmt.Errorf("imageDigest %s is not valid for imageName %s: %w", v.ImageDigest, image, err)
+						}
+					}
+				}
+			}
+		} else {
+			if validDigest, err := shared.ValidateImageDigest(t, names.Image, r.Status.ContainerStatuses[0].ImageDigest); !validDigest {
+				return false, fmt.Errorf("imageDigest %s is not valid for imageName %s: %w", r.Status.ContainerStatuses[0].ImageDigest, names.Image, err)
+			}
 		}
 		return true, nil
 	})
diff --git a/test/conformance/api/v1alpha1/placeholder.go b/test/conformance/api/v1alpha1/placeholder.go
new file mode 100644
index 000000000..5ad859c55
--- /dev/null
+++ b/test/conformance/api/v1alpha1/placeholder.go
@@ -0,0 +1,18 @@
+/*
+Copyright 2020 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 v1alpha1 is a placeholder until we add some actual tests here.
+package v1alpha1
diff --git a/test/e2e/autoscale.go b/test/e2e/autoscale.go
index 00ea347dc..4aa65d51d 100644
--- a/test/e2e/autoscale.go
+++ b/test/e2e/autoscale.go
@@ -158,7 +158,7 @@ func generateTraffic(
 			totalRequests++
 			if res.Code != http.StatusOK {
 				ctx.t.Logf("Status = %d, want: 200", res.Code)
-				ctx.t.Logf("URL: %s Duration: %v Body:\n%s", res.URL, res.Latency, string(res.Body))
+				ctx.t.Logf("URL: %s Duration: %v Error: %s Body:\n%s", res.URL, res.Latency, res.Error, string(res.Body))
 				continue
 			}
 			successfulRequests++