Skip to content

Adds declarative-friendly state transition guidance #1079

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.

Sign up for GitHub

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

Draft
mbleigh wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Adds declarative-friendly state transition guidance #1079

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions aip/general/0216.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,44 @@ message PublishBookRequest {
- The comment for the field **should** document the resource pattern.
- Other fields **may** be included.

### Declarative-friendly state transitions

@toumorokoshi toumorokoshi Apr 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn't call this "declarative-friendly": there really shouldn't be a different between declarative-friendly APIs and non-declarative friendly APIs.

Is there a way to make the guidance more generic?

@toumorokoshi toumorokoshi Apr 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say this guidance is more "managing state with resource fields" than declarative.


If a resource's state can be modified by [declarative tooling][aip-128], it
**should** expose a separate `desired_state` field taking the same enum type.

```proto
message Book {
enum State {
STATE_UNSPECIFIED = 0;
UNPUBLISHED = 1;
PUBLISHING = 2;
PUBLISHED = 3;
}

State state = 1 [(google.api.field_behaviopr = OUTPUT_ONLY)];

@bgrant0607 bgrant0607 Apr 27, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

State desired_state = 2;

@bgrant0607 bgrant0607 Apr 27, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issues with this:

  1. Many State values (e.g., transitional ones) wouldn't be valid for the desired state.
  2. Enums can't be extended without potentially breaking clients.
  3. Per-service state enums prevent cross-service lifecycle management. This is a major gap in AIP 152 (Jobs), as one example. Also in 164 (soft delete).

Separate attributes are often better. For instance, Kubernetes Deployment has a "paused" field:
https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.27/#deploymentspec-v1-apps

@mbleigh mbleigh Apr 27, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. As mentioned below, transitional states would be INVALID_ARGUMENT if supplied to desired_state.
  2. I feel like I've known what scenario this breaks before but can't recall it - can you give an example?
  3. It sounds like you're saying that this entire AIP is a mistake and should be deprecated -- if that's the case we really need guidance to replace it.

@toumorokoshi toumorokoshi Apr 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like I've known what scenario this breaks before but can't recall it - can you give an example?

@bgrant0607 I feel like we should just update google.aip.dev/180 to explain this one. One reason is that clients unaware of the new enum value will receive the unspecified value, and send that over the wire potentially erasing the currently set parameter.

It sounds like you're saying that this entire AIP is a mistake and should be deprecated -- if that's the case we really need guidance to replace it.

I'm not sure if it's that strong of a case, but we do need updated guidance that enables known gaps like cross-service lifecycle management.

I definitely agree guidance needs be updated, but it's on a longer backlog. At minimum I think anything that removes guidance from AIP-128 and reduces the gap between declarative-friendly and non-declarative-friendly APIs is an improvement. Thanks for authoring this!

@toumorokoshi toumorokoshi Apr 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify I think Brian is arguing for more granular switches on fields (paused with a boolean vs desired_state that has the same set of possible enum values). I do agree that, at minimum, desired_state shouldn't have the ability to be set to transient states (RUNNING)

@loudej loudej Apr 29, 2023 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest this actually isn't a recommendation we should make. It may even be an anti-pattern, although it's understandable why it comes up as an idea every so often.

  • It's true that custom methods are essentially invisible to desired-state systems

  • It's also true that some custom methods have the effect of moving the output-only state field (eventually) to a similarly-named value.

So it seems like there's a connection between the two. But having an assignable field of the output-only enum is very rarely the best way to design the new hand-free resource control feature.

A different question to ask is "What config-feature on this resource would I add that, when optionally configured by the customer, would tell the backend when these custom methods should be called automatically?"

Looking at VM instance, some custom methods that can't be reached are start/stop/reset/suspend/resume... A quick hypothetical of those...

message Instance {
  ...
  // Assign PowerControl details to enable automated control of running state.
  // Default behavior when nil is to leave all running-state to imperative method calls.
  PowerControl power_control;

  message PowerControl {
    // When specified, indicates that the appropriate custom method should be called as needed.
    // Calls to the custom methods "stop" "start" "suspend" and "resume" will fail if they contradict
    // the configured goal.
    enum goal = UNSPECIFIED | STARTED | SUSPENDED | STOPPED;

    // If the instance has been running since before this time, and it is not after this time,
    // then the instance will restart. Setting it to "now" is an effective way to cause a single reset.
    // Setting to a future time will schedule a single reset for when that time arrives.
    Timestamp restart_when;

    // other control features added here as they become necessary
  }
}

... just a gist, but you get the idea.

  • if unassigned the current behavior (non-automated, custom method driven only) is fully in effect
  • if there is an enum goal, the values can be only what's needed and meaningful. they don't even necessarily have to align with actual output_only state enum.
  • any other resource-type-specific subtlety about rules and conditions of when those automatic effects can/should take place can be taken into account with additional fields in the new structure (and more can be added in the future to make the hands-free control more sophisticated)

}
```

- The `state` field's output **may** change to more granular
[active states](#common-states) to indicate progress toward the `desired_state`.
- The `desired_state` field **must** only allow [resting state](#common-states)

@toumorokoshi toumorokoshi Apr 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed above, the best way to enforce this is to use a separate enum entirely.

Doing so eliminates the need to require the runtime behavior outlined below (preconditions, invalid_argument)

enum values.
- Attempting to set `desired_state` to an [active state](#common-states) **must**
error with `INVALID_ARGUMENT` (HTTP 400).
- Attempting to set `desired_state` to a state that cannot be transitioned to
from the current state **must** error with `FAILED_PRECONDITION` (HTTP 400).
- A [long-running][] operation **must not** complete until the `state` has
Comment thread
mbleigh marked this conversation as resolved.

@toumorokoshi toumorokoshi Apr 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this apply for custom methods, or for updates?

In either case I kind of feel like this shouldn't be the case - the client can poll the resource state if desired, and if the operation takes hours, have a several-hour-long LRO just to see if your parameter was accepted feels like it'll limit it's utility for local clients like gcloud and Terraform.

reached the `desired_state` (or **must** error if it cannot).
- If a resource supports both declarative `desired_state` and imperative
[state transition methods](#state-transition-methods), executing a state
transition method **should** modify the desired state appropriately.

@bgrant0607 bgrant0607 Apr 27, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will create drift in declarative clients. It would be better to not have the desired state field in that case.

@mbleigh mbleigh Apr 27, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will create drift - but I'm trying to consider:

  1. Existing APIs with custom methods that might migrate to this pattern and have both for that reason.
  2. Flexibility for clients to use a more imperative state transition method if they prefer (presumably the client is not generally declarative in this scenario).

Declarative tooling expects to be the only manager of a given resource, so out of band updates of any kind create drift problems. This is something that API consumers have to manage for themselves.

@toumorokoshi toumorokoshi Apr 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this is why it's best to not have desired_state, but rather individual flags that limit state transitions?

e.g. suspended would set the resource to be suspended, and not allow other state transitions.

Or maybe have a way to clear desired state (unspecified) that allows custom methods to mutate the state? effectively a lock.

- If a resource is created without `desired_state`, the field **must** default

@bgrant0607 bgrant0607 Apr 27, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're working on guidance around how to handle default values. When services change values set by clients, it causes problems for declarative and other state-driven clients. It would be better to leave the value unspecified.

@mbleigh mbleigh Apr 27, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can buy an argument for leaving it unspecified.

@toumorokoshi toumorokoshi Apr 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to unspecified. unspecified IMO means "I'm managing this state outside of the resource". If i saw a resting value I'd read that to be "keep this resource in the resting state".

to the same resting value as `state`.
- If an out-of-band event causes the resting `state` of the resource to change,
the `desired_state` **must** be changed to match. This does not apply to
a temporary condition that will return to the same resting state.

## Additional Guidance

### Default value
Expand Down Expand Up @@ -239,4 +277,5 @@ necessary.
- **2019-07-18**: Added explicit guidance on the unspecified value.

[aip-126]: ./0126.md
[aip-128]: ./0128.md
[long-running]: ./0151.md