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

v2 API proposal "desired vs actual" #17333

Open
thockin opened this issue Nov 16, 2015 · 30 comments
Open

v2 API proposal "desired vs actual" #17333

thockin opened this issue Nov 16, 2015 · 30 comments
Assignees
Labels
area/api Indicates an issue on api area. area/app-lifecycle kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/cli Categorizes an issue or PR as relevant to SIG CLI. wg/api-expression Categorizes an issue or PR as relevant to WG API Expression.

Comments

@thockin
Copy link
Member

thockin commented Nov 16, 2015

This has come up a bunch in a lot of small ways, and my mind keeps coming back to this idea, so I want to write it down.

In early early kube API we had desired and actual (or something like that) structs. Since they were the same struct there were a bunch of things in the struct that were things we didn't want users to set (which eventually became status) and other problems. As we changed to spec+status, we lost the concept of "actual" and just folded it into spec.

This conflates a couple of ideas and makes it hard to distinguish what the user actually asked for from what was assigned to them. I am proposing we reinstate that distinction.

I propose that every top-level object (all of them, not just the ones that make sense RIGHT NOW) holds 4 fields (plus ObjectMeta):

  • metadata - same meaning as today
  • spec - what the user asked for, with API-defined defaults applied AND NOTHING ELSE. Every field is available to users.
  • actual - the exact same structure as spec, but with other fields populated by the system (e.g. nodeName when scheduled, clusterIP when auto-assigned, and so on)
  • status - same meaning as today

If a user wants to save a pod and move it to another cluster, spec is what they want. If a piece of software wants to know how a pod is operating, actual and status are what they want.

I chose this layout (which changes the meaning of spec because I figured it makes it easier on users to change over, while making it harder for system components (change to actual). This may be wrong, but it's not obvious. An alternative would be that spec means "actual" and request means "what the user asked for"; users write request and the system writes spec. This has the advantage of software that observes state being easier to change over, but means that users have more pain. But it is perhaps better to break users, who actually SEE error messages. Sometimes bigger changes are better because the need to something is obvious (see the bugs people hit with some really subtle v1beta1 -> v1 changes).

It would be nice to make one field writable ONLY by users (create/update) and one field writable only by the system. We've used binding for scheduling, but there are other things (like clusterIP) that are written directly in-process.

We could make this round-trippable by embedding request under status in v1 or even as a new top-level object + sub-resources.

Lastly, there's some question about preserving exactly what the user asked for WITHOUT defaults applied, but I am less sure that is needed or particularly useful.

@bgrant0607 @smarterclayton @lavalamp

@thockin thockin added the area/api Indicates an issue on api area. label Nov 16, 2015
@lavalamp
Copy link
Member

I have been thinking for a while that a "one writer" policy (at some TBD level of granularity) would be a good thing.

For comparison, why not annotate (comment/struct tag) every field that the system writes and forbid the user from setting it. This seems less invasive. (If the user is allowed to make a request, add a "BlahRequest" block or some such.)

With the system in the OP, it looks to me like there is a big new class of bugs when the user updates the spec.

  • How does that get translated to "actual"
  • Components I guess should always watch actual and not spec? Watching the wrong thing will be mostly correct and therefore hard to notice.

Additionally, doesn't this still have the original problem that caused us to split the types? There'd be fields in 'spec' that the user shouldn't set.

kubectl apply already stores the original request in an annotation.

You can never unapply defaults with this system (also with our current system). It's unclear to me if this is a bug or a feature.

@bgrant0607 bgrant0607 added area/app-lifecycle team/api kind/design Categorizes issue or PR as related to design. labels Nov 16, 2015
@bgrant0607
Copy link
Member

See also #1178

@thockin
Copy link
Member Author

thockin commented Nov 16, 2015

I have been thinking for a while that a "one writer" policy (at some TBD level of granularity) would be a good thing.

For comparison, why not annotate (comment/struct tag) every field that the system writes and forbid the user from setting it. This seems less invasive. (If the user is allowed to make a request, add a "BlahRequest" block or some such.)

This is basically what we do with spec and status. There should be
nothing in spec that a user can't set. But frequently (nodeName)
they won't make a choice and the system will do it for them. We could
reflect that in status but then status becomes an ad hoc mess of
random fields, and components end up watching some fields in both.

I much prefer to have a source of "combined desired truth" in a single
structure.

With the system in the OP, it looks to me like there is a big new class of bugs when the user updates the spec.

How does that get translated to "actual"

Writes to spec are reflected into actual. Its seems to be the
same logic we have for create/update today.

Components I guess should always watch actual and not spec? Watching the wrong thing will be mostly correct and therefore hard to notice.

Correct. I also do not like this. This is why we should consider
names and semantics carefully :)

Additionally, doesn't this still have the original problem that caused us to split the types? There'd be fields in 'spec' that the user shouldn't set.

Without fully analyzing EVERY field of every struct: No, everything in
spec is user-writeable. actual is where we write things like
bindings and assignments, etc.

kubectl apply already stores the original request in an annotation.

It's not very usable, and it is becoming a problem, as I understand it.

