Update the operand when image changes by jsafrane · Pull Request #32 · openshift/aws-ebs-csi-driver-operator

jsafrane · 2020-04-14T18:45:33Z

The operator may need to update the operands (Deployment, DaemonSet) when:

The images are changed by env. variables.
Log level changes in CR.
New operator starts and has updated bindata.

It's nearly impossible to detect these changes, therefore hash whole Deployment / DaemonSet .Spec and make sure the new objects are applied when the hash gets different.

cc @bertinatto @gnufied

bertinatto · 2020-04-15T14:00:39Z

pkg/operator/sync.go

+		if c.containerImagesNeedsUpdate(&existingDeployment.Spec.Template.Spec, &deploy.Spec.Template.Spec) {
+			forceRollout = true
+		}
+	}


Have you considered doing this in library-go's ApplyDeployment()? I think it'd be cleaner, as ApplyDeployment needs to Get the object anyway.

Updating the object when there's a new container image seems to be desired, so it shouldn't be a problem for other users of ApplyDeployment()?

To clarify, perhaps even better would be to compare the Spec, something like:

diff --git a/pkg/operator/resource/resourceapply/apps.go b/pkg/operator/resource/resourceapply/apps.go index 253b17ef..1a8ecb0a 100644 --- a/pkg/operator/resource/resourceapply/apps.go +++ b/pkg/operator/resource/resourceapply/apps.go if !*modified && existingCopy.ObjectMeta.Generation == expectedGeneration && !forceRollout { - return existingCopy, false, nil + if equality.Semantic.DeepEqual(required.Spec, existingCopy.Spec) { + return existingCopy, false, nil + } }

This has bothered me for awhile but none of the Apply* functions in library-go check for spec. Are you proposing that we patch it in library-go?

IMO you can't check Spec, some fields gets defaulted by API server. That's IMO the reason why Generation was introduced.

And each team will have different opinion what should be checked - now we want conainer images, but someone will want also cmdline arguments (that's where the log level is) and whatnot.

It would be better to change resourceapply functions to accept existing object, if it is known.

@deads2k, are we missing anything? This PR tries to re-deploy operand (DaemonSet + Deployment) when the operator is restarted with env. variables pointing to new images. Someone must have it solved already. How other operators detect this?

library-go's ApplyDeployment uses generation, not spec to determine whether it needs to be applied. See the check here: https://github.com/openshift/library-go/blob/master/pkg/operator/resource/resourceapply/apps.go#L39-L42

The recommended .status for operators keep track of the last applied generation. You can see usage here: https://github.com/openshift/cluster-openshift-apiserver-operator/blob/1bc63aca70a3258876befa7db6aeb8e6e05cee87/pkg/operator/workload/workload_openshiftapiserver_v311_00_sync.go#L396

@deads2k Is there a point to tracking the generation if ApplyDeployment sets the deployment spec as an annotation?

gnufied · 2020-04-15T15:12:47Z

@jsafrane can we remove operand and operator image version checks then?

gnufied · 2020-04-15T15:15:15Z

namely - https://github.com/openshift/aws-ebs-csi-driver-operator/blob/master/pkg/operator/operator.go#L45 if we are not going to use them. We also need to update the CSV

jsafrane · 2020-04-15T16:20:48Z

can we remove operand and operator image version checks then?

Maybe we can remove the operand check. I would keep the operator check in place - a new operator version may introduce changed assets in its bindata and we should apply them.

jsafrane · 2020-04-15T19:32:33Z

Talked on Slack with @deads2k and @marun, some operators add "configuration" that was used to render the required Deployment to the Deployment.Annotations

ApplyDeployment then spots the change in metadata and applies the new version. We should do the same. Hashing whole json of Deployment.Spec and putting it into annotation might be an interesting option - we could forget to add a new item to the "configuration annotation", but hash of whole Deployment.Spec will catch it.

gnufied · 2020-04-15T21:20:13Z

@jsafrane if I understand this correctly, are we planning to store all the imagePullSpec or just of the operator based on the fact that if operand image changes, operator image changes too?

gnufied · 2020-04-15T21:21:07Z

I can get behind generating SHA of spec too, because that way we may not have to track each operand individually..

bertinatto · 2020-04-16T07:03:36Z

Talked on Slack with @deads2k and @marun, some operators add "configuration" that was used to render the required Deployment to the Deployment.Annotations

ApplyDeployment then spots the change in metadata and applies the new version. We should do the same. Hashing whole json of Deployment.Spec and putting it into annotation might be an interesting option - we could forget to add a new item to the "configuration annotation", but hash of whole Deployment.Spec will catch it.

Sounds like an elegant solution to what we need. Even if some spec fields are changed/defaulted by the apiserver, the hash won't change and Apply*() won't update the object.

marun · 2020-04-16T07:23:20Z

I have a PR up if you want to test with it: openshift/library-go#773

bertinatto · 2020-04-17T08:13:49Z

pkg/operator/sync.go

+	if daemonSet.Annotations == nil {
+		daemonSet.Annotations = map[string]string{}
 	}
+	daemonSet.Annotations[specHashAnnotation] = specHash


I'd suggest moving this to the getDaemonSet() function down below (`getDeployment() for the deployment). So far we've been "patching" the objects only in those functions.

I expect the hashing to be done by ApplyDaemonSet soon, so I want to keep it close.

bertinatto · 2020-04-17T08:16:00Z

pkg/operator/sync.go

 	return deployment
 }

