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

Uniform Use of Conditions across Workloads API Surface #51594

Closed
kow3ns opened this issue Aug 30, 2017 · 15 comments
Closed

Uniform Use of Conditions across Workloads API Surface #51594

kow3ns opened this issue Aug 30, 2017 · 15 comments

Comments

@kow3ns
Copy link
Member

@kow3ns kow3ns commented Aug 30, 2017

FEATURE REQUEST:

tldr: We should either remove Conditions from the workloads API surface or invest in them.

The workloads API does not use Conditions uniformly across the surface.

Object Uses Conditions
DaemonSet No
Deployment Yes
ReplicaSet Yes
StatefulSet No

If they are valuable to users, and if their current use is a best practice, we should use them uniformly across the API surface. If they are not valuable, or if we should really be using Status fields for the workloads API, we should deprecate them (preferably prior to v1).

To achieve consistency across the surface we could take three approaches.

  1. Invest in Conditions - If they have value we should use them more uniformly.

    1. StatefulSet could add a Condition type to indicate InProgress when it has less than the desired number of replicas, Available when all Pods are Ready, and Blocked when it fails to launch a Pod and it can no longer make progress due to its ordering constraint.
    2. DaemonSet could add a Condition type to indicate InProgress when it has yet to launch a Pod on all Nodes that its Node selector matches, Failed when it can't schedule a Pod on a Node due to a resource constraint violation (port conflict, insufficient non-compressible resource, etc), and Saturated when all selected Nodes have a Pod that is Ready.
  2. Move Conditions to Fields - Aside from reason and message, the information contained in Deployment and ReplicaSet Conditions can be represented as an enumeration (this will have the same issue as Phase) and a boolean value respectively. We will no longer have the ability communicate messages and reasons via Status, but we could consider providing these asynchronously via Events.

  3. Remove Conditions - If we think (2) is a good idea, we should also consider removing Conditions entirely. Without a reason or a message, the condition field is a summary that is mostly extrapolated form other fields of the objects' Status.

The investment to maintain what is implemented for (1) is non-trivial, and the investment to ship it as a feature that spans the workloads API surface is significant when we account for maintenance cost throughout the lifetime of the API. The detailed data collected in #50798 seems to demonstrate the use of Conditions is far from Universal in our API objects and that most of the fields of Conditions, when implemented, are not used.

(2) and (3) have a one time cost to modify the Deployment and ReplicaSet APIs, and they reduce maintenance overhead across the workloads API, but if we want to converge the storage and processing for ReplicaSet and ReplicationController we will have to carry the deprecation for quite a long time. Also, we should consider that, though they are few, the uses of Deployment Conditions might add legitimate value and warrant adoption in other controllers.

API
@kubernetes/sig-api-machinery-feature-requests
Given #7856 and community #606 we still lack a formal direction on when to use Status fields vs Conditions in the general case. We do seem to agree that we should not use them to build state machines and that they should be orthogonal to Phase. Given the options above, or any others that we should consider, what is consistent with best practices for the k8s API surface?

Apps
@kubernetes/sig-apps-feature-requests
Are Deployment Conditions useful? Is the work required to ship and maintain (1) worth it, or are (2) or (3) sufficient?

@kow3ns kow3ns added this to the v1.9 milestone Aug 30, 2017
@kow3ns kow3ns closed this Aug 30, 2017
@kow3ns kow3ns removed this from the v1.9 milestone Aug 30, 2017
@kow3ns kow3ns reopened this Aug 30, 2017
@kargakis

This comment has been minimized.

Copy link
Member

@kargakis kargakis commented Aug 30, 2017

I think there is a lot of value in signaling progress and failures via the API. We want to be able to inform users when they have waited long enough for an upgrade. We also want to expose failures such as when a Pod fails to be created in order to provide richer programmatic access to errors than just a "you have waited long enough". Both kinds of conditions enable higher-level orchestrators to do stuff like automatic rollbacks w/o the need to support automatic rollbacks in the API. It's not easy to do anything orchestration-wise today by using events and I don't know if that will ever be the case.

