Repository: knative/serving
Pull request #11746 (2021-07-29 22:22:52)

Allow setting priorityClassName in pod spec
Fixes https://github.com/knative/serving/issues/3587

<!-- Please include the 'why' behind your changes if no issue exists -->
## Proposed Changes

* allow setting priorityClassName in podSpec
* control this behavior with a feature flag.

**Release Note**

<!-- Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE". -->

```release-note
A feature flag is available to enable priorityClassName for Knative Services. See config-features for details.

```


knative-prow-robot: Hi @nealhu. Thanks for your PR.

I'm waiting for a [knative](https://github.com/orgs/knative/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://github.com/orgs/knative/people) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=knative%2Fserving).

<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>

knative-prow-robot: Welcome @nealhu! It looks like this is your first PR to knative/serving 🎉
knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

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

- **[config/OWNERS](https://github.com/knative/serving/blob/main/config/OWNERS)**
- **[pkg/apis/OWNERS](https://github.com/knative/serving/blob/main/pkg/apis/OWNERS)**
- **[pkg/reconciler/route/OWNERS](https://github.com/knative/serving/blob/main/pkg/reconciler/route/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":["zhiminxiang"]} -->

Patch:
diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md
index 7254b8553..34b1db2e2 100644
--- a/DEVELOPMENT.md
+++ b/DEVELOPMENT.md
@@ -213,7 +213,7 @@ to pick up an alternative Ingress solution and install it.
 
 ## Iterating
 
-As you make changes to the code-base, there are two special cases to be aware
+As you make changes to the code-base, there are several special cases to be aware
 of:
 
 - **If you change an input to generated code**, then you must run
@@ -230,14 +230,22 @@ of:
 - **If you change a package's deps** (including adding an external dependency),
   then you must run [`./hack/update-deps.sh`](./hack/update-deps.sh).
 
-These are both idempotent, and we expect that running these at `HEAD` to have no
+- **If you change surface area of `PodSpec` that we allow in our resources** then you must update
+  the relevant section of [`./hack/schemapatch-config.yaml`](./hack/schemapatch-config.yaml)
+  and run [`./hack/update-schemas.sh`](./hack/update-schemas.sh) Additionally:
+  - If the new field is added _without feature-gating_, then it must be added to the
+    `allowedFields` list.
+  - If the new field is added _behind a feature flag_, then set `preserveUnknownFields:
+    true # for feature flagged fields` on its parent type. Do **not** add it to `allowedFields`.
+
+These are all idempotent, and we expect that running these at `HEAD` to have no
 diffs. Code generation and dependencies are automatically checked to produce no
 diffs for each pull request.
 
-update-deps.sh runs go get/mod command. In some cases, if newer dependencies are
+[`update-deps.sh`](./hack/update-deps.sh) runs go get/mod command. In some cases, if newer dependencies are
 required, you need to run "go get" manually.
 
-Once the codegen and dependency information is correct, redeploying the
+Once the codegen, dependency, and schema information is correct, redeploying the
 controller is simply:
 
 ```shell
diff --git a/config/core/configmaps/features.yaml b/config/core/configmaps/features.yaml
index a25334e54..f038b7fa4 100644
--- a/config/core/configmaps/features.yaml
+++ b/config/core/configmaps/features.yaml
@@ -22,7 +22,7 @@ metadata:
     app.kubernetes.io/version: devel
     app.kubernetes.io/part-of: knative-serving
   annotations:
-    knative.dev/example-checksum: "3bac317a"
+    knative.dev/example-checksum: "211d4000"
 data:
   _example: |-
     ################################
@@ -107,6 +107,12 @@ data:
     # See: https://knative.dev/docs/serving/feature-flags/#kubernetes-security-context
     kubernetes.podspec-securitycontext: "disabled"
 
+    # Indicates whether Kubernetes PriorityClassName support is enabled
+    #
+    # WARNING: Cannot safely be disabled once enabled.
+    # See: https://knative.dev/docs/serving/feature-flags/#kubernetes-priority-class-name
+    kubernetes.podspec-priorityclassname: "disabled"
+
     # This feature flag allows end-users to add a subset of capabilities on the Pod's SecurityContext.
     #
     # When set to "enabled" or "allowed" it allows capabilities to be added to the container.
diff --git a/pkg/apis/config/features.go b/pkg/apis/config/features.go
index bcc15d9e1..cfdda32de 100644
--- a/pkg/apis/config/features.go
+++ b/pkg/apis/config/features.go
@@ -49,6 +49,7 @@ func defaultFeaturesConfig() *Features {
 		PodSpecNodeSelector:          Disabled,
 		PodSpecRuntimeClassName:      Disabled,
 		PodSpecSecurityContext:       Disabled,
+		PodSpecPriorityClassName:     Disabled,
 		ContainerSpecAddCapabilities: Disabled,
 		PodSpecTolerations:           Disabled,
 		PodSpecVolumesEmptyDir:       Disabled,
@@ -70,6 +71,7 @@ func NewFeaturesConfigFromMap(data map[string]string) (*Features, error) {
 		asFlag("kubernetes.podspec-nodeselector", &nc.PodSpecNodeSelector),
 		asFlag("kubernetes.podspec-runtimeclassname", &nc.PodSpecRuntimeClassName),
 		asFlag("kubernetes.podspec-securitycontext", &nc.PodSpecSecurityContext),
+		asFlag("kubernetes.podspec-priorityclassname", &nc.PodSpecPriorityClassName),
 		asFlag("kubernetes.containerspec-addcapabilities", &nc.ContainerSpecAddCapabilities),
 		asFlag("kubernetes.podspec-tolerations", &nc.PodSpecTolerations),
 		asFlag("kubernetes.podspec-volumes-emptydir", &nc.PodSpecVolumesEmptyDir),
@@ -95,6 +97,7 @@ type Features struct {
 	PodSpecNodeSelector          Flag
 	PodSpecRuntimeClassName      Flag
 	PodSpecSecurityContext       Flag
+	PodSpecPriorityClassName     Flag
 	ContainerSpecAddCapabilities Flag
 	PodSpecTolerations           Flag
 	PodSpecVolumesEmptyDir       Flag
diff --git a/pkg/apis/serving/fieldmask.go b/pkg/apis/serving/fieldmask.go
index 991d8a1e7..983e1c64a 100644
--- a/pkg/apis/serving/fieldmask.go
+++ b/pkg/apis/serving/fieldmask.go
@@ -207,6 +207,9 @@ func PodSpecMask(ctx context.Context, in *corev1.PodSpec) *corev1.PodSpec {
 	if cfg.Features.PodSpecSecurityContext != config.Disabled {
 		out.SecurityContext = in.SecurityContext
 	}
+	if cfg.Features.PodSpecPriorityClassName != config.Disabled {
+		out.PriorityClassName = in.PriorityClassName
+	}
 
 	// Disallowed fields
 	// This list is unnecessary, but added here for clarity
@@ -223,7 +226,6 @@ func PodSpecMask(ctx context.Context, in *corev1.PodSpec) *corev1.PodSpec {
 	out.Hostname = ""
 	out.Subdomain = ""
 	out.SchedulerName = ""
-	out.PriorityClassName = ""
 	out.Priority = nil
 	out.DNSConfig = nil
 	out.ReadinessGates = nil


Test patch:
diff --git a/pkg/apis/config/features_test.go b/pkg/apis/config/features_test.go
index cc1f72050..9db3d93b0 100644
--- a/pkg/apis/config/features_test.go
+++ b/pkg/apis/config/features_test.go
@@ -59,26 +59,28 @@ func TestFeaturesConfiguration(t *testing.T) {
 		name:    "features Enabled",
 		wantErr: false,
 		wantFeatures: defaultWith(&Features{
-			MultiContainer:          Enabled,
-			PodSpecAffinity:         Enabled,
-			PodSpecDryRun:           Enabled,
-			PodSpecHostAliases:      Enabled,
-			PodSpecNodeSelector:     Enabled,
-			PodSpecRuntimeClassName: Enabled,
-			PodSpecSecurityContext:  Enabled,
-			PodSpecTolerations:      Enabled,
-			TagHeaderBasedRouting:   Enabled,
+			MultiContainer:           Enabled,
+			PodSpecAffinity:          Enabled,
+			PodSpecDryRun:            Enabled,
+			PodSpecHostAliases:       Enabled,
+			PodSpecNodeSelector:      Enabled,
+			PodSpecRuntimeClassName:  Enabled,
+			PodSpecSecurityContext:   Enabled,
+			PodSpecTolerations:       Enabled,
+			PodSpecPriorityClassName: Enabled,
+			TagHeaderBasedRouting:    Enabled,
 		}),
 		data: map[string]string{
-			"multi-container":                     "Enabled",
-			"kubernetes.podspec-affinity":         "Enabled",
-			"kubernetes.podspec-dryrun":           "Enabled",
-			"kubernetes.podspec-hostaliases":      "Enabled",
-			"kubernetes.podspec-nodeselector":     "Enabled",
-			"kubernetes.podspec-runtimeclassname": "Enabled",
-			"kubernetes.podspec-securitycontext":  "Enabled",
-			"kubernetes.podspec-tolerations":      "Enabled",
-			"tag-header-based-routing":            "Enabled",
+			"multi-container":                      "Enabled",
+			"kubernetes.podspec-affinity":          "Enabled",
+			"kubernetes.podspec-dryrun":            "Enabled",
+			"kubernetes.podspec-hostaliases":       "Enabled",
+			"kubernetes.podspec-nodeselector":      "Enabled",
+			"kubernetes.podspec-runtimeclassname":  "Enabled",
+			"kubernetes.podspec-securitycontext":   "Enabled",
+			"kubernetes.podspec-tolerations":       "Enabled",
+			"kubernetes.podspec-priorityclassname": "Enabled",
+			"tag-header-based-routing":             "Enabled",
 		},
 	}, {
 		name:    "multi-container Allowed",
@@ -350,6 +352,33 @@ func TestFeaturesConfiguration(t *testing.T) {
 		data: map[string]string{
 			"kubernetes.podspec-volumes-emptydir": "Enabled",
 		},
+	}, {
+		name:    "kubernetes.podspec-priorityclassname Allowed",
+		wantErr: false,
+		wantFeatures: defaultWith(&Features{
+			PodSpecPriorityClassName: Allowed,
+		}),
+		data: map[string]string{
+			"kubernetes.podspec-priorityclassname": "Allowed",
+		},
+	}, {
+		name:    "kubernetes.podspec-priorityclassname Enabled",
+		wantErr: false,
+		wantFeatures: defaultWith(&Features{
+			PodSpecPriorityClassName: Enabled,
+		}),
+		data: map[string]string{
+			"kubernetes.podspec-priorityclassname": "Enabled",
+		},
+	}, {
+		name:    "kubernetes.podspec-priorityclassname Disabled",
+		wantErr: false,
+		wantFeatures: defaultWith(&Features{
+			PodSpecPriorityClassName: Disabled,
+		}),
+		data: map[string]string{
+			"kubernetes.podspec-priorityclassname": "Disabled",
+		},
 	}}
 
 	for _, tt := range configTests {
diff --git a/pkg/apis/serving/k8s_validation_test.go b/pkg/apis/serving/k8s_validation_test.go
index 4832a565d..f50df97b8 100644
--- a/pkg/apis/serving/k8s_validation_test.go
+++ b/pkg/apis/serving/k8s_validation_test.go
@@ -103,6 +103,13 @@ func withPodSpecVolumesEmptyDirEnabled() configOption {
 	}
 }
 
+func withPodSpecPriorityClassNameEnabled() configOption {
+	return func(cfg *config.Config) *config.Config {
+		cfg.Features.PodSpecPriorityClassName = config.Enabled
+		return cfg
+	}
+}
+
 func TestPodSpecValidation(t *testing.T) {
 	tests := []struct {
 		name    string
@@ -147,6 +154,15 @@ func TestPodSpecValidation(t *testing.T) {
 			}},
 		},
 		want: nil,
+	}, {
+		name: "with automountServiceAccountToken = true",
+		ps: corev1.PodSpec{
+			AutomountServiceAccountToken: ptr.Bool(true),
+			Containers: []corev1.Container{{
+				Image: "helloworld",
+			}},
+		},
+		want: apis.ErrDisallowedFields("automountServiceAccountToken"),
 	}, {
 		name: "with volume name collision",
 		ps: corev1.PodSpec{
@@ -668,6 +684,16 @@ func TestPodSpecFeatureValidation(t *testing.T) {
 			Paths:   []string{"securityContext"},
 		},
 		cfgOpts: []configOption{withPodSpecSecurityContextEnabled()},
+	}, {
+		name: "PriorityClassName",
+		featureSpec: corev1.PodSpec{
+			PriorityClassName: "foo",
+		},
+		err: &apis.FieldError{
+			Message: "must not set the field(s)",
+			Paths:   []string{"priorityClassName"},
+		},
+		cfgOpts: []configOption{withPodSpecPriorityClassNameEnabled()},
 	}}
 
 	featureTests := []struct {
diff --git a/pkg/reconciler/route/resources/service_test.go b/pkg/reconciler/route/resources/service_test.go
index ca7c5c2da..3d97d7402 100644
--- a/pkg/reconciler/route/resources/service_test.go
+++ b/pkg/reconciler/route/resources/service_test.go
@@ -370,15 +370,16 @@ func testConfig() *config.Config {
 			TagTemplate:         network.DefaultTagTemplate,
 		},
 		Features: &apiConfig.Features{
-			MultiContainer:         apiConfig.Disabled,
-			PodSpecAffinity:        apiConfig.Disabled,
-			PodSpecFieldRef:        apiConfig.Disabled,
-			PodSpecDryRun:          apiConfig.Enabled,
-			PodSpecHostAliases:     apiConfig.Disabled,
-			PodSpecNodeSelector:    apiConfig.Disabled,
-			PodSpecTolerations:     apiConfig.Disabled,
-			PodSpecVolumesEmptyDir: apiConfig.Disabled,
-			TagHeaderBasedRouting:  apiConfig.Disabled,
+			MultiContainer:           apiConfig.Disabled,
+			PodSpecAffinity:          apiConfig.Disabled,
+			PodSpecFieldRef:          apiConfig.Disabled,
+			PodSpecDryRun:            apiConfig.Enabled,
+			PodSpecHostAliases:       apiConfig.Disabled,
+			PodSpecNodeSelector:      apiConfig.Disabled,
+			PodSpecTolerations:       apiConfig.Disabled,
+			PodSpecVolumesEmptyDir:   apiConfig.Disabled,
+			PodSpecPriorityClassName: apiConfig.Disabled,
+			TagHeaderBasedRouting:    apiConfig.Disabled,
 		},
 	}
 }
diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go
index ee754a479..0ed6834bd 100644
--- a/pkg/reconciler/route/table_test.go
+++ b/pkg/reconciler/route/table_test.go
@@ -2830,14 +2830,15 @@ func reconcilerTestConfig(enableAutoTLS bool) *config.Config {
 			DefaultExternalScheme:   "http",
 		},
 		Features: &cfgmap.Features{
-			MultiContainer:        cfgmap.Disabled,
-			PodSpecAffinity:       cfgmap.Disabled,
-			PodSpecFieldRef:       cfgmap.Disabled,
-			PodSpecDryRun:         cfgmap.Enabled,
-			PodSpecHostAliases:    cfgmap.Disabled,
-			PodSpecNodeSelector:   cfgmap.Disabled,
-			PodSpecTolerations:    cfgmap.Disabled,
-			TagHeaderBasedRouting: cfgmap.Disabled,
+			MultiContainer:           cfgmap.Disabled,
+			PodSpecAffinity:          cfgmap.Disabled,
+			PodSpecFieldRef:          cfgmap.Disabled,
+			PodSpecDryRun:            cfgmap.Enabled,
+			PodSpecHostAliases:       cfgmap.Disabled,
+			PodSpecNodeSelector:      cfgmap.Disabled,
+			PodSpecTolerations:       cfgmap.Disabled,
+			PodSpecPriorityClassName: cfgmap.Disabled,
+			TagHeaderBasedRouting:    cfgmap.Disabled,
 		},
 	}
 }
diff --git a/pkg/testing/v1/service.go b/pkg/testing/v1/service.go
index 0320ceace..e1522db45 100644
--- a/pkg/testing/v1/service.go
+++ b/pkg/testing/v1/service.go
@@ -161,6 +161,13 @@ func WithServiceImage(img string) ServiceOption {
 	}
 }
 
+// WithTrafficTarget sets the traffic to be the provided traffic target.
+func WithTrafficTarget(tt []v1.TrafficTarget) ServiceOption {
+	return func(svc *v1.Service) {
+		svc.Spec.Traffic = tt
+	}
+}
+
 // WithServiceTemplateMeta sets the container image to be the provided string.
 func WithServiceTemplateMeta(m metav1.ObjectMeta) ServiceOption {
 	return func(svc *v1.Service) {
diff --git a/test/conformance/api/v1/blue_green_test.go b/test/conformance/api/v1/blue_green_test.go
index 73a953422..04c853a61 100644
--- a/test/conformance/api/v1/blue_green_test.go
+++ b/test/conformance/api/v1/blue_green_test.go
@@ -64,7 +64,7 @@ func TestBlueGreenRoute(t *testing.T) {
 
 	// Setup Initial Service
 	t.Log("Creating a new Service in runLatest")
-	objects, err := v1test.CreateServiceReady(t, clients, &names)
+	_, err := v1test.CreateServiceReady(t, clients, &names)
 	if err != nil {
 		t.Fatalf("Failed to create initial Service: %v: %v", names.Service, err)
 	}
@@ -76,11 +76,10 @@ func TestBlueGreenRoute(t *testing.T) {
 	green.TrafficTarget = "green"
 
 	t.Log("Updating the Service to use a different image")
-	service, err := v1test.PatchService(t, clients, objects.Service, rtesting.WithServiceImage(greenImagePath))
+	_, err = v1test.UpdateService(t, clients, names, rtesting.WithServiceImage(greenImagePath))
 	if err != nil {
-		t.Fatalf("Patch update for Service %s with new image %s failed: %v", names.Service, greenImagePath, err)
+		t.Fatalf("Update for Service %s with new image %s failed: %v", names.Service, greenImagePath, err)
 	}
-	objects.Service = service
 
 	t.Log("Since the Service was updated a new Revision will be created and the Service will be updated")
 	green.Revision, err = v1test.WaitForServiceLatestRevision(clients, names)
@@ -89,8 +88,8 @@ func TestBlueGreenRoute(t *testing.T) {
 	}
 
 	t.Log("Updating RouteSpec")
-	if _, err := v1test.UpdateServiceRouteSpec(t, clients, names, v1.RouteSpec{
-		Traffic: []v1.TrafficTarget{{
+	if _, err := v1test.UpdateService(t, clients, names, rtesting.WithTrafficTarget(
+		[]v1.TrafficTarget{{
 			Tag:          blue.TrafficTarget,
 			RevisionName: blue.Revision,
 			Percent:      ptr.Int64(50),
@@ -99,7 +98,7 @@ func TestBlueGreenRoute(t *testing.T) {
 			RevisionName: green.Revision,
 			Percent:      ptr.Int64(50),
 		}},
-	}); err != nil {
+	)); err != nil {
 		t.Fatal("Failed to update Service:", err)
 	}
 
@@ -108,7 +107,7 @@ func TestBlueGreenRoute(t *testing.T) {
 		t.Fatalf("The Service %s was not marked as Ready to serve traffic: %v", names.Service, err)
 	}
 
-	service, err = clients.ServingClient.Services.Get(context.Background(), names.Service, metav1.GetOptions{})
+	service, err := clients.ServingClient.Services.Get(context.Background(), names.Service, metav1.GetOptions{})
 	if err != nil {
 		t.Fatalf("Error fetching Service %s: %v", names.Service, err)
 	}
diff --git a/test/conformance/api/v1/configuration_test.go b/test/conformance/api/v1/configuration_test.go
index c18ea4855..39e818558 100644
--- a/test/conformance/api/v1/configuration_test.go
+++ b/test/conformance/api/v1/configuration_test.go
@@ -56,8 +56,8 @@ func TestUpdateConfigurationMetadata(t *testing.T) {
 
 	t.Log("Updating labels of Configuration", names.Config)
 	newLabels := map[string]string{
-		"labelX": "abc",
-		"labelY": "def",
+		"label-x": "abc",
+		"label-y": "def",
 	}
 	// Copy over new labels.
 	cfg.Labels = kmeta.UnionMaps(cfg.Labels, newLabels)
@@ -89,8 +89,8 @@ func TestUpdateConfigurationMetadata(t *testing.T) {
 
 	t.Log("Updating annotations of Configuration", names.Config)
 	newAnnotations := map[string]string{
-		"annotationA": "123",
-		"annotationB": "456",
+		"annotation-a": "123",
+		"annotation-b": "456",
 	}
 	cfg.Annotations = kmeta.UnionMaps(cfg.Annotations, newAnnotations)
 	cfg, err = clients.ServingClient.Configs.Update(context.Background(), cfg, metav1.UpdateOptions{})
diff --git a/test/conformance/api/v1/service_test.go b/test/conformance/api/v1/service_test.go
index 19d15cc15..4bd3b35d8 100644
--- a/test/conformance/api/v1/service_test.go
+++ b/test/conformance/api/v1/service_test.go
@@ -87,8 +87,8 @@ func TestServiceCreateAndUpdate(t *testing.T) {
 	t.Log("Updating the Service to use a different image.")
 	names.Image = test.PizzaPlanet2
 	image2 := pkgtest.ImagePath(names.Image)
-	if _, err := v1test.PatchService(t, clients, objects.Service, rtesting.WithServiceImage(image2)); err != nil {
-		t.Fatalf("Patch update for Service %s with new image %s failed: %v", names.Service, image2, err)
+	if objects.Service, err = v1test.UpdateService(t, clients, names, rtesting.WithServiceImage(image2)); err != nil {
+		t.Fatalf("Update for Service %s with new image %s failed: %v", names.Service, image2, err)
 	}
 
 	t.Log("Service should reflect new revision created and ready in status.")
@@ -114,11 +114,11 @@ func TestServiceCreateAndUpdate(t *testing.T) {
 	t.Logf("Updating labels of the RevisionTemplateSpec for service %s.", names.Service)
 	metadata := metav1.ObjectMeta{
 		Labels: map[string]string{
-			"labelX": "abc",
-			"labelY": "def",
+			"label-x": "abc",
+			"label-y": "def",
 		},
 	}
-	if objects.Service, err = v1test.PatchService(t, clients, objects.Service, rtesting.WithServiceTemplateMeta(metadata)); err != nil {
+	if objects.Service, err = v1test.UpdateService(t, clients, names, rtesting.WithServiceTemplateMeta(metadata)); err != nil {
 		t.Fatalf("Service %s was not updated with labels in its RevisionTemplateSpec: %v", names.Service, err)
 	}
 
@@ -131,11 +131,11 @@ func TestServiceCreateAndUpdate(t *testing.T) {
 	t.Log("Updating annotations of RevisionTemplateSpec for service", names.Service)
 	metadata = metav1.ObjectMeta{
 		Annotations: map[string]string{
-			"annotationA": "123",
-			"annotationB": "456",
+			"annotation-a": "123",
+			"annotation-b": "456",
 		},
 	}
-	if objects.Service, err = v1test.PatchService(t, clients, objects.Service, rtesting.WithServiceTemplateMeta(metadata)); err != nil {
+	if objects.Service, err = v1test.UpdateService(t, clients, names, rtesting.WithServiceTemplateMeta(metadata)); err != nil {
 		t.Fatalf("Service %s was not updated with annotation in its RevisionTemplateSpec: %v", names.Service, err)
 	}
 
@@ -233,8 +233,8 @@ func TestServiceBYOName(t *testing.T) {
 	t.Log("Updating the Service to use a different image.")
 	names.Image = test.PizzaPlanet2
 	image2 := pkgtest.ImagePath(names.Image)
-	if _, err := v1test.PatchService(t, clients, objects.Service, rtesting.WithServiceImage(image2)); err == nil {
-		t.Fatalf("Patch update for Service %s didn't fail.", names.Service)
+	if _, err := v1test.UpdateService(t, clients, names, rtesting.WithServiceImage(image2)); err == nil {
+		t.Fatalf("Update for Service %s didn't fail.", names.Service)
 	}
 }
 
@@ -281,8 +281,8 @@ func TestServiceWithTrafficSplit(t *testing.T) {
 
 	// 1. One Revision Specified, current == latest.
 	t.Log("1. Updating Service to ReleaseType using lastCreatedRevision")
-	objects.Service, err = v1test.UpdateServiceRouteSpec(t, clients, names, v1.RouteSpec{
-		Traffic: []v1.TrafficTarget{{
+	objects.Service, err = v1test.UpdateService(t, clients, names, rtesting.WithTrafficTarget(
+		[]v1.TrafficTarget{{
 			Tag:          "current",
 			RevisionName: firstRevision,
 			Percent:      ptr.Int64(100),
@@ -290,7 +290,7 @@ func TestServiceWithTrafficSplit(t *testing.T) {
 			Tag:     "latest",
 			Percent: nil,
 		}},
-	})
+	))
 	if err != nil {
 		t.Fatal("Failed to update Service:", err)
 	}
@@ -330,8 +330,8 @@ func TestServiceWithTrafficSplit(t *testing.T) {
 
 	// 2. One Revision Specified, current != latest.
 	t.Log("2. Updating the Service Spec with a new image")
-	if objects.Service, err = v1test.PatchService(t, clients, objects.Service, rtesting.WithServiceImage(releaseImagePath2)); err != nil {
-		t.Fatalf("Patch update for Service %s with new image %s failed: %v", names.Service, releaseImagePath2, err)
+	if objects.Service, err = v1test.UpdateService(t, clients, names, rtesting.WithServiceImage(releaseImagePath2)); err != nil {
+		t.Fatalf("Update for Service %s with new image %s failed: %v", names.Service, releaseImagePath2, err)
 	}
 
 	t.Log("Since the Service was updated a new Revision will be created")
@@ -368,8 +368,8 @@ func TestServiceWithTrafficSplit(t *testing.T) {
 
 	// 3. Two Revisions Specified, 50% rollout, candidate == latest.
 	t.Log("3. Updating Service to split traffic between two revisions using Release mode")
-	objects.Service, err = v1test.UpdateServiceRouteSpec(t, clients, names, v1.RouteSpec{
-		Traffic: []v1.TrafficTarget{{
+	objects.Service, err = v1test.UpdateService(t, clients, names, rtesting.WithTrafficTarget(
+		[]v1.TrafficTarget{{
 			Tag:          "current",
 			RevisionName: firstRevision,
 			Percent:      ptr.Int64(50),
@@ -380,7 +380,7 @@ func TestServiceWithTrafficSplit(t *testing.T) {
 		}, {
 			Tag: "latest",
 		}},
-	})
+	))
 	if err != nil {
 		t.Fatal("Failed to update Service:", err)
 	}
@@ -429,8 +429,8 @@ func TestServiceWithTrafficSplit(t *testing.T) {
 
 	// 4. Two Revisions Specified, 50% rollout, candidate != latest.
 	t.Log("4. Updating the Service Spec with a new image")
-	if objects.Service, err = v1test.PatchService(t, clients, objects.Service, rtesting.WithServiceImage(releaseImagePath3)); err != nil {
-		t.Fatalf("Patch update for Service %s with new image %s failed: %v", names.Service, releaseImagePath3, err)
+	if objects.Service, err = v1test.UpdateService(t, clients, names, rtesting.WithServiceImage(releaseImagePath3)); err != nil {
+		t.Fatalf("Update for Service %s with new image %s failed: %v", names.Service, releaseImagePath3, err)
 	}
 	t.Log("Since the Service was updated a new Revision will be created")
 	if names.Revision, err = v1test.WaitForServiceLatestRevision(clients, names); err != nil {
@@ -469,8 +469,8 @@ func TestServiceWithTrafficSplit(t *testing.T) {
 	// Now update the service to use `@latest` as candidate.
 	t.Log("5. Updating Service to split traffic between two `current` and `@latest`")
 
-	objects.Service, err = v1test.UpdateServiceRouteSpec(t, clients, names, v1.RouteSpec{
-		Traffic: []v1.TrafficTarget{{
+	objects.Service, err = v1test.UpdateService(t, clients, names, rtesting.WithTrafficTarget(
+		[]v1.TrafficTarget{{
 			Tag:          "current",
 			RevisionName: firstRevision,
 			Percent:      ptr.Int64(50),
@@ -481,7 +481,7 @@ func TestServiceWithTrafficSplit(t *testing.T) {
 			Tag:     "latest",
 			Percent: nil,
 		}},
-	})
+	))
 	if err != nil {
 		t.Fatal("Failed to update Service:", err)
 	}
@@ -548,7 +548,7 @@ func TestAnnotationPropagation(t *testing.T) {
 		t.Error("Annotations are incorrect:", err)
 	}
 
-	if objects.Service, err = v1test.PatchService(t, clients, objects.Service,
+	if objects.Service, err = v1test.UpdateService(t, clients, names,
 		rtesting.WithServiceAnnotation("juicy", "jamba")); err != nil {
 		t.Fatalf("Service %s was not updated with new annotation: %v", names.Service, err)
 	}
@@ -557,8 +557,8 @@ func TestAnnotationPropagation(t *testing.T) {
 	// change, so let's generate a change that we can watch.
 	t.Log("Updating the Service to use a different image.")
 	image2 := pkgtest.ImagePath(test.PizzaPlanet2)
-	if _, err := v1test.PatchService(t, clients, objects.Service, rtesting.WithServiceImage(image2)); err != nil {
-		t.Fatalf("Patch update for Service %s with new image %s failed: %v", names.Service, image2, err)
+	if objects.Service, err = v1test.UpdateService(t, clients, names, rtesting.WithServiceImage(image2)); err != nil {
+		t.Fatalf("Update for Service %s with new image %s failed: %v", names.Service, image2, err)
 	}
 
 	t.Log("Service should reflect new revision created and ready in status.")
@@ -581,7 +581,7 @@ func TestAnnotationPropagation(t *testing.T) {
 		t.Error("Annotations are incorrect:", err)
 	}
 
-	if objects.Service, err = v1test.PatchService(t, clients, objects.Service,
+	if objects.Service, err = v1test.UpdateService(t, clients, names,
 		rtesting.WithServiceAnnotationRemoved("juicy")); err != nil {
 		t.Fatalf("Service %s was not updated with annotation deleted: %v", names.Service, err)
 	}
@@ -590,8 +590,8 @@ func TestAnnotationPropagation(t *testing.T) {
 	// change, so let's generate a change that we can watch.
 	t.Log("Updating the Service to use a different image.")
 	image3 := pkgtest.ImagePath(test.HelloWorld)
-	if _, err := v1test.PatchService(t, clients, objects.Service, rtesting.WithServiceImage(image3)); err != nil {
-		t.Fatalf("Patch update for Service %s with new image %s failed: %v", names.Service, image3, err)
+	if objects.Service, err = v1test.UpdateService(t, clients, names, rtesting.WithServiceImage(image3)); err != nil {
+		t.Fatalf("Update for Service %s with new image %s failed: %v", names.Service, image3, err)
 	}
 
 	t.Log("Service should reflect new revision created and ready in status.")
diff --git a/test/e2e/autotls/auto_tls_test.go b/test/e2e/autotls/auto_tls_test.go
index 78f84fc72..8599a5883 100644
--- a/test/e2e/autotls/auto_tls_test.go
+++ b/test/e2e/autotls/auto_tls_test.go
@@ -120,7 +120,7 @@ func testAutoTLS(t *testing.T) {
 			return transport
 		}
 
-		if _, err := v1test.UpdateServiceRouteSpec(t, clients, names, servingv1.RouteSpec{
+		if _, err := v1test.PatchServiceRouteSpec(t, clients, names, servingv1.RouteSpec{
 			Traffic: []servingv1.TrafficTarget{{
 				Tag:            "r",
 				Percent:        ptr.Int64(50),
diff --git a/test/e2e/subroutes_test.go b/test/e2e/subroutes_test.go
index e8428afa5..ac7c62dd6 100644
--- a/test/e2e/subroutes_test.go
+++ b/test/e2e/subroutes_test.go
@@ -153,7 +153,7 @@ func TestSubrouteVisibilityPublicToPrivate(t *testing.T) {
 			LatestRevision: ptr.Bool(true),
 		})
 
-	if _, err = v1test.UpdateServiceRouteSpec(t, clients, names, v1.RouteSpec{Traffic: ksvcCopyRouteTraffic}); err != nil {
+	if _, err = v1test.PatchServiceRouteSpec(t, clients, names, v1.RouteSpec{Traffic: ksvcCopyRouteTraffic}); err != nil {
 		t.Fatal("Failed to patch service:", err)
 	}
 
diff --git a/test/e2e/tagheader/tag_header_based_routing_test.go b/test/e2e/tagheader/tag_header_based_routing_test.go
index cdb8b86b9..be34a3df4 100644
--- a/test/e2e/tagheader/tag_header_based_routing_test.go
+++ b/test/e2e/tagheader/tag_header_based_routing_test.go
@@ -76,7 +76,7 @@ func TestTagHeaderBasedRouting(t *testing.T) {
 		t.Fatalf("Service %s was not updated with the Revision for image %s: %v", names.Service, test.PizzaPlanet2, err)
 	}
 
-	if _, err := v1test.UpdateServiceRouteSpec(t, clients, names, v1.RouteSpec{
+	if _, err := v1test.PatchServiceRouteSpec(t, clients, names, v1.RouteSpec{
 		Traffic: []v1.TrafficTarget{{
 			Tag:          "rev1",
 			RevisionName: revision1,
diff --git a/test/e2e/websocket_test.go b/test/e2e/websocket_test.go
index 03231fd76..b40f93863 100644
--- a/test/e2e/websocket_test.go
+++ b/test/e2e/websocket_test.go
@@ -243,7 +243,7 @@ func TestWebSocketBlueGreenRoute(t *testing.T) {
 	}
 
 	t.Log("Updating RouteSpec")
-	if _, err := v1test.UpdateServiceRouteSpec(t, clients, names, v1.RouteSpec{
+	if _, err := v1test.PatchServiceRouteSpec(t, clients, names, v1.RouteSpec{
 		Traffic: []v1.TrafficTarget{{
 			Tag:          blue.TrafficTarget,
 			RevisionName: blue.Revision,
diff --git a/test/upgrade/postupgrade.go b/test/upgrade/postupgrade.go
index 711f2fd1d..d02ed2bcd 100644
--- a/test/upgrade/postupgrade.go
+++ b/test/upgrade/postupgrade.go
@@ -108,7 +108,7 @@ func bYORevisionPostUpgrade(t *testing.T) {
 		Service: byoServiceName,
 	}
 
-	if _, err := v1test.UpdateServiceRouteSpec(t, clients, names, v1.RouteSpec{
+	if _, err := v1test.PatchServiceRouteSpec(t, clients, names, v1.RouteSpec{
 		Traffic: []v1.TrafficTarget{{
 			Tag:          "example-tag",
 			RevisionName: byoRevName,
diff --git a/test/v1/service.go b/test/v1/service.go
index f229884ba..bf96d0c80 100644
--- a/test/v1/service.go
+++ b/test/v1/service.go
@@ -147,6 +147,25 @@ func createService(t testing.TB, clients *test.Clients, service *v1.Service) (sv
 	})
 }
 
+// UpdateService updates the existing service with name in names with the applied mutations.
+// Returns the latest service object
+func UpdateService(t testing.TB, clients *test.Clients, names test.ResourceNames, fopt ...rtesting.ServiceOption) (svc *v1.Service, err error) {
+	return svc, reconciler.RetryTestErrors(func(int) (err error) {
+		newSvc, err := clients.ServingClient.Services.Get(context.Background(), names.Service, metav1.GetOptions{})
+		if err != nil {
+			return err
+		}
+
+		for _, opt := range fopt {
+			opt(newSvc)
+		}
+
+		LogResourceObject(t, ResourceObjects{Service: newSvc})
+		svc, err = clients.ServingClient.Services.Update(context.Background(), newSvc, metav1.UpdateOptions{})
+		return err
+	})
+}
+
 // PatchService patches the existing service passed in with the applied mutations.
 // Returns the latest service object
 func PatchService(t testing.TB, clients *test.Clients, service *v1.Service, fopt ...rtesting.ServiceOption) (svc *v1.Service, err error) {
@@ -165,8 +184,8 @@ func PatchService(t testing.TB, clients *test.Clients, service *v1.Service, fopt
 	})
 }
 
-// UpdateServiceRouteSpec updates a service to use the route name in names.
-func UpdateServiceRouteSpec(t testing.TB, clients *test.Clients, names test.ResourceNames, rs v1.RouteSpec) (svc *v1.Service, err error) {
+// PatchServiceRouteSpec patches a service to use the route name in names.
+func PatchServiceRouteSpec(t testing.TB, clients *test.Clients, names test.ResourceNames, rs v1.RouteSpec) (svc *v1.Service, err error) {
 	patch := []map[string]interface{}{{
 		"op":    "replace",
 		"path":  "/spec/traffic",