Add ability to update the image and tag of knative serving images #20

trshafer · 2019-06-20T19:15:41Z

Fixes #1.

greghaynes

A couple nits but LGTM.

greghaynes · 2019-06-20T20:12:49Z

pkg/controller/knativeserving/common/extensions.go

 				UpdateConfigMap(u, data, log)
 			}
 		}
+		if u.GetKind() == "Deployment" {


The comment on line 60 should get moved in to this function block now since it is not purely about config overriding

I think it's one challenge here, just consider the image url in deployment.
Actually, there is image url in configmap, event in args in pod definition.
Could we have a common solution to pick them up and do replacement, use regex or something.

I'm concerned with a common solution that supports replacing all possible values. I think if the configmap values need to be changed, they should use the config key that mutates configmaps.

The Image kind needs to be able to be updated though.

Updated to support Image

greghaynes · 2019-06-20T20:17:51Z

pkg/controller/knativeserving/common/helpers.go

 	}
 }
+
+// Set some data in a configmap, only overwriting common keys if they differ


Is this comment leftover from somewhere?

I'd also expect it to start with // UpdateImage

jcrossley3

Looks good! I think we only need to update the CRD file.

jcrossley3 · 2019-06-21T00:30:23Z

pkg/controller/knativeserving/common/extensions.go

+			registry := instance.Spec.Image.Image
+			tag := instance.Spec.Image.Tag
+			var deployment *appsv1.Deployment
+			if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, deployment); err == nil {


I've been using scheme.Convert to do this in other places, but I'm not sure if either is preferred. It makes me think maybe the controller's scheme should be made available in this function though.

jcrossley3 · 2019-06-21T00:33:19Z

pkg/controller/knativeserving/common/extensions.go

 		result = append(result, extension.Transformers...)
 	}
-	// Let any config in instance override everything else
 	return append(result, func(u *unstructured.Unstructured) error {


If we start adding more conditions in here, it might make more sense to define mf.Transformer functions elsewhere in the file and reference them here, similar to the mf.Inject* functions above (though they wouldn't have to be closures). I'll let you decide whether we do that now or later.

Right, if manifestival could supply something like InjectImageURL, that's will be great.

jcrossley3 · 2019-06-21T00:37:40Z

pkg/apis/serving/v1alpha1/knativeserving_types.go

 	// +optional
 	Config map[string]map[string]string `json:"config,omitempty"`
+
+	Image Registry `json:"registry,omitempty"`


I think we should include the proper updates to deploy/crds/serving_v1alpha1_knativeserving_crd.yaml with this PR. Either manually or via operator-sdk generate k8s

jcrossley3 · 2019-06-21T00:44:40Z

pkg/controller/knativeserving/common/helpers.go

 		unstructured.SetNestedField(cm.Object, v, "data", k)
 	}
 }
+


I don't love the name "helpers.go" for this file (my fault), and you've added so many related functions that I might lean toward putting them in their own say, "images.go" file? Still in the common package, of course.

k4leung4 · 2019-06-24T15:51:38Z

/test pull-knative-serving-operator-go-coverage

trshafer · 2019-06-25T22:27:19Z

I have verified that this works with:

apiVersion: serving.knative.dev/v1alpha1
kind: KnativeServing
metadata:
  name: knative-serving
spec:
  registry:
    default-registry: "my-registry.io/my-build/knative/cmd"
    default-tag: "v0.6.0"

How do you feel about this CRD?

Should it be:

spec:
  registry:
    default:
      registry: my-registry.io/my-build/knative/cmd
      tag: v0.6.0

It's a bit strange to have a default but not support the other services in the crd. Should this also support the other services before merging?

config/crds/serving_v1alpha1_knativeserving_crd.yaml

jcrossley3 · 2019-06-25T23:45:49Z

pkg/controller/knativeserving/common/extensions.go

+func updateDeployment(scheme *runtime.Scheme, instance *servingv1alpha1.KnativeServing, u *unstructured.Unstructured) error {
+
+	var deployment = &appsv1.Deployment{}
+	err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, deployment)


is this better than scheme.Convert(u, deployment)?

I couldn't get scheme.Convert(u, deployment) to work. I got error message TypeMeta not present in src which comes from https://github.com/kubernetes/apimachinery/blob/961b39a1baa06f6c52bdd048a809b9f5b47f1337/pkg/conversion/converter.go#L828. The issue I believe is TypeMeta

serving-operator/vendor/k8s.io/api/apps/v1/types.go

Line 255 in 6c0272e

metav1.TypeMeta `json:",inline"`

is not in the unstructured map. It's unable to take the inline json content and convert it to a Deployment. This way worked.

I'll look more into the scheme tests and see if I can reproduce this issue: https://github.com/kubernetes/apimachinery/blob/961b39a1baa06f6c52bdd048a809b9f5b47f1337/pkg/runtime/scheme_test.go#L1

jcrossley3 · 2019-06-25T23:49:35Z

pkg/apis/serving/v1alpha1/knativeserving_types.go

+type Registry struct {
+	DefaultRegistry string `json:"default-registry,omitempty"`
+	DefaultTag      string `json:"default-tag,omitempty"`
+}


What is the "Default*" prefix meant to imply? Why not just Name and Tag as the only elements of the Registry struct?

Per comment #20 (comment), it demonstrates the that this is the default registry and tag to use. I am starting to prefer the nested default map.

jcrossley3 · 2019-06-25T23:55:24Z

How do you feel about this CRD?

Should it be:

spec:
  registry:
    default:
      registry: my-registry.io/my-build/knative/cmd
      tag: v0.6.0

Personally, I'd prefer not to see "registry" used in two places: how about just name and tag?

It's a bit strange to have a default but not support the other services in the crd. Should this also support the other services before merging?

What other services? Like activator or autoscaler? So something like this?

spec:
  registry:
    default:
      name: my-registry.io/my-build/knative/cmd
      tag: v0.6.0
    autoscaler:
      name: my-other-registry.io/my-build/knative/cmd
      tag: v0.6.1

jcrossley3 · 2019-06-26T13:17:16Z

Also, would we expect this to be affected as well?

serving-operator/config/resources/knative-serving-0.6.0.yaml

Line 997 in 6c344b7

    
             queueSidecarImage: gcr.io/knative-releases/github.com/knative/serving/cmd/queue@sha256:1e40c99ff5977daa2d69873fff604c6d09651af1f9ff15aadf8849b3ee77ab45

jcrossley3 · 2019-06-26T13:31:10Z

pkg/controller/knativeserving/common/images_test.go

+		registry: "new-registry.io/test/path",
+		tag:      "new-tag",
+		expected: "new-registry.io/test/path/queue:new-tag",
+	},


