Repository: knative/serving
Pull request #10633 (2021-01-26 21:56:01)

Deprecate the service name on the revision.
So far there is going to be no changes to it, besides internal name.
But after 0.23 cuts we can stop populating and then it'll be gone.

/assign @dprotaso @tcnghia 

For #10540


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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10633#" title="Author self-approved">vagababov</a>*

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

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:

- ~~[pkg/apis/OWNERS](https://github.com/knative/serving/blob/master/pkg/apis/OWNERS)~~ [vagababov]
- ~~[pkg/reconciler/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/OWNERS)~~ [vagababov]
- ~~[pkg/testing/OWNERS](https://github.com/knative/serving/blob/master/pkg/testing/OWNERS)~~ [vagababov]
- ~~[test/OWNERS](https://github.com/knative/serving/blob/master/test/OWNERS)~~ [vagababov]

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":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/10633?src=pr&el=h1) Report
> Merging [#10633](https://codecov.io/gh/knative/serving/pull/10633?src=pr&el=desc) (13cc42b) into [master](https://codecov.io/gh/knative/serving/commit/0aa3d0bfad0668e98c40d68b9c6b66d18a8e3300?el=desc) (0aa3d0b) will **decrease** coverage by `0.02%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master   #10633      +/-   ##
==========================================
- Coverage   88.16%   88.14%   -0.03%     
==========================================
  Files         186      186              
  Lines        8891     8891              
==========================================
- Hits         7839     7837       -2     
- Misses        812      813       +1     
- Partials      240      241       +1     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10633?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/apis/serving/v1/revision\_types.go](https://codecov.io/gh/knative/serving/pull/10633/diff?src=pr&el=tree#diff-cGtnL2FwaXMvc2VydmluZy92MS9yZXZpc2lvbl90eXBlcy5nbw==) | `100.00% <ø> (ø)` | |
| [pkg/reconciler/revision/reconcile\_resources.go](https://codecov.io/gh/knative/serving/pull/10633/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcmV2aXNpb24vcmVjb25jaWxlX3Jlc291cmNlcy5nbw==) | `80.95% <100.00%> (ø)` | |
| [pkg/reconciler/route/traffic/traffic.go](https://codecov.io/gh/knative/serving/pull/10633/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy90cmFmZmljLmdv) | `93.06% <100.00%> (ø)` | |
| [pkg/autoscaler/statforwarder/leases.go](https://codecov.io/gh/knative/serving/pull/10633/diff?src=pr&el=tree#diff-cGtnL2F1dG9zY2FsZXIvc3RhdGZvcndhcmRlci9sZWFzZXMuZ28=) | `76.97% <0.00%> (-1.44%)` | :arrow_down: |
| [pkg/reconciler/autoscaling/kpa/scaler.go](https://codecov.io/gh/knative/serving/pull/10633/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvYXV0b3NjYWxpbmcva3BhL3NjYWxlci5nbw==) | `88.57% <0.00%> (-1.43%)` | :arrow_down: |
| [pkg/reconciler/configuration/configuration.go](https://codecov.io/gh/knative/serving/pull/10633/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvY29uZmlndXJhdGlvbi9jb25maWd1cmF0aW9uLmdv) | `88.28% <0.00%> (+1.56%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/10633?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/10633?src=pr&el=footer). Last update [0aa3d0b...13cc42b](https://codecov.io/gh/knative/serving/pull/10633?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10633#pullrequestreview-576863182" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/10633#" title="Author self-approved">vagababov</a>*

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

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:

- ~~[pkg/apis/OWNERS](https://github.com/knative/serving/blob/master/pkg/apis/OWNERS)~~ [tcnghia,vagababov]
- ~~[pkg/reconciler/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/OWNERS)~~ [tcnghia,vagababov]
- ~~[pkg/testing/OWNERS](https://github.com/knative/serving/blob/master/pkg/testing/OWNERS)~~ [tcnghia,vagababov]
- ~~[test/OWNERS](https://github.com/knative/serving/blob/master/test/OWNERS)~~ [tcnghia,vagababov]

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":[]} -->

Patch:
diff --git a/pkg/apis/serving/v1/revision_types.go b/pkg/apis/serving/v1/revision_types.go
index 4304a0770..d3d5ab5b4 100644
--- a/pkg/apis/serving/v1/revision_types.go
+++ b/pkg/apis/serving/v1/revision_types.go
@@ -122,10 +122,14 @@ func IsRevisionCondition(t apis.ConditionType) bool {
 type RevisionStatus struct {
 	duckv1.Status `json:",inline"`
 
-	// ServiceName holds the name of a core Kubernetes Service resource that
+	// DeprecatedServiceName holds the name of a core Kubernetes Service resource that
 	// load balances over the pods backing this Revision.
+	// DEPRECATED: revision service name is effectively equal to the revision name,
+	// as per #10540.
+	// 0.23 — stop populating
+	// 0.25 — remove.
 	// +optional
-	ServiceName string `json:"serviceName,omitempty"`
+	DeprecatedServiceName string `json:"serviceName,omitempty"`
 
 	// LogURL specifies the generated logging url for this particular revision
 	// based on the revision url template specified in the controller's config.
diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go
index 8f7e1ed2e..233d8b75e 100644
--- a/pkg/reconciler/revision/reconcile_resources.go
+++ b/pkg/reconciler/revision/reconcile_resources.go
@@ -174,7 +174,7 @@ func (c *Reconciler) reconcilePA(ctx context.Context, rev *v1.Revision) error {
 
 	// The public service name is always equal to the revision name itself.
 	// Historically it's been acquired from the PA object, so the assignment is here.
-	rev.Status.ServiceName = rev.Name
+	rev.Status.DeprecatedServiceName = rev.Name
 
 	logger.Debugf("Observed PA Status=%#v", pa.Status)
 	rev.Status.PropagateAutoscalerStatus(&pa.Status)
diff --git a/pkg/reconciler/route/traffic/traffic.go b/pkg/reconciler/route/traffic/traffic.go
index 463ff4bf0..c9596e117 100644
--- a/pkg/reconciler/route/traffic/traffic.go
+++ b/pkg/reconciler/route/traffic/traffic.go
@@ -364,7 +364,7 @@ func (cb *configBuilder) addConfigurationTarget(tt *v1.TrafficTarget) error {
 	target := RevisionTarget{
 		TrafficTarget: *ntt,
 		Protocol:      rev.GetProtocol(),
-		ServiceName:   rev.Status.ServiceName,
+		ServiceName:   rev.Status.DeprecatedServiceName,
 	}
 	target.TrafficTarget.RevisionName = rev.Name
 	cb.addFlattenedTarget(target)
@@ -383,7 +383,7 @@ func (cb *configBuilder) addRevisionTarget(tt *v1.TrafficTarget) error {
 	target := RevisionTarget{
 		TrafficTarget: *ntt,
 		Protocol:      rev.GetProtocol(),
-		ServiceName:   rev.Status.ServiceName,
+		ServiceName:   rev.Status.DeprecatedServiceName,
 	}
 	if configName, ok := rev.Labels[serving.ConfigurationLabelKey]; ok {
 		target.TrafficTarget.ConfigurationName = configName


Test patch:
diff --git a/pkg/reconciler/route/route_test.go b/pkg/reconciler/route/route_test.go
index 25e7b7d43..566b5de21 100644
--- a/pkg/reconciler/route/route_test.go
+++ b/pkg/reconciler/route/route_test.go
@@ -225,7 +225,7 @@ func TestCreateRouteForOneReserveRevision(t *testing.T) {
 					Splits: []v1alpha1.IngressBackendSplit{{
 						IngressBackend: v1alpha1.IngressBackend{
 							ServiceNamespace: testNamespace,
-							ServiceName:      rev.Status.ServiceName,
+							ServiceName:      rev.Status.DeprecatedServiceName,
 							ServicePort:      intstr.FromInt(80),
 						},
 						Percent: 100,
@@ -246,7 +246,7 @@ func TestCreateRouteForOneReserveRevision(t *testing.T) {
 					Splits: []v1alpha1.IngressBackendSplit{{
 						IngressBackend: v1alpha1.IngressBackend{
 							ServiceNamespace: testNamespace,
-							ServiceName:      rev.Status.ServiceName,
+							ServiceName:      rev.Status.DeprecatedServiceName,
 							ServicePort:      intstr.FromInt(80),
 						},
 						Percent: 100,
@@ -353,7 +353,7 @@ func TestCreateRouteWithMultipleTargets(t *testing.T) {
 					Splits: []v1alpha1.IngressBackendSplit{{
 						IngressBackend: v1alpha1.IngressBackend{
 							ServiceNamespace: testNamespace,
-							ServiceName:      cfgrev.Status.ServiceName,
+							ServiceName:      cfgrev.Status.DeprecatedServiceName,
 							ServicePort:      intstr.FromInt(80),
 						},
 						Percent: 90,
@@ -364,7 +364,7 @@ func TestCreateRouteWithMultipleTargets(t *testing.T) {
 					}, {
 						IngressBackend: v1alpha1.IngressBackend{
 							ServiceNamespace: testNamespace,
-							ServiceName:      rev.Status.ServiceName,
+							ServiceName:      rev.Status.DeprecatedServiceName,
 							ServicePort:      intstr.FromInt(80),
 						},
 						Percent: 10,
@@ -385,7 +385,7 @@ func TestCreateRouteWithMultipleTargets(t *testing.T) {
 					Splits: []v1alpha1.IngressBackendSplit{{
 						IngressBackend: v1alpha1.IngressBackend{
 							ServiceNamespace: testNamespace,
-							ServiceName:      cfgrev.Status.ServiceName,
+							ServiceName:      cfgrev.Status.DeprecatedServiceName,
 							ServicePort:      intstr.FromInt(80),
 						},
 						Percent: 90,
@@ -396,7 +396,7 @@ func TestCreateRouteWithMultipleTargets(t *testing.T) {
 					}, {
 						IngressBackend: v1alpha1.IngressBackend{
 							ServiceNamespace: testNamespace,
-							ServiceName:      rev.Status.ServiceName,
+							ServiceName:      rev.Status.DeprecatedServiceName,
 							ServicePort:      intstr.FromInt(80),
 						},
 						Percent: 10,
@@ -470,7 +470,7 @@ func TestCreateRouteWithOneTargetReserve(t *testing.T) {
 					Splits: []v1alpha1.IngressBackendSplit{{
 						IngressBackend: v1alpha1.IngressBackend{
 							ServiceNamespace: testNamespace,
-							ServiceName:      cfgrev.Status.ServiceName,
+							ServiceName:      cfgrev.Status.DeprecatedServiceName,
 							ServicePort:      intstr.FromInt(80),
 						},
 						Percent: 90,
@@ -481,7 +481,7 @@ func TestCreateRouteWithOneTargetReserve(t *testing.T) {
 					}, {
 						IngressBackend: v1alpha1.IngressBackend{
 							ServiceNamespace: testNamespace,
-							ServiceName:      rev.Status.ServiceName,
+							ServiceName:      rev.Status.DeprecatedServiceName,
 							ServicePort:      intstr.FromInt(80),
 						},
 						Percent: 10,
@@ -502,7 +502,7 @@ func TestCreateRouteWithOneTargetReserve(t *testing.T) {
 					Splits: []v1alpha1.IngressBackendSplit{{
 						IngressBackend: v1alpha1.IngressBackend{
 							ServiceNamespace: testNamespace,
-							ServiceName:      cfgrev.Status.ServiceName,
+							ServiceName:      cfgrev.Status.DeprecatedServiceName,
 							ServicePort:      intstr.FromInt(80),
 						},
 						Percent: 90,
@@ -513,7 +513,7 @@ func TestCreateRouteWithOneTargetReserve(t *testing.T) {
 					}, {
 						IngressBackend: v1alpha1.IngressBackend{
 							ServiceNamespace: testNamespace,
-							ServiceName:      rev.Status.ServiceName,
+							ServiceName:      rev.Status.DeprecatedServiceName,
 							ServicePort:      intstr.FromInt(80),
 						},
 						Percent: 10,
diff --git a/pkg/testing/v1/revision.go b/pkg/testing/v1/revision.go
index 5db2740c1..56af1c695 100644
--- a/pkg/testing/v1/revision.go
+++ b/pkg/testing/v1/revision.go
@@ -52,7 +52,7 @@ func WithRevName(name string) RevisionOption {
 // WithServiceName propagates the given service name to the revision status.
 func WithServiceName(sn string) RevisionOption {
 	return func(rev *v1.Revision) {
-		rev.Status.ServiceName = sn
+		rev.Status.DeprecatedServiceName = sn
 	}
 }
 
@@ -139,7 +139,7 @@ func MarkActive(r *v1.Revision) {
 
 // WithK8sServiceName applies sn to the revision status field.
 func WithK8sServiceName(r *v1.Revision) {
-	r.Status.ServiceName = r.Name
+	r.Status.DeprecatedServiceName = r.Name
 }
 
 // MarkInactive calls .Status.MarkInactive on the Revision.
diff --git a/test/e2e/autoscale_test.go b/test/e2e/autoscale_test.go
index 0ee16e114..d61fe166c 100644
--- a/test/e2e/autoscale_test.go
+++ b/test/e2e/autoscale_test.go
@@ -153,7 +153,8 @@ func TestTargetBurstCapacity(t *testing.T) {
 	// uniformness with one above.
 	if err := wait.Poll(250*time.Millisecond, 2*cfg.StableWindow, func() (bool, error) {
 		svcEps, err := ctx.clients.KubeClient.CoreV1().Endpoints(test.ServingNamespace).Get(
-			context.Background(), ctx.resources.Revision.Status.ServiceName, metav1.GetOptions{})
+			context.Background(), ctx.resources.Revision.Name, /* revision service name is equal to revision name*/
+			metav1.GetOptions{})
 		if err != nil {
 			return false, err
 		}
diff --git a/test/e2e/e2e.go b/test/e2e/e2e.go
index 02572a22b..6450e1165 100644
--- a/test/e2e/e2e.go
+++ b/test/e2e/e2e.go
@@ -131,7 +131,7 @@ func waitForActivatorEndpoints(ctx *TestContext) error {
 		}
 
 		svcEps, err := ctx.clients.KubeClient.CoreV1().Endpoints(test.ServingNamespace).Get(
-			context.Background(), ctx.resources.Revision.Status.ServiceName, metav1.GetOptions{})
+			context.Background(), ctx.resources.Revision.Status.DeprecatedServiceName, metav1.GetOptions{})
 		if err != nil {
 			return false, err
 		}