dont create new revision if image is unchanged

guillaumeblaquiere commented 5 years ago

In what area(s)?

Classifications: /kind bug

What version of Knative Client?

Version: v20190828-local-f53ebd8 Build Date: 2019-08-28 09:06:01 Git Revision: f53ebd8-dirty Dependencies:

serving: v0.6.0
eventing: v0.8.0

What version of Knative Serving running on your cluster?

0.6.x

Expected Behavior

When I run the command kn service update XXX --image gcr.io/project/XXX I would like that the latest version of my container is deployed. The latest version must be automatically checked against the GCR full checksum and not with the short name in the command.

Actual Behavior

Nothing is performed because the kn tool check only the name of the image and not the tag of the latest version in the GCR.

Steps to Reproduce the Problem

Run kn service create XXX --image gcr.io/project/XXX
Deploy a new version in GCR
Run kn service update XXX --image gcr.io/project/XXX -> No new revision created. But the container are really different.

Workaround

I alternate the command

kn service update XXX --image gcr.io/project/XXX
kn service update XXX --image gcr.io/project/XXX:latest

navidshaikh commented 5 years ago

Version: v20190828-local-f53ebd8 Build Date: 2019-08-28 09:06:01 Git Revision: f53ebd8-dirty Dependencies:

serving: v0.6.0 eventing: v0.8.0

This doesn't look like built from latest master on 2019-08-28.

You can grab the nightly kn binary here https://github.com/knative/client/blob/master/docs/README.md#installing-kn.

kn has couple of flags added recently to configure the behavior to pull the image tag afresh with each new revision. You can flag --no-lock-to-digest to pull the image tag afresh with each new revision. Following flags are available during service create and update.

      --lock-to-digest           keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision) (default true)
      --no-lock-to-digest        do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)

guillaumeblaquiere commented 5 years ago

Better but not perfect Here my tests:

guillaume_blaquiere@cloudshell:~/knative-client-pilot (gbl-imt-homerider-basguillaueb)$ kn service create process-message --image gcr.io/gbl-imt-homerider-basguillaueb/process-me
ssage-gke -e TOPIC_PERIODIC=message-periodic -e TOPIC_EVENT=message-event -e GCP_PROJECT=gbl-imt-homerider-basguillaueb -e ALLOWED_PATTERN_ORANGE='.*((90DFFB(81|BD58|7367){1})|(5
322)).*'
Service 'process-message' successfully created in namespace 'default'.
Waiting for service 'process-message' to become ready ... OK
Service URL:
http://process-message.default.example.com
guillaume_blaquiere@cloudshell:~/knative-client-pilot (gbl-imt-homerider-basguillaueb)$ kn service list
NAME              URL                                          GENERATION   AGE     CONDITIONS   READY   REASON
process-message   http://process-message.default.example.com   1            5m54s   3 OK / 3     True
pubsub-to-bq      http://pubsub-to-bq.default.example.com      3            11m     3 OK / 3     True
guillaume_blaquiere@cloudshell:~/knative-client-pilot (gbl-imt-homerider-basguillaueb)$ kn service update process-message --image gcr.io/gbl-imt-homerider-basguillaueb/process-me
ssage-gke
Waiting for service 'process-message' to become ready ... OK
Service 'process-message' updated in namespace 'default'.
guillaume_blaquiere@cloudshell:~/knative-client-pilot (gbl-imt-homerider-basguillaueb)$ kn service list
NAME              URL                                          GENERATION   AGE     CONDITIONS   READY   REASON
process-message   http://process-message.default.example.com   2            6m59s   3 OK / 3     True
pubsub-to-bq      http://pubsub-to-bq.default.example.com      3            12m     3 OK / 3     True
guillaume_blaquiere@cloudshell:~/knative-client-pilot (gbl-imt-homerider-basguillaueb)$ kn service update process-message --image gcr.io/gbl-imt-homerider-basguillaueb/process-me
ssage-gke
Waiting for service 'process-message' to become ready ...
timeout: service 'process-message' not ready after 60 seconds
guillaume_blaquiere@cloudshell:~/knative-client-pilot (gbl-imt-homerider-basguillaueb)$ kn service list
NAME              URL                                          GENERATION   AGE     CONDITIONS   READY   REASON
process-message   http://process-message.default.example.com   3            9m12s   3 OK / 3     True
pubsub-to-bq      http://pubsub-to-bq.default.example.com      3            14m     3 OK / 3     True
guillaume_blaquiere@cloudshell:~/knative-client-pilot (gbl-imt-homerider-basguillaueb)$

There no container update in gcr

Create the service -> revision 1
Update -> revision 2 -> container digest is the same, should do nothing.
Update -> failed timeout 60s -> Finally deployed revision 3 -> container digest is the same, should do nothing

for information:

guillaume_blaquiere@cloudshell:~/knative-client-pilot (gbl-imt-homerider-basguillaueb)$ kn version
Version:      v20190904-local-2985e55
Build Date:   2019-09-04 06:07:49
Git Revision: 2985e55
Support:
- Serving: v0.8.0  v0.7.1
- API(s):  v1alpha1

rusde commented 5 years ago

As per the code kn client currently does not query for imageDigest. It just calls KNative API's to update service.

Does the proposal here is for kn client to query imageDigest parameters and do a diff with current KNative revision deployed. Only call KNative API if diff contains some properties?

As per I see it flag --lock-to-digest does allow customers to update other parameters without deploying latest image version. I see this as no issue. Thoughts?

navidshaikh commented 5 years ago

As per the code kn client currently does not query for imageDigest. It just calls KNative API's to update service.

+1

Does the proposal here is for kn client to query imageDigest parameters and do a diff with current KNative revision deployed. Only call KNative API if diff contains some properties?

I'd say this is something we should probably discuss with Serving WG, IMO this check belongs Serving side.

rusde commented 5 years ago

Opened serving issue

sixolet commented 5 years ago

I believe the current client behavior on master is that:

When you specify an --image parameter to an update, we'll generate a name for you locally, by default
By generating a new name locally, we force a new revision
By forcing a new revision, Serving will re-resolve the image
When you specify --image, any locked-to-digest image will be replaced with the image-by-tag again, which the server will re-resolve. If you don't specify --image, we will lock it to the digest if we're doing that for the service.

I think this does exactly what you want.

If you don't generate the revision name locally, you can end up with a noop change on the server, which will not force a new revision, and thus not re-resolve your tag.

navidshaikh commented 5 years ago

While switching from client side revision-name generation to server side revision-name generation, there is one revision created though

client-side revision-name generation

13:24 ➜  client git:(master)  ./kn service create hello --image gcr.io/knative-samples/helloworld-go
Service 'hello' successfully created in namespace 'default'.
Waiting for service 'hello' to become ready ... OK

Service URL:
http://hello.default.apps-crc.testing

13:24 ➜  client git:(master)  ./kn service update hello --image gcr.io/knative-samples/helloworld-go
Waiting for service 'hello' to become ready ... OK
Service 'hello' updated in namespace 'default'.

13:25 ➜  client git:(master)  ./kn revision list -s hello
NAME            SERVICE   GENERATION   AGE   CONDITIONS   READY   REASON
hello-vxndg-2   hello     2            21s   4 OK / 4     True    
hello-lgrjp-1   hello     1            39s   4 OK / 4     True

switching to server-side revision-name generation + no-change in image, new revision generated

13:25 ➜  client git:(master)  ./kn service update hello --image gcr.io/knative-samples/helloworld-go --revision-name=
Waiting for service 'hello' to become ready ... OK
Service 'hello' updated in namespace 'default'.

13:25 ➜  client git:(master)  ./kn revision list -s hello                                                            
NAME            SERVICE   GENERATION   AGE   CONDITIONS   READY   REASON
hello-ntjq6     hello     3            18s   4 OK / 4     True    
hello-vxndg-2   hello     2            54s   4 OK / 4     True    
hello-lgrjp-1   hello     1            72s   4 OK / 4     True

sub-sequent update + server-side revision-name generation + no-image-change, no new revision generated

13:25 ➜  client git:(master)  ./kn service update hello --image gcr.io/knative-samples/helloworld-go --revision-name=
Waiting for service 'hello' to become ready ... OK
Service 'hello' updated in namespace 'default'.

13:25 ➜  client git:(master)  ./kn revision list -s hello                                                            
NAME            SERVICE   GENERATION   AGE   CONDITIONS   READY   REASON
hello-ntjq6     hello     3            22s   4 OK / 4     True    
hello-vxndg-2   hello     2            58s   4 OK / 4     True    
hello-lgrjp-1   hello     1            76s   3 OK / 4     True

github-actions[bot] commented 4 years ago

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

navidshaikh commented 3 years ago

/reopen /remove-lifecycle stale

knative-prow-robot commented 3 years ago

@navidshaikh: Reopened this issue.

In response to [this](https://github.com/knative/client/issues/398#issuecomment-728752479): >/reopen >/remove-lifecycle stale Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

github-actions[bot] commented 3 years ago

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

rhuss commented 3 years ago

To reiterate on this issue, I see really two possible desired behaviours here, when --image is provided on update with the same image as the original one.

A: The image has changed in the registry (different digest) --> Then a new revision should be created
B: The image has not changed in the registry --> No new revision should be created.

When using BYO revision as we do now, then for every update we change the name locally which triggers the image-digest resolving algorithm on the server-side. However, even when the digest is the same a new revision is created as the client is in the driver seat and decided to create a new revision because it provided a new revision name. Using this with BYO revision names can fix this only if the client has a way to detect whether an image has been changed, by contacting the registry directly. This, however, is not possible for multiple reasons (network topology, authentication mismatch).

When using server-side generated names then it looks like that no new revision is created if the same image name is provided during an update. I'm not sure if this is still the case, we should verify that. However, I believe if this issue should be resolved, then it should be done on the server-side. Not via BYO name (which we really should move into a niche and not use it as default as outlined in #1144 )

github-actions[bot] commented 3 years ago

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

rhuss commented 3 years ago

/remove-lifecycle stale

This issue is still important and connected to these issues:

github-actions[bot] commented 3 years ago

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

rhuss commented 3 years ago

/remove-lifecycle stale

github-actions[bot] commented 2 years ago

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

rhuss commented 2 years ago

/remove-lifecycle stale

knative / client