Another challenge just occurred to me: RHT currently renames its images with a knative-serving- prefix, so queue becomes knative-serving-queue. Hence, our image name will be quay.io/openshift-knative/knative-serving-queue:v0.6.0. Will the current logic stick a / in there? If so, can we just expect the separator to be in the name, e.g quay.io/openshift-knative/knative-serving- and new-registry.io/test/path/?

trshafer · 2019-06-26T15:47:20Z

Per comment #20 (comment), @greghaynes suggested an option that supports both a default and an individual override: #1 (comment). Your suggestion is the correct assumption. I should probably the ability to override the default per service in this PR, so I don't need to circle back again.

trshafer · 2019-06-26T15:52:10Z

Per comment #20 (comment), that's a good question. I don't think so, but it would be surprising. Is it unreasonable for the user to make the CR:

spec:
  config:
    deployment: my-registry.io/my-build/knative/cmd/queue:v0.6.0
  registry:
    default:
      name: my-registry.io/my-build/knative/cmd
      tag: v0.6.0

What's unfortunate is the user will always need to do this, which is pretty non-obvious. @greghaynes @jcrossley3 what do you think?

jcrossley3

Only minor nits, so...
/lgtm

jcrossley3 · 2019-07-03T00:06:18Z

pkg/apis/serving/v1alpha1/knativeserving_types.go