+func (c *csiDriverOperator) containerImagesNeedsUpdate(actual, required *corev1.PodSpec) bool {


This one is no longer used.

jsafrane · 2020-04-17T08:50:34Z

Fixed unit tests with a bit of refactoring.

jsafrane · 2020-04-17T12:27:00Z

Squashed and updated commit message.

The operator may need to update the operands (Deployment, DaemonSet) when: - The images are changed by env. variables. - Log level changes in CR. - New operator starts and has updated bindata. It's nearly impossible to detect these changes, therefore hash whole Deployment / DaemonSet .Spec and make sure the new objects are applied when the hash gets different.

bertinatto · 2020-04-17T13:34:50Z

pkg/operator/credential_request.go

+	needApply := forceRollout
+	if existing.GetGeneration() != expectedGeneration {
+		needApply = true
+	}


Do we need to check the generation if we're checking the spec hash?

Yes, we need both hash and generation. Spec hash checks for changes in the operator (new images, new bindata, ...). Generation checks for changes in the cluster (user changing the object).

bertinatto · 2020-04-17T13:36:24Z

Onde minor comment, otherwise LGTM.

marun · 2020-04-17T14:42:25Z

pkg/operator/credential_request.go

 	}

-	crClient := client.Resource(credentialsRequestResourceGVR).Namespace(required.GetNamespace())
+	if err := addCredetialsRequestHash(required); err != nil {


nit: s/Credetials/Credentials/

Nice catch, fixed

gnufied · 2020-04-18T01:06:39Z

/lgtm

openshift-ci-robot · 2020-04-18T01:07:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, jsafrane

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~~ [gnufied,jsafrane]

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

openshift-ci-robot requested review from gnufied and tsmetana April 14, 2020 18:45

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2020

bertinatto reviewed Apr 15, 2020

View reviewed changes

gnufied mentioned this pull request Apr 15, 2020

Bug 1822213: Include version in diskmaker and provisioner configmap openshift/local-storage-operator#105

Merged

bertinatto reviewed Apr 17, 2020

View reviewed changes

jsafrane force-pushed the add-image-check branch from 08260ab to c88c792 Compare April 17, 2020 08:45

jsafrane changed the title ~~Add image check~~ Update the operand when image changes Apr 17, 2020

jsafrane force-pushed the add-image-check branch from c88c792 to 8bf45f3 Compare April 17, 2020 12:26

jsafrane force-pushed the add-image-check branch from 8bf45f3 to 94e62f1 Compare April 17, 2020 13:17

bertinatto reviewed Apr 17, 2020

View reviewed changes

marun reviewed Apr 17, 2020

View reviewed changes

jsafrane force-pushed the add-image-check branch from 94e62f1 to 6a9a02a Compare April 17, 2020 16:37

openshift-ci-robot assigned gnufied Apr 18, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2020

openshift-merge-robot merged commit 9d6998e into openshift:master Apr 18, 2020

jsafrane mentioned this pull request Apr 20, 2020

Check why deployment and daemonset are updated when there're no changes #20

Closed

Conversation

jsafrane commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

gnufied commented Apr 15, 2020

Uh oh!

gnufied commented Apr 15, 2020

Uh oh!

jsafrane commented Apr 15, 2020

Uh oh!

jsafrane commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnufied commented Apr 15, 2020

Uh oh!

gnufied commented Apr 15, 2020

Uh oh!

bertinatto commented Apr 16, 2020

Uh oh!

marun commented Apr 16, 2020

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!

jsafrane commented Apr 17, 2020

Uh oh!

jsafrane commented Apr 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bertinatto commented Apr 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnufied commented Apr 18, 2020

Uh oh!

openshift-ci-robot commented Apr 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jsafrane commented Apr 14, 2020 •

edited

Loading

jsafrane commented Apr 15, 2020 •

edited

Loading