Inject Serving images by skonto · Pull Request #1630 · openshift-knative/serverless-operator

skonto · 2022-06-20T07:46:42Z

Part of the work defined in SIG Release
Replaces Serving images with the ones built on CI via env vars
Gets manifests from the mid stream repo
Ingress will come as a next step

skonto · 2022-06-20T10:16:28Z

Serving tests faiure:
AutoscaleSustainingTest
TestAutoscaleSustaining/aggregation-weighted-exponential

skonto · 2022-06-20T10:16:32Z

/retest

skonto · 2022-06-20T11:19:59Z

error: unable to recognize "STDIN": Get "https://api.ci-ocp-4-10-amd64-aws-us-east-1-dsbnd.hive.aws.ci.openshift.org:6443/api?timeout=32s": dial tcp: lookup api.ci-ocp-4-10-amd64-aws-us-east-1-dsbnd.hive.aws.ci.openshift.org on 172.30.0.10:53: no such host

skonto · 2022-06-20T11:21:50Z

/retest

skonto · 2022-06-20T12:50:38Z

/retest

skonto · 2022-06-22T10:04:18Z

/hold

skonto · 2022-06-22T10:08:55Z

-    wget --no-check-certificate "$url" -O "$target_file"
-
+    if [[ ${KNATIVE_SERVING_MANIFESTS_DIR} = "" ]]; then
+      url="https://raw.githubusercontent.com/skonto/serving/${branch}/openshift/release/artifacts/$index-$file"


After we merge the PR at the openshift/serving repo we can then change back to https://raw.githubusercontent.com/openshift/knative-serving/${branch}/openshift/release/artifacts/$index-$file

+1 we did same

pierDipi

Great, another little step!

pierDipi · 2022-06-22T10:07:01Z

+  serving_artifacts_branch: image_injection_1.3
+


nit: maybe group this with dependencies.serving as put them closer to each other?

pierDipi · 2022-06-22T10:14:07Z

-    wget --no-check-certificate "$url" -O "$target_file"
-
+    if [[ ${KNATIVE_SERVING_MANIFESTS_DIR} = "" ]]; then
+      url="https://raw.githubusercontent.com/skonto/serving/${branch}/openshift/release/artifacts/$index-$file"


nit: if we align the serving repo first we can point to the proper midstream branch instead of yours, meaning we can merge your branch in serving first, and then we can just use the final one here.

This would simplify the review of the deleted patches making sure that what we're removing here is added to the serving repo.

or that way - yeah

pierDipi · 2022-06-22T10:15:24Z

-git apply "$root/openshift-knative-operator/hack/001-serving-namespace-deletion.patch"
+# When openshift-knative/serving uses this repo to run a job (eg. PR against openshift-knative/serving) it will use a minimum
+# setup with net-kourier. Thus it will not use the release artifacts generated under openshift-knative-operator/cmd/kodata/knative-serving.
+# Instead openshift-knative/serving uses its own generated ci manifests and sets KNATIVE_SERVING_TEST_MANIFESTS_DIR.


Suggested change

# Instead openshift-knative/serving uses its own generated ci manifests and sets KNATIVE_SERVING_TEST_MANIFESTS_DIR.

# Instead openshift/knative-serving uses its own generated ci manifests and sets KNATIVE_SERVING_TEST_MANIFESTS_DIR.

Missing nit

pierDipi · 2022-06-22T10:16:40Z

-diff --git a/openshift-knative-operator/cmd/operator/kodata/knative-serving/1.3.0/2-serving-core.yaml b/openshift-knative-operator/cmd/operator/kodata/knative-serving/1.3.0/2-serving-core.yaml
-index 4f7af33d..4a5ce15f 100644
--- a/openshift-knative-operator/cmd/operator/kodata/knative-serving/1.3.0/2-serving-core.yaml
-+++ b/openshift-knative-operator/cmd/operator/kodata/knative-serving/1.3.0/2-serving-core.yaml


Great! Another step to the removal of versioned directories!

pierDipi · 2022-06-22T10:21:21Z

@@ -1,31 +0,0 @@
-diff --git a/openshift-knative-operator/cmd/operator/kodata/knative-serving/1.3.0/2-serving-core.yaml b/openshift-knative-operator/cmd/operator/kodata/knative-serving/1.3.0/2-serving-core.yaml


I don't see this patch in https://github.com/skonto/serving/tree/image_injection_1.3/openshift/patches, maybe it's planned for later but I'd keep track of these removed patches.

It is here: https://github.com/skonto/serving/tree/image_injection_1.3/openshift/release/manifest-patches.
Reason is that https://github.com/skonto/serving/tree/image_injection_1.3/openshift/patches only contains the patches for the ci yaml files. The release ones are there. This is because the Serving setup for ci tests (eg. when a PR is done at the openshift/knative-serving) repo is minimal. It is not the same one we have at the S-O side eg. no istio is used only kourier. For now the release manifests are downloaded in that new dir until we unify the process of testing and releasing.

Ok, thanks!

One note on patches:

https://github.com/skonto/serving/blob/image_injection_1.3/openshift/release/manifest-patches/001-serving-namespace-deletion.patch is based on a generated bundles, is there any reason for not doing something like this https://github.com/openshift-knative/eventing-kafka-broker/blob/2f16a913ea82e6b36d0ccc7e4eb5f5deea7c0f1b/openshift/release/generate-release.sh#L10-L11, that would remove the need to change "serving.knative.dev/release: "v1.3.0" to serving.knative.dev/release: "v1.4.0" in the next release.

Similar comment on other patches since basing them on the origin of the final manifests would benefit from future changes.

