Repository: knative/operator
Pull request #1479 (2023-06-22 21:11:07)

Enable the support of NodeSelectors and tolerations on Jobs
Fixes: #1480

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

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

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

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/operator/blob/main/OWNERS)~~ [houshengbo]

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://app.codecov.io/gh/knative/operator/pull/1479?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
Patch coverage: **`57.14`**% and project coverage change: **`-0.09`** :warning:
> Comparison is base [(`0658995`)](https://app.codecov.io/gh/knative/operator/commit/0658995c4d3b45290897f854c0fb53aaa89c4ba6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) 79.32% compared to head [(`de2c938`)](https://app.codecov.io/gh/knative/operator/pull/1479?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) 79.23%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #1479      +/-   ##
==========================================
- Coverage   79.32%   79.23%   -0.09%     
==========================================
  Files          40       40              
  Lines        1775     1782       +7     
==========================================
+ Hits         1408     1412       +4     
- Misses        267      269       +2     
- Partials      100      101       +1     
```


| [Impacted Files](https://app.codecov.io/gh/knative/operator/pull/1479?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/common/workload\_override.go](https://app.codecov.io/gh/knative/operator/pull/1479?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL3JlY29uY2lsZXIvY29tbW9uL3dvcmtsb2FkX292ZXJyaWRlLmdv) | `85.71% <57.14%> (-1.43%)` | :arrow_down: |


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/knative/operator/pull/1479?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).

knative-prow[bot]: @houshengbo: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
serving-upgrade-tests_operator_main | 9f4b20a194c6cfffee49866b6297dee18ca74b0b | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_operator/1479/serving-upgrade-tests_operator_main/1676999480185982976) | true | `/test serving-upgrade-tests`

[Your PR dashboard](https://prow.knative.dev/pr).

<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 -->
nak3: /lgtm
/retest
knative-prow[bot]: @houshengbo: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
serving-upgrade-tests_operator_main | 9f4b20a194c6cfffee49866b6297dee18ca74b0b | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_operator/1479/serving-upgrade-tests_operator_main/1677116911327907840) | true | `/test serving-upgrade-tests`

[Your PR dashboard](https://prow.knative.dev/pr).

<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/OWNERS_ALIASES b/OWNERS_ALIASES
index 103ef9259..2c1fc43b5 100644
--- a/OWNERS_ALIASES
+++ b/OWNERS_ALIASES
@@ -125,6 +125,7 @@ aliases:
   - evankanderson
   serving-approvers:
   - nak3
+  - psschwei
   - skonto
   serving-reviewers:
   - KauzClay
@@ -140,7 +141,6 @@ aliases:
   - skonto
   serving-wg-leads:
   - dprotaso
-  - psschwei
   serving-writers:
   - dprotaso
   - nak3
diff --git a/pkg/reconciler/common/workload_override.go b/pkg/reconciler/common/workload_override.go
index 35cddf29e..c9ed1699d 100644
--- a/pkg/reconciler/common/workload_override.go
+++ b/pkg/reconciler/common/workload_override.go
@@ -20,6 +20,7 @@ import (
 	mf "github.com/manifestival/manifestival"
 	"go.uber.org/zap"
 	appsv1 "k8s.io/api/apps/v1"
+	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -61,6 +62,14 @@ func OverridesTransform(overrides []base.WorkloadOverride, log *zap.SugaredLogge
 					ss.Spec.Replicas = override.Replicas
 				}
 			}
+			if u.GetKind() == "Job" && u.GetGenerateName() == override.Name {
+				job := &batchv1.Job{}
+				if err := scheme.Scheme.Convert(u, job, nil); err != nil {
+					return err
+				}
+				obj = job
+				ps = &job.Spec.Template
+			}
 
 			if obj == nil {
 				continue


Test patch:
diff --git a/pkg/reconciler/common/testdata/manifest.yaml b/pkg/reconciler/common/testdata/manifest.yaml
index 41917dbcd..222219f67 100644
--- a/pkg/reconciler/common/testdata/manifest.yaml
+++ b/pkg/reconciler/common/testdata/manifest.yaml
@@ -3017,3 +3017,56 @@ spec:
       dnsConfig:
         options:
           - name: single-request-reopen
+---
+
+apiVersion: batch/v1
+kind: Job
+metadata:
+  generateName: storage-version-migration-serving-
+  namespace: knative-serving
+  labels:
+    app: storage-version-migration-serving
+    app.kubernetes.io/name: knative-serving
+    app.kubernetes.io/component: storage-version-migration-job
+    app.kubernetes.io/version: "1.10.2"
+spec:
+  ttlSecondsAfterFinished: 600
+  backoffLimit: 10
+  template:
+    metadata:
+      annotations:
+        sidecar.istio.io/inject: "false"
+      labels:
+        app: storage-version-migration-serving
+        app.kubernetes.io/name: knative-serving
+        app.kubernetes.io/component: storage-version-migration-job
+        app.kubernetes.io/version: "1.10.2"
+    spec:
+      serviceAccountName: controller
+      restartPolicy: OnFailure
+      containers:
+        - name: migrate
+          # This is the Go import path for the binary that is containerized
+          # and substituted here.
+          image: gcr.io/knative-releases/knative.dev/pkg/apiextensions/storageversion/cmd/migrate@sha256:bc91e1fdaf3b67876ca33de1ce15b1268ed0ca8da203102b7699286fae97cf58
+          args:
+            - "services.serving.knative.dev"
+            - "configurations.serving.knative.dev"
+            - "revisions.serving.knative.dev"
+            - "routes.serving.knative.dev"
+          resources:
+            requests:
+              cpu: 100m
+              memory: 100Mi
+            limits:
+              cpu: 1000m
+              memory: 1000Mi
+          securityContext:
+            allowPrivilegeEscalation: false
+            readOnlyRootFilesystem: true
+            runAsNonRoot: true
+            capabilities:
+              drop:
+                - ALL
+            seccompProfile:
+              type: RuntimeDefault
diff --git a/pkg/reconciler/common/workload_override_test.go b/pkg/reconciler/common/workload_override_test.go
index e2e185860..33e6b3e79 100644
--- a/pkg/reconciler/common/workload_override_test.go
+++ b/pkg/reconciler/common/workload_override_test.go
@@ -24,6 +24,7 @@ import (
 	mf "github.com/manifestival/manifestival"
 	"google.golang.org/api/googleapi"
 	appsv1 "k8s.io/api/apps/v1"
+	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -55,6 +56,11 @@ type expDeployments struct {
 	expDNSPolicy                 *corev1.DNSPolicy
 }
 
+type expJobs struct {
+	expNodeSelector map[string]string
+	expTolerations  []corev1.Toleration
+}
+
 func TestComponentsTransform(t *testing.T) {
 	var four int32 = 4
 	var five int32 = 5
@@ -897,6 +903,64 @@ func TestStatefulSetTransform(t *testing.T) {
 	}
 }
 
+func TestJobOverridesTransform(t *testing.T) {
+	tests := []struct {
+		Input    servingv1beta1.KnativeServing
+		Expected expJobs
+	}{{
+		Input: servingv1beta1.KnativeServing{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "job-NodeSelector-Tolerations",
+			},
+			Spec: servingv1beta1.KnativeServingSpec{
+				CommonSpec: base.CommonSpec{
+					Version: test.OperatorFlags.PreviousEventingVersion,
+					Workloads: []base.WorkloadOverride{
+						{
+							Name:         "storage-version-migration-serving-",
+							NodeSelector: map[string]string{"env": "dev"},
+							Tolerations: []corev1.Toleration{{
+								Key:      corev1.TaintNodeNotReady,
+								Operator: corev1.TolerationOpExists,
+								Effect:   corev1.TaintEffectNoSchedule,
+							}},
+						},
+					},
+				},
+			},
+		},
+		Expected: expJobs{
+			expNodeSelector: map[string]string{"env": "dev"},
+			expTolerations: []corev1.Toleration{{
+				Key:      corev1.TaintNodeNotReady,
+				Operator: corev1.TolerationOpExists,
+				Effect:   corev1.TaintEffectNoSchedule,
+			}},
+		},
+	}}
+
+	for _, test := range tests {
+		t.Run(test.Input.Name, func(t *testing.T) {
+			manifest, err := mf.NewManifest("testdata/manifest.yaml")
+			if err != nil {
+				t.Fatalf("Failed to create manifest: %v", err)
+			}
+			actual, err := manifest.Transform(OverridesTransform(test.Input.GetSpec().GetWorkloadOverrides(), log))
+			if err != nil {
+				t.Fatalf("Failed to transform manifest: %v", err)
+			}
+			resources := actual.Filter(mf.ByKind("Job")).Resources()
+			util.AssertEqual(t, len(resources), 1)
+			job := &batchv1.Job{}
+			if err = scheme.Scheme.Convert(&resources[0], job, nil); err != nil {
+				t.Fatalf("Failed to convert unstructured to deployment: %v", err)
+			}
+			util.AssertDeepEqual(t, job.Spec.Template.Spec.Tolerations, test.Expected.expTolerations)
+			util.AssertDeepEqual(t, job.Spec.Template.Spec.NodeSelector, test.Expected.expNodeSelector)
+		})
+	}
+}
+
 func TestDeploymentResourceRequirementsTransform(t *testing.T) {
 	tests := []struct {
 		DeployName string