You can never unapply defaults with this system (also with our current system). It's unclear to me if this is a bug or a feature.

This was my last point. My feeling is that it's not that important to
get back "what I asked for" independent of API version. I could be
wrong.

@bgrant0607
Copy link
Member

Also relevant: #13038, #16543, #8190

@bgrant0607 bgrant0607 added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Nov 16, 2015
@lavalamp
Copy link
Member

Writes to spec are reflected into actual. Its seems to be the same logic we have for create/update today.

I think it requires an entirely new process? User writes an update to spec; then some mystery code runs in the server, and afterwards both spec and actual will have been updated.

That mystery code will involve a bunch of boilerplate (probably under-tested and bug ridden), or it will involve another invocation of the conversion stuff (complex, still under-tested, but probably fewer bugs).

This is basically what we do with spec and status. ...

That's not really what I had in mind at all. I was saying, individually per-field in spec, have a comment marker/struct field tag called e.g. set-by-system; if that's on the field, then only a system component may write to the field.

@thockin
Copy link
Member Author

thockin commented Nov 17, 2015

Writes to spec are reflected into actual. Its seems to be the same logic we have for create/update today.

I think it requires an entirely new process? User writes an update to spec; then some mystery code runs in the server, and afterwards both spec and actual will have been updated.

User POSTs to spec. Per-resource logic copies that do actual and
then does all the stuff we do today.

User PUTs to spec. Per-resource logic merges that with actual.

I'm hand-waving, obviously it's a little tricky and we'll need to
define the semantics we actually want to see and that we can plausibly
implement.

That mystery code will involve a bunch of boilerplate (probably under-tested and bug ridden), or it will involve another invocation of the conversion stuff (complex, still under-tested, but probably fewer bugs).

yeah, we can code-gen a lot of this.

This is basically what we do with spec and status. ...

That's not really what I had in mind at all. I was saying, individually per-field in spec, have a comment marker/struct field tag called e.g. set-by-system; if that's on the field, then only a system component may write to the field.

I think it's a little gross, but I guess it could work. We could
maybe be clever and store dual-source fields in a special type so that
depending on which sub-resource you access, get a different view of
the object.

e.g.

User POSTs {"kind": "Frobber", "spec": { "byUser": 123 }} to /api/v2/.../name.

System writes .spec.bySystem = 456

GET of /api/v2/.../name yields {"kind": "Frobber", "spec": { "byUser": 123, bySystem: 456 }, "status": {}}

GET of /api/v2/.../name/orig yields {"kind": "Frobber", "spec": { "byUser": 123 }, "status": {}}

Internally bySystem is held in a struct that knows which plane the
value exists on.

@smarterclayton
Copy link
Contributor

I'm in favor of this, at the risk of having even larger API objects than we
do now. I agree with Brian that separation between user's desired intent
and the reactive elements of the cluster (actual) is much better expressed
with a new field.

If you think about what most automated elements of the cluster are doing,
they're applying override / field mutations on specific entities. Maybe we
should have an "overrides" endpoint that takes patches (or something less
than a full object) and adds them to the resource. Most users can't see
the patches, but they can see "actual". A machine can clear overrides by
DELETEing /overrides. Storing the delta changes may make more sense in
some cases - especially if they contain source annotations about who is
applying them.

user POST {spec: {something: bar}}
user GET {spec: {something: bar}, actual: {something: bar}}
machine PUT /overrides "patch format, change spec.something to 5 by
'machine1'"
user GET {spec: {something: bar}, actual: {something: 5}}
machine GET /overrides "sees list of 1 patch format, including machine1"

Both admission control and initializers could set overrides.

On Mon, Nov 16, 2015 at 8:22 PM, Tim Hockin notifications@github.com
wrote:

Writes to spec are reflected into actual. Its seems to be the same
logic we have for create/update today.

I think it requires an entirely new process? User writes an update to
spec; then some mystery code runs in the server, and afterwards both spec
and actual will have been updated.

User POSTs to spec. Per-resource logic copies that do actual and
then does all the stuff we do today.

User PUTs to spec. Per-resource logic merges that with actual.

I'm hand-waving, obviously it's a little tricky and we'll need to
define the semantics we actually want to see and that we can plausibly
implement.

That mystery code will involve a bunch of boilerplate (probably
under-tested and bug ridden), or it will involve another invocation of the
conversion stuff (complex, still under-tested, but probably fewer bugs).

yeah, we can code-gen a lot of this.

This is basically what we do with spec and status. ...

That's not really what I had in mind at all. I was saying, individually
per-field in spec, have a comment marker/struct field tag called e.g.
set-by-system; if that's on the field, then only a system component may
write to the field.

I think it's a little gross, but I guess it could work. We could
maybe be clever and store dual-source fields in a special type so that
depending on which sub-resource you access, get a different view of
the object.

e.g.

User POSTs {"kind": "Frobber", "spec": { "byUser": 123 }} to
/api/v2/.../name.

System writes .spec.bySystem = 456

GET of /api/v2/.../name yields {"kind": "Frobber", "spec": { "byUser": 123, bySystem: 456 }, "status": {}}