But that's not really relevant for this PR, just a thought

Yeah we could do that I will take a note and do it when we merge the patches. As a side note https://github.com/skonto/serving/tree/image_injection_1.3/openshift/patches, contains also code patches in https://github.com/skonto/serving/blob/image_injection_1.3/openshift/release/manifest-patches it is only manifest patches as they are used in Serverless Operator.

skonto · 2022-07-01T12:44:33Z

/retest

skonto · 2022-07-01T13:22:39Z

@matzew @pierDipi ready, gentle ping.

pierDipi

/hold
2 nits, they can be fixed later tho

/lgtm
/approve

pierDipi · 2022-07-01T13:35:31Z

+# setup with net-kourier. Thus it will not use the release artifacts generated under openshift-knative-operator/cmd/kodata/knative-serving.
+# Instead openshift-knative/serving uses its own generated ci manifests and sets KNATIVE_SERVING_TEST_MANIFESTS_DIR.
+# Extensive Serving testing is done at this repo only. For the latter we do use manifests under openshift-knative-operator/cmd/kodata/knative-serving which are fetched from the midstream
+# repo. TODO: unify the artifacts at the mid stream repo.


I guess TODO is done?

Suggested change

# repo. TODO: unify the artifacts at the mid stream repo.

# repo.

Nope this is not done. Unifying means using the manifests from one place. Check my comment: #1630 (comment).
But we can remove that.

pierDipi · 2022-07-01T13:36:35Z

-git apply "$root/openshift-knative-operator/hack/001-serving-namespace-deletion.patch"
+# When openshift-knative/serving uses this repo to run a job (eg. PR against openshift-knative/serving) it will use a minimum
+# setup with net-kourier. Thus it will not use the release artifacts generated under openshift-knative-operator/cmd/kodata/knative-serving.
+# Instead openshift-knative/serving uses its own generated ci manifests and sets KNATIVE_SERVING_TEST_MANIFESTS_DIR.


Missing nit

skonto · 2022-07-01T14:49:01Z

/retest

skonto · 2022-07-01T15:13:07Z

/retest

skonto · 2022-07-01T15:13:55Z

Nits done waiting for tests to pass.

pierDipi · 2022-07-01T15:23:40Z

/lgtm

openshift-ci · 2022-07-01T15:23:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierDipi, skonto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [pierDipi,skonto]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

skonto · 2022-07-01T17:12:37Z

/unhold

openshift-ci Bot added the do-not-merge/work-in-progress label Jun 20, 2022

openshift-ci Bot requested review from jcrossley3 and rhuss June 20, 2022 07:46

openshift-ci Bot added the approved label Jun 20, 2022

skonto mentioned this pull request Jun 20, 2022

Inject images and download manifests for S-O openshift/knative-serving#1150

Merged

skonto removed request for jcrossley3 and rhuss June 20, 2022 07:55

skonto changed the title ~~[wip] Inject Serving images~~ Inject Serving images Jun 22, 2022

openshift-ci Bot removed the do-not-merge/work-in-progress label Jun 22, 2022

skonto requested review from nak3 and pierDipi June 22, 2022 10:03

openshift-ci Bot added the do-not-merge/hold label Jun 22, 2022

skonto commented Jun 22, 2022

View reviewed changes

pierDipi reviewed Jun 22, 2022

View reviewed changes

Stavros Kontopoulos added 2 commits July 1, 2022 13:52

inject images

1aa3022

updates

2738ac4

pierDipi approved these changes Jul 1, 2022

View reviewed changes

openshift-ci Bot assigned pierDipi Jul 1, 2022

openshift-ci Bot added the lgtm label Jul 1, 2022

remove todo

e36450c

openshift-ci Bot removed the lgtm label Jul 1, 2022

fix repo ref

c13ac17

openshift-ci Bot added the lgtm label Jul 1, 2022

openshift-ci Bot removed the do-not-merge/hold label Jul 1, 2022

openshift-ci Bot merged commit a352d77 into openshift-knative:main Jul 1, 2022

	# Instead openshift-knative/serving uses its own generated ci manifests and sets KNATIVE_SERVING_TEST_MANIFESTS_DIR.
	# Instead openshift/knative-serving uses its own generated ci manifests and sets KNATIVE_SERVING_TEST_MANIFESTS_DIR.

		@@ -1,31 +0,0 @@
		diff --git a/openshift-knative-operator/cmd/operator/kodata/knative-serving/1.3.0/2-serving-core.yaml b/openshift-knative-operator/cmd/operator/kodata/knative-serving/1.3.0/2-serving-core.yaml

	# repo. TODO: unify the artifacts at the mid stream repo.
	# repo.

Conversation

skonto commented Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skonto commented Jun 20, 2022

Uh oh!

skonto commented Jun 20, 2022

Uh oh!

skonto commented Jun 20, 2022

Uh oh!

skonto commented Jun 20, 2022

Uh oh!

skonto commented Jun 20, 2022

Uh oh!

skonto commented Jun 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierDipi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto commented Jul 1, 2022

Uh oh!

skonto commented Jul 1, 2022

Uh oh!

pierDipi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto commented Jul 1, 2022

Uh oh!

skonto commented Jul 1, 2022

Uh oh!

skonto commented Jul 1, 2022

Uh oh!

pierDipi commented Jul 1, 2022

Uh oh!

openshift-ci Bot commented Jul 1, 2022

Uh oh!

skonto commented Jul 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

skonto commented Jun 20, 2022 •

edited

Loading

skonto Jun 23, 2022 •

edited

Loading

skonto Jun 24, 2022 •

edited

Loading

skonto Jul 1, 2022 •

edited

Loading