Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use server-generated revisions names as default for services (instead of BYO revision name) #1144

Closed
rhuss opened this issue Nov 26, 2020 · 8 comments · Fixed by #1240
Closed

Comments

@rhuss
Copy link
Contributor

rhuss commented Nov 26, 2020

As discussed in knative/serving#9544 BYO revision names that we use by default (i.e. the client set the revision name and not the serving controller), has some issues when it comes to sink bindings, but also for kn service apply we can't use BYO revision names. Tbh, I always have a hard time to remember the actual use case for what BYO revision names are used (and again, I forgot about it ;-(. I conclude, that the use case for BYO revision names is so rare that it does not justified to use this as a default (when contrasting this to the trouble that it brings elsewhere).

My suggestion is to make server-side naming the default and only when --revision-name is specified we switch to BYO revision names. And the moment, switching off BYO revision naming is even very obfuscated as one has to provide a --revision-name= (i.e. an empty name) so switch this off.

The switch should work transparently except for that one use that I don't remember anymore (@evankanderson I think you explained it to me earlier, but I forgot again. sorry :( . From a UX perspective we just would not allow anymore a --revision-name without or empty argument, and just use server-side revision naming by default when this option is not given.

Opinions ?

@rhuss rhuss changed the title User server-generated revisions names as default for services Use server-generated revisions names as default for services (instead of BYO revision name) Nov 26, 2020
@duglin
Copy link
Contributor

duglin commented Nov 26, 2020

Did serving agree to add support?

@rhuss
Copy link
Contributor Author

rhuss commented Dec 7, 2020

Did serving agree to add support?

@duglin wdy mean with adding support ? Server-side naming of revisions exists from the very befinning.

@duglin
Copy link
Contributor

duglin commented Dec 8, 2020

Maybe I'm doing something wrong, but when I create a ksvc via kubectl the revision name (spec.template.metadata.name) field is not set by the server. Is there something I need to turn on to make the serving controller set a name?

@rhuss
Copy link
Contributor Author

rhuss commented Dec 8, 2020

It's set on the revisions, not on the service. The lack of the name in the service definition indicates that the server should generate the revision names when they are created by the backend. When this field is set, then its taken over for the name. But this also mean, that when you set the field on your own, you have to change the name manually every time you are doing an update (that is what kn does for you). And that is why kubectl apply doesn't work when the name is set manually as kubectl apply has not idea that it should change the name to something unique.

@rhuss
Copy link
Contributor Author

rhuss commented Dec 8, 2020

tl;dr - Ksvc spec.template.metadata.name is only set when you use the BYO revision name feature and the value of the field is the client-side provided revision name. If left empty, it stays empty but the server will generate unique revisions names by itself if it needs to create a new revision.

@duglin
Copy link
Contributor

duglin commented Dec 8, 2020

ah sorry - for some reason I thought you were talking about the ksvc revName, not the revision itself. Pardon the distraction :-)

jcrossley3 added a commit to drogue-iot/drogue-cloud that referenced this issue Dec 16, 2020
This required 3 fixes:

1) the knative webhook was failing when the ksvc yaml was applied
because we were using kn to update them. by default, kn uses "BYO
revision naming": knative/client#1144. so
since we're only using kn for nudging the services to address a
different bug, i just bounce the ksvc deployments to achieve the same
effect. i also moved the loop into the nudge function itself, after
realizing that the 'device-management-service' was failing anyway.

2) added the ENABLE_AUTH env var to the 'device-management-service' to
prevent its crash-looping.

3) ignored the error resulting from patching ingress/keycloak.
rhuss added a commit to rhuss/knative-client that referenced this issue Feb 22, 2021
knative-prow-robot pushed a commit that referenced this issue Feb 24, 2021
…ded) (#1240)

* update: Change default to server-side generated revision names

Fixes #1144

* update: Fix wait loop for synthetic events for which the generations are already in sync

* fix assertion as a service label change does not create a new revision

* fix: --cluster-local does not create a new revision but just changes a service's label

* chore: Fixed formatting

* update: Check generation in update already before doing a watch

* fixed lint issue

* fix test assertions
@mkedwards
Copy link

FTR, one common use case for client-side generation of revision names is when you're recording what the revision name will be in an immutable object that's frozen before the revision itself is created. In GCP world, you might have a Cloud Build job that does a deploy to a Cloud Run service, and you want a randomly-generated-in-advance revision suffix to appear as a substitution in the build description. There's room for maybe 10 characters of nonce there, so collisions will be infrequent even when restricted to lowercase + numbers. (Sequence numbers alone, no matter where they're generated, are not really a good answer, for any number of reasons; knative/serving#9736 is a totally unsurprising flavor of bug.)

@rhuss
Copy link
Contributor Author

rhuss commented Jan 3, 2022

@mkedwards I agree that client-side naming makes some sense for automated setups (like CI builds) if you can live with the disadvantages as described in https://github.com/knative/client/releases/tag/v0.21.0. As an alternative, you can achieve the correlation via other meta-data like labels (searchable) or annotations, even when using server-side naming (though tooling might be a bit more awkward to use then).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants