Add image override logic for DaemonSet #281

savitaashture · 2020-02-03T07:43:36Z

Fixes #280

Proposed Changes

Extended image override logic for DaemonSet when there is image update by override

Release Note

This PR will be validated when release manifest updated to changes from knative/serving#6624
This PR has dependency knative.dev/pkg

knative-prow-robot

@savitaashture: 1 warning.

In response to this:

Fixes #280

Proposed Changes

Extended image override logic for DaemonSet when there is image update by override

Release Note

This PR will be validated when release manifest updated to changes from knative/serving#6624

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.

knative-prow-robot · 2020-02-03T07:43:48Z

pkg/reconciler/knativeserving/common/images.go

@@ -38,12 +38,16 @@ var (
 	containerNameVariable = "${NAME}"
 )

-func DeploymentTransform(instance *servingv1alpha1.KnativeServing, log *zap.SugaredLogger) mf.Transformer {
+func ResourceTransform(instance *servingv1alpha1.KnativeServing, log *zap.SugaredLogger) mf.Transformer {


Golint comments: exported function ResourceTransform should have comment or be unexported. More info.

markusthoemmes

A lot of the duplication can probably be removed by using a PodSpecable duck type rather than Deployment and DaemonSet specifically. That's material for a follow up PR though imo.

/lgtm

markusthoemmes · 2020-02-03T10:04:35Z

pkg/reconciler/knativeserving/common/images.go

 	return func(u *unstructured.Unstructured) error {
 		// Update the deployment with the new registry and tag
 		if u.GetKind() == "Deployment" {
 			return updateDeployment(instance, u, log)
 		}
+		// Update the daemonSet with the new registry and tag


nit:

switch u.GetKind() case "Deployment": return updateDeployment(instance, u, log) case "DaemonSet": return updateDaemonSet(instance, u, log) default: return nil }

Thank you @markusthoemmes Updated PR

jcrossley3

I almost feel like it's worth biting the bullet and using PodSpecable. Seems like that would reduce a lot of the code in the PR and simultaneously pull in support for StatefulSets as well. WDYT?

jcrossley3 · 2020-02-03T16:11:18Z

pkg/reconciler/knativeserving/common/extensions.go

@@ -32,7 +32,7 @@ func (platforms Platforms) Transformers(kubeClientSet kubernetes.Interface, inst
 		mf.InjectOwner(instance),
 		mf.InjectNamespace(instance.GetNamespace()),
 		ConfigMapTransform(instance, log),
-		DeploymentTransform(instance, log),
+		ResourceTransform(instance, log),


I think this name is a little vague. DaemonSets and StatefulSets are really just specialized Deployments. And pretty much everything is a Resource. I'd say just keep the name DeploymentTransform or maybe PodSpecableTransform? Or save that for another PR?

Thank you @jcrossley3 I will keep PodSpecableTransform 👍

savitaashture · 2020-02-03T18:51:17Z

I almost feel like it's worth biting the bullet and using PodSpecable. Seems like that would reduce a lot of the code in the PR and simultaneously pull in support for StatefulSets as well. WDYT?

Yep true i will make use of PodSpecable in the PR and update

savitaashture · 2020-02-04T15:40:38Z

CI fail until PR merge

markusthoemmes · 2020-02-04T15:47:59Z

pkg/reconciler/knativeserving/common/images.go

+	specData.Spec.Template.Spec.ImagePullSecrets = addImagePullSecrets(
+		specData.Spec.Template.Spec.ImagePullSecrets, &registry, log)
+
+	unstructuredData, err := duck.ToUnstructured(specData.DeepCopyObject().(duck.OneOfOurs))


Oh I see, you need the selector in PodSpecable to successfully serialize back here? 🤔

Can you instead use the existing unstructured and only override things? I.e. take the transformed podSpec and set it into the unstructured element at Spec.Template or something like that?

Do you mean this https://github.com/knative/serving-operator/pull/281/files#diff-411a842e3f404ed5adc1b082ea595d72R78 directly instead of duck.ToUnstructured ?

No I don't think that'd work either. Lemme try if what I though actually works before I bother you more with it 😂

Yep that also won't work

In pseudo-code, we need something like this:

podSpecable := duck.FromUnstructured(u) updateImages(podSpecable) updatePullSecret(podSpecable) unstructured.SetNestedField(u.Object, podSpecable.Spec.Template, "spec", "template")

Or alternatively

podSpecable := duck.FromUnstructured(u) afterChanges := podSpecable.DeepCopy() updateImages(afterChanges) updatePullSecret(afterChanges) patch := generatePatch(podSpecable, afterChanges) applyPatch(u, patch)

That way we keep the fields of the unstructured intact that PodSpecable doesn't capture (see selector etc.) while also having the profit of using just one type. I haven't gotten a working version of the above together yet though.

jcrossley3

I love that you're handling all the possible image overrides in a single ImageTransform function. Great idea!

jcrossley3 · 2020-02-04T15:54:36Z

pkg/reconciler/knativeserving/common/images.go

-		if u.GetAPIVersion() == "caching.internal.knative.dev/v1alpha1" && u.GetKind() == "Image" {
-			return updateCachingImage(instance, u)
+		switch u.GetKind() {
+		case "Deployment", "DaemonSet":


Wanna knock out "StatefulSet" while we're at it? 😄

jcrossley3

Just some minor nits

jcrossley3 · 2020-02-05T18:06:53Z

pkg/reconciler/knativeserving/common/images.go

+			return nil
+		default:
+			return nil
 		}
-		return nil


nit: i would lose the default case and replace the two returns in the switch with the old one at the end of the function.

jcrossley3 · 2020-02-05T18:08:08Z

pkg/reconciler/knativeserving/common/images.go

+	err := scheme.Scheme.Convert(u, daemonSet, nil)
+	if err != nil {
+		log.Error(err, "Error converting Unstructured to daemonSet", "unstructured", u, "daemonSet", daemonSet)
+		return err
+	}


nit: do the assignment in the if clause for consistency

jcrossley3 · 2020-02-05T18:08:23Z

pkg/reconciler/knativeserving/common/images.go

+	err = scheme.Scheme.Convert(daemonSet, u, nil)
+	if err != nil {
+		return err
+	}


nit: same as ^^

savitaashture · 2020-02-06T04:58:02Z

@jcrossley3 thank you
Addressed all the comments

knative-metrics-robot · 2020-02-06T04:58:22Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/knativeserving/common/images.go	87.3%	85.1%	-2.2

jcrossley3 · 2020-02-06T12:47:59Z

Thanks @savitaashture 😄
/lgtm
/approve

knative-prow-robot · 2020-02-06T12:48:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcrossley3, savitaashture

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jcrossley3]

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

googlebot added the cla: yes Author(s) signed a CLA. label Feb 3, 2020

knative-prow-robot reviewed Feb 3, 2020

View reviewed changes

knative-prow-robot added the size/L label Feb 3, 2020

knative-prow-robot requested review from jcrossley3 and k4leung4 February 3, 2020 07:43

Add image override logic to Daemonset

b2a4459

savitaashture force-pushed the issue-280 branch from 11051d1 to b2a4459 Compare February 3, 2020 07:48

markusthoemmes reviewed Feb 3, 2020

View reviewed changes

knative-prow-robot assigned markusthoemmes Feb 3, 2020

knative-prow-robot added the lgtm label Feb 3, 2020

fix review comment

445a1a9

knative-prow-robot removed the lgtm label Feb 3, 2020

jcrossley3 reviewed Feb 3, 2020

View reviewed changes

savitaashture mentioned this pull request Feb 4, 2020

Add GetGroupVersionKind and Selector WithPod knative/pkg#1043

Closed

Addressed review comment by added duck type implementation

22ce2c3

markusthoemmes reviewed Feb 4, 2020

View reviewed changes

jcrossley3 reviewed Feb 4, 2020

View reviewed changes

fix review comment

4c2c562

jcrossley3 reviewed Feb 5, 2020

View reviewed changes

fix review comment

2e17274

knative-prow-robot assigned jcrossley3 Feb 6, 2020

knative-prow-robot added the lgtm label Feb 6, 2020

knative-prow-robot added the approved label Feb 6, 2020

knative-prow-robot merged commit 3ebfe06 into knative:master Feb 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add image override logic for DaemonSet #281

Add image override logic for DaemonSet #281

savitaashture commented Feb 3, 2020 •

edited

Loading

knative-prow-robot left a comment

knative-prow-robot Feb 3, 2020

markusthoemmes left a comment

markusthoemmes Feb 3, 2020

savitaashture Feb 3, 2020

jcrossley3 left a comment •

edited

Loading

jcrossley3 Feb 3, 2020 •

edited

Loading

savitaashture Feb 3, 2020 •

edited

Loading

savitaashture commented Feb 3, 2020

savitaashture commented Feb 4, 2020

markusthoemmes Feb 4, 2020

savitaashture Feb 4, 2020 •

edited

Loading

markusthoemmes Feb 4, 2020

savitaashture Feb 5, 2020

markusthoemmes Feb 5, 2020

jcrossley3 left a comment

jcrossley3 Feb 4, 2020

jcrossley3 left a comment

jcrossley3 Feb 5, 2020

jcrossley3 Feb 5, 2020

jcrossley3 Feb 5, 2020

savitaashture commented Feb 6, 2020

knative-metrics-robot commented Feb 6, 2020

jcrossley3 commented Feb 6, 2020

knative-prow-robot commented Feb 6, 2020

Add image override logic for DaemonSet #281

Add image override logic for DaemonSet #281

Conversation

savitaashture commented Feb 3, 2020 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Choose a reason for hiding this comment

markusthoemmes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcrossley3 left a comment • edited Loading

Choose a reason for hiding this comment

jcrossley3 Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

savitaashture Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

savitaashture commented Feb 3, 2020

savitaashture commented Feb 4, 2020

Choose a reason for hiding this comment

savitaashture Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcrossley3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcrossley3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savitaashture commented Feb 6, 2020

knative-metrics-robot commented Feb 6, 2020

jcrossley3 commented Feb 6, 2020

knative-prow-robot commented Feb 6, 2020

savitaashture commented Feb 3, 2020 •

edited

Loading

jcrossley3 left a comment •

edited

Loading

jcrossley3 Feb 3, 2020 •

edited

Loading

savitaashture Feb 3, 2020 •

edited

Loading

savitaashture Feb 4, 2020 •

edited

Loading