@kow3ns kow3ns self-assigned this Aug 31, 2017
@lavalamp lavalamp self-assigned this Aug 31, 2017
@mml

This comment has been minimized.

Copy link
Contributor

@mml mml commented Aug 31, 2017

/sub

@lavalamp

This comment has been minimized.

Copy link
Member

@lavalamp lavalamp commented Aug 31, 2017

That might be a question for sig architecture, actually.

@lavalamp lavalamp removed their assignment Aug 31, 2017
@smarterclayton

This comment has been minimized.

Copy link
Contributor

@smarterclayton smarterclayton commented Oct 5, 2017

Please add the other consumers of conditions for non-core APIs - i.e. the openshift ones referenced in the original PR

@smarterclayton

This comment has been minimized.

Copy link
Contributor

@smarterclayton smarterclayton commented Oct 5, 2017

Note that various UIs use conditions heavily. ReplicationControllers are also V1 and use conditions

@kow3ns

This comment has been minimized.

Copy link
Member Author

@kow3ns kow3ns commented Oct 5, 2017

@smarterclayton

This comment has been minimized.

Copy link
Contributor

@smarterclayton smarterclayton commented Oct 5, 2017

Yes, let me find the UI level consumption of conditions and link them in too

@smarterclayton

This comment has been minimized.

Copy link
Contributor

@smarterclayton smarterclayton commented Oct 5, 2017

Pods have it as well

@smarterclayton

This comment has been minimized.

Copy link
Contributor

@smarterclayton smarterclayton commented Oct 5, 2017

We will no longer have the ability communicate messages and reasons via Status, but we could consider providing these asynchronously via Events.

Seems very disadvantageous

@kow3ns

This comment has been minimized.

Copy link
Member Author

@kow3ns kow3ns commented Oct 6, 2017

The conclusion of sig architecture is that

  1. Events are used to express point in time occurrences, and Conditions are used to express the current state of an ongoing process. For instance, Pod creation is an event, Deployment progressing is a condition.
  2. While one could argue that, as Conditions are not used frequently, they are not particularly useful, one could also draw the conclusion that we have not done a good job of educating users and contributors as to their proper use.
  3. Conditions should not be used to build state machines. The set of Reasons may change at any time, and code that expects a fixed enumeration will be brittle.
  4. Conditions are useful for UI's to provide indications to the user as to the potential cause of the observable state of the system.
  5. Promoting Conditions to status fields is a valid use case. Some uses of Condition could be more concisely expressed as a status field. For instance, almost all controllers depend on Pod readiness and look for the boolean value associated with that Condition. It is only ever true or false. Its representation is unlikely to change.
  6. Removing Conditions from the workloads surface will require more work than supporting Conditions uniformly. We can not remove Conditions from Deployment without a high probability breaking users, and we gain little by doing so.

What we should do from here is:

  1. Add Conditions to StatefulSet and DeamonSet. The StatefulSet and DeamonSet controller will not use Conditions at the time of GA release, but they may do so in the future.
  2. Update our user and developer documentation to more clearly communicate the intended use of Conditions vs Status fields.
  3. As the API surface evolves, we should propose the promotion of Conditions to Status fields where it makes sense, and consider what policy might be applied to deprecating existing Conditions.
@kow3ns kow3ns mentioned this issue Oct 7, 2017
0 of 1 task complete
@antoineco

This comment has been minimized.

Copy link
Contributor

@antoineco antoineco commented Oct 30, 2017

@kow3ns are there already open issues for the last 3 points you suggested?

@kow3ns

This comment has been minimized.

Copy link
Member Author

@kow3ns kow3ns commented Oct 30, 2017

@antoineco I was planning on using this issue.

@kow3ns

This comment has been minimized.

Copy link
Member Author

@kow3ns kow3ns commented Nov 1, 2017

ref #7856

@kow3ns

This comment has been minimized.

Copy link
Member Author

@kow3ns kow3ns commented Jan 9, 2018

/close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.