+type Registry struct {
+	// The default image reference template to use for all knative images.
+	// It takes the form of example-registry.io/custom/path/${NAME}:custom-tag
+	// ${NAME} will be replaced by the deployment name.


Deployment name? Container? Image? Basename?

jcrossley3 · 2019-07-03T00:07:53Z

pkg/apis/serving/v1alpha1/knativeserving_types.go

 	// +optional
 	Config map[string]map[string]string `json:"config,omitempty"`
+
+	// A means to override the corresponding deployment images in the upstream deployments.


s/deployment //

jcrossley3 · 2019-07-03T00:09:32Z

pkg/controller/knativeserving/common/extensions.go


+func updateCachingImage(scheme *runtime.Scheme, instance *servingv1alpha1.KnativeServing, u *unstructured.Unstructured) error {
+	var image = &caching.Image{}
+	err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, image)


I'll look into why scheme.Convert doesn't work after this is merged.

…the Deployment Containers Adds knative.dev/caching dependency

knative-metrics-robot · 2019-07-03T00:18:18Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/controller/knativeserving/common/config_maps.go	Do not exist	0.0%
pkg/controller/knativeserving/common/extensions.go	Do not exist	0.0%
pkg/controller/knativeserving/common/images.go	Do not exist	100.0%

knative-prow-robot · 2019-07-03T00:18:29Z

@trshafer: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-knative-serving-operator-go-coverage	`be16a31`	link	`/test pull-knative-serving-operator-go-coverage`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jcrossley3 · 2019-07-03T00:54:04Z

/lgtm

evankanderson

/lgtm
/hold cancel

knative-prow-robot requested review from bbrowning and houshengbo June 20, 2019 19:15

knative-prow-robot added the size/XXL label Jun 20, 2019

k4leung4 changed the title ~~Add ability to update the image and tag of knative serving images~~ [WIP] Add ability to update the image and tag of knative serving images Jun 20, 2019

knative-prow-robot added the do-not-merge/work-in-progress label Jun 20, 2019

greghaynes reviewed Jun 20, 2019

View reviewed changes

trshafer force-pushed the add-image branch 2 times, most recently from b874955 to ce4fe0f Compare June 20, 2019 23:24

jcrossley3 suggested changes Jun 21, 2019

View reviewed changes

jcrossley3 reviewed Jun 21, 2019

View reviewed changes

trshafer force-pushed the add-image branch 4 times, most recently from e7ed236 to f4dba39 Compare June 24, 2019 15:37

trshafer force-pushed the add-image branch 3 times, most recently from eab4ac6 to 82d87a3 Compare June 25, 2019 22:08

trshafer changed the title ~~[WIP] Add ability to update the image and tag of knative serving images~~ Add ability to update the image and tag of knative serving images Jun 25, 2019

knative-prow-robot removed the do-not-merge/work-in-progress label Jun 25, 2019

jcrossley3 reviewed Jun 25, 2019

View reviewed changes

config/crds/serving_v1alpha1_knativeserving_crd.yaml Outdated Show resolved Hide resolved

jcrossley3 reviewed Jun 25, 2019

View reviewed changes

config/crds/serving_v1alpha1_knativeserving_crd.yaml Outdated Show resolved Hide resolved

jcrossley3 reviewed Jun 25, 2019

View reviewed changes

jcrossley3 reviewed Jun 26, 2019

View reviewed changes

trshafer force-pushed the add-image branch from fa59ba9 to 1be4b20 Compare June 29, 2019 00:44

knative-prow-robot removed the lgtm label Jun 29, 2019

trshafer force-pushed the add-image branch 9 times, most recently from 66c69d1 to 3963496 Compare July 2, 2019 20:51

Add ability to update the registry and tag of the knative serving images

249ed79

trshafer force-pushed the add-image branch from 3963496 to 6776d1a Compare July 2, 2019 21:22

knative-prow-robot added size/XXL and removed size/L labels Jul 2, 2019

trshafer force-pushed the add-image branch from 6776d1a to 430ebfc Compare July 2, 2019 21:26

jcrossley3 reviewed Jul 3, 2019

View reviewed changes

knative-prow-robot assigned jcrossley3 Jul 3, 2019

knative-prow-robot added the lgtm label Jul 3, 2019

Support replacing the knative Image Spec using the same structure as …

be16a31

…the Deployment Containers Adds knative.dev/caching dependency

trshafer force-pushed the add-image branch from 430ebfc to be16a31 Compare July 3, 2019 00:17

knative-prow-robot removed the lgtm label Jul 3, 2019

knative-prow-robot added the lgtm label Jul 3, 2019

evankanderson reviewed Jul 3, 2019

View reviewed changes

knative-prow-robot removed the do-not-merge/hold label Jul 3, 2019

knative-prow-robot merged commit d267a64 into knative:master Jul 3, 2019

jcrossley3 mentioned this pull request Nov 8, 2019

Clean up the [un]structured conversions a bit #211

Merged

Add ability to update the image and tag of knative serving images #20

Add ability to update the image and tag of knative serving images #20

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

trshafer Jul 2, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

k4leung4 commented Jun 24, 2019

Uh oh!

trshafer commented Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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!

jcrossley3 commented Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcrossley3 commented Jun 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trshafer commented Jun 26, 2019

Uh oh!

trshafer commented Jun 26, 2019

Uh oh!

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

trshafer commented Jun 20, 2019 •

edited

Loading

trshafer Jul 2, 2019 •

edited

Loading

trshafer commented Jun 25, 2019 •

edited

Loading

jcrossley3 commented Jun 25, 2019 •

edited

Loading