GET of /api/v2/.../name/orig yields {"kind": "Frobber", "spec": { "byUser": 123 }, "status": {}}

Internally bySystem is held in a struct that knows which plane the
value exists on.


Reply to this email directly or view it on GitHub
#17333 (comment)
.

@bgrant0607
Copy link
Member

@lavalamp @thockin
Quick comment regarding: "That's not really what I had in mind at all. I was saying, individually per-field in spec, have a comment marker/struct field tag called e.g. set-by-system; if that's on the field, then only a system component may write to the field."

This cannot work and I'm opposed to it on principle. There is deliberately no hard line between what can be set by the user and what can be set automatically. One example of many: vertical auto-scaling. For more examples, see #17097.

@bgrant0607
Copy link
Member

@smarterclayton My original proposal in #1178 was that automated components set "underrides" -- everything the user specifies takes precedence over automatically set values.

@lavalamp
Copy link
Member

@bgrant0607 sounds like you just don't buy my "one-writer" policy idea at all. I mention it because it's much simpler and less invasive.

I guess I am not in favor of this at the moment. We're already struggling with the amount of complexity in the API system and this makes it worse.

@bgrant0607
Copy link
Member

cc @jackgr

@bgrant0607
Copy link
Member

I think the annotation is fine for now. I agree we can't take this on at the moment. We can revisit later. I'll add this to #8190.

If we're worried about the cost, we should make it possible to retrieve selected fields (#1459).

@pwittrock
Copy link
Member

In this proposal, what would be the behavior for fields set by the user initially and then overridden by the system in actual? Presumably actual would take priority. If the user updates the field in spec, does it then overwrite the field in actual set by the system?

Is actual meant to be written by any actor that is not the "owner" / creator of the object? e.g. a 3rd party CICD system that would continuously update the image for a Deployment?

@jbeda
Copy link
Contributor

jbeda commented Jan 11, 2017

It looks like this issue has new life based on some recent discussions.

I think that it is important to look at this from the end user experience. Right now the best we have is kubectl apply that does a 3 way merge to try and determine which fields should be clobbered. This type of "inexact ownership" is, I think, simple on the surface but a bucket of complexity underneath.

I wrote up some of the thoughts on viewing driving the system as declarative vs. imperative and the confusing aspects of apply here: https://docs.google.com/document/d/12x8wcyn7GPKvkw5Aj28yiGQJZqvzm6xA8888n0LzJB0/edit#heading=h.kwnwi2roeue0

@thockin
Copy link
Member Author

thockin commented Jan 11, 2017 via email

@fabiand
Copy link
Contributor

fabiand commented Jan 12, 2017

I always had the assumption that - as @thockin says - actual will end up to have the same value as spec, if not then there must be an issue somewhere.
And thus it would be odd if actual could somehow override the spec value.

Just my 2ct

@bgrant0607
Copy link
Member

Declarative defaulting is relevant: #25460

The current open-coded version-specific defaults are a problem when converting the configured state, since defaults are deliberately not set in the configured state.

@bgrant0607 bgrant0607 added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/feature Categorizes issue or PR as related to a new feature. and removed priority/backlog Higher priority than priority/awaiting-more-evidence. labels Sep 12, 2017
@bgrant0607 bgrant0607 added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Oct 3, 2017
@bgrant0607
Copy link
Member

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 13, 2018
@bgrant0607
Copy link
Member

/remove-lifecycle stale
/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 23, 2018
@lavalamp lavalamp self-assigned this Feb 9, 2018
@bgrant0607
Copy link
Member

@bgrant0607
Copy link
Member

Presentation: https://docs.google.com/presentation/d/1fty6X_k0ABEn2Cq9kWO6JyIYhx6-8xtEbj3AP3qcQRQ/edit#slide=id.p

@nilebox
Copy link

nilebox commented Mar 1, 2018

For those who want to watch @lavalamp talking through the presentation posted above, there is a recording of SIG Architecture meeting https://www.youtube.com/watch?v=6HfntQM757M

@julianvmodesto
Copy link
Contributor

/wg apply

@MadhavJivrajani
Copy link
Contributor

/remove-kind design
/kind feature

kind/design will soon be removed from k/k in favor of kind/feature. Relevant discussion can be found here: kubernetes/community#5641

@k8s-ci-robot k8s-ci-robot removed the kind/design Categorizes issue or PR as related to design. label Jun 29, 2021
@sftim
Copy link
Contributor

sftim commented Jun 27, 2022

Should we open a KEP / start a pre-KEP discussion about this change?

@lavalamp
Copy link
Member

We should close this as it will never happen.

@sftim
Copy link
Contributor

sftim commented Jul 30, 2023

Maybe we can enhance server-side apply to better capture the specified vs. actual differences. I can see that being both useful and compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates an issue on api area. area/app-lifecycle kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/cli Categorizes an issue or PR as relevant to SIG CLI. wg/api-expression Categorizes an issue or PR as relevant to WG API Expression.
Projects
Status: Needs Triage
Development

No branches or pull requests