Add image override logic for DaemonSet by savitaashture · Pull Request #281 · knative/serving-operator

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.

Details

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

 )

-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

 		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

 		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

Details

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

Conversation

savitaashture commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

jcrossley3 left a comment • 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!

jcrossley3 Feb 3, 2020 • 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!

savitaashture Feb 3, 2020 • 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!

savitaashture commented Feb 3, 2020

Uh oh!

savitaashture commented Feb 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savitaashture Feb 4, 2020 • 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!

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!

savitaashture commented Feb 6, 2020

Uh oh!

knative-metrics-robot commented Feb 6, 2020

Uh oh!

jcrossley3 commented Feb 6, 2020

Uh oh!

knative-prow-robot commented Feb 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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