Consolidate workload controllers status #2833
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
sig/apps
|
|
||
| See the [Kubernetes API Conventions](https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties) for more information on status conditions | ||
|
|
||
| Because of the number of changes that are involved as part of this effort, we are thinking of a phased approach where we introduce these conditions to DaemonSet controller behind a featuregate flag first in one release and then make similar changes to StatefulSet controller. This also needs some code refactoring of existing conditions for Deployment controller. |
There was a problem hiding this comment.
wrt to Deployments, are we going to copy ReplicaFailure values to a new Failed condition for consistency?
| API type(s): DaemonSet, StatefulSet | ||
| - Estimated increase in size: | ||
| - New field in DaemonSet, StatefulSet spec about 4 bytes | ||
| - Add new condition in DaemonSet, StatefulSet status |
There was a problem hiding this comment.
- are we planning on having a shared type for the condition types or is this going to be a naming convention?
- can we start using v1.Condition for DaemonSet and StatefulSet ? What about Deployments?
|
|
||
| Consider including folks who also work outside the SIG or subproject. | ||
| --> | ||
| We are proposing a new field called `.spec.progressDeadlineSeconds` to both DaemonSet and StatefulSet whose default value will be set to the max value of int32 (i.e. 2147483647) by default, which means "no deadline". In this mode, DaemonSet and StatefulSet controllers will behave exactly like its current behavior but with no `Failed` mode as they're either `Progressing` or `Complete`. This is to ensure backward compatibility with current behavior. We will default to reasonable values for both the controllers in a future release. Since a DaemonSet can make progress no faster than "healthy but not ready nodes", the default value for `progressDeadlineSeconds` can be set to 30 minutes but this value can vary depending on the node count in the cluster. The value for StatefulSet can be longer than 10 minutes since it also involves provisioning storage and binding. This default value can be set to 15 minutes in case of StatefulSet. |
There was a problem hiding this comment.
please can you elaborate on but this value can vary depending on the node count in the cluster. ?
|
|
||
| - All of the replicas associated with the DaemonSet or StatefulSet have been updated to the latest version you've specified, meaning any updates you've requested have been completed. | ||
| - All of the replicas associated with the DaemonSet or StatefulSet are available. | ||
| - No old replicas for the DaemonSet or Stateful are running. |
There was a problem hiding this comment.
is the behaviour similar to Deployment's Available? What happens when scaling down?
| --> | ||
|
|
||
| - [x] Feature gate (also fill in values in `kep.yaml`) | ||
| - Feature gate name: DaemonSetConditions, StatefulSetConditions |
There was a problem hiding this comment.
Are there two new feature flags? Why?
There was a problem hiding this comment.
There should be just one which will apply to all controllers.
There was a problem hiding this comment.
I think the idea behind this is to have better granularity for progressDeadlineSeconds in DaemonSet and StatefulSet since this change could be unexpected to some users and they might want to opt in only for one of these.
| - kube-apiserver | ||
|
|
||
| ###### Does enabling the feature change any default behavior? | ||
| The default behavior won't change but the new conditions will be additive. |
There was a problem hiding this comment.
Please elaborate on what this means.
|
|
||
| ###### What happens if we reenable the feature if it was previously rolled back? | ||
|
|
||
| The DaemonSet, StatefulSet controller sstarts respecting the `progressDeadlineSeconds` again |
There was a problem hiding this comment.
| The DaemonSet, StatefulSet controller sstarts respecting the `progressDeadlineSeconds` again | |
| The DaemonSet, StatefulSet controller starts respecting the `progressDeadlineSeconds` again. |
| - DaemonSet | ||
| - StatefulSet |
There was a problem hiding this comment.
This seems to be written as though this list could be expanded. Is that a possibility? If not, I think we should be clear about the scope.
There was a problem hiding this comment.
This is the initial set of controllers, we do plan to expand and put this as recommendation for all controllers.
| change are understandable. This may include API specs (though not always | ||
| required) or even code snippets. If there's any ambiguity about HOW your | ||
| proposal will be implemented, this is the place to discuss them. | ||
| --> |
|
|
||
| ### Test Plan | ||
| Unit and E2E tests will be added to cover the | ||
| API validation, behavioral change of DaemonSet and StatefulSet with feature gate enabled and disabled. |
There was a problem hiding this comment.
What scenarios will be covered?
| ###### Will enabling / using this feature result in any new API calls? | ||
| Yes | ||
| - Update StatefulSet, DaemonSet status | ||
| - StatefulSet, DaemonSet controllers make these calls |
| --> | ||
| - Upgrades | ||
| When upgrading from a release without this feature, to a release with | ||
| `progressDeadlineSeconds`, we will set `progressDeadlineSeconds` to max value of int32 (i.e. 2147483647). This would give users |
There was a problem hiding this comment.
I'm not a fan of making the max value have a special meaning... this doesn't seem to be in line with API standards. Are there other possible approaches?
There was a problem hiding this comment.
Since the default value for progress deadline will be in 10s of minutes, upgrading to this default value or 0 could break users. Setting this to int max is a safer option as it is effectively an infinity and thus keeps the old behaviour for these workloads.
I suppose there could be some switch for this, but this would bring more complexity and confusion to the users in my opinion.
|
|
||
| ###### Will enabling / using this feature result in increasing time taken by any operations covered by existing SLIs/SLOs? | ||
|
|
||
| No. |
There was a problem hiding this comment.
This is changing the behaviour of core workload controllers so I think this is a possibility and should be discussed.
There was a problem hiding this comment.
Agreed, although I don't expect additional API calls. The goal is to expand the current paths with additional/unified conditions.
There was a problem hiding this comment.
@ehashman added additional info
atiratree
force-pushed
the
consolidate-status
branch
from
ce32a07
to
3fde61b
Compare
atiratree
force-pushed
the
consolidate-status
branch
from
3fde61b
to
83ec184
Compare
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
k8s-ci-robot
removed
the
lifecycle/stale
atiratree
force-pushed
the
consolidate-status
branch
from
5a9b174
to
cf6306d
Compare
| @@ -0,0 +1,45 @@ | |||
| title: Consolidate Workload controllers life cycle state | |||
There was a problem hiding this comment.
| title: Consolidate Workload controllers life cycle state | |
| title: Consolidate Workload controllers life cycle status |
| # The most recent milestone for which work toward delivery of this KEP has been | ||
| # done. This can be the current (upcoming) milestone, if it is being actively | ||
| # worked on. | ||
| latest-milestone: "v1.23" |
| # The following PRR answers are required at alpha release | ||
| # List the feature gate name and the components for which it must be enabled | ||
| feature-gates: | ||
| - name: DaemonSetConditions, StatefulSetConditions |
There was a problem hiding this comment.
I'd keep a single feature flag, that will be sufficient. We don't need and want that granularity even if the implementation will happen gradually, and not all controllers will be done in the first alpha.
There was a problem hiding this comment.
Started using ExtraWorkloadConditions. Please let me know if you can come up with a better name.
There was a problem hiding this comment.
ExtendedWorkloadConditions is another alternative, but yours sounds equally well.
There was a problem hiding this comment.
that sounds a bit better, changed
| - name: DaemonSetConditions, StatefulSetConditions | ||
| components: | ||
| - kube-controller-manager | ||
| - kube-apiserver |
There was a problem hiding this comment.
Why kube-apiserver, I think this will be only on the KCM side.
There was a problem hiding this comment.
removed, but unsure. We might need it when downgrading/upgrading for validating conditions?
There was a problem hiding this comment.
conditions are not validated
| @@ -0,0 +1,912 @@ | |||
| <!-- | |||
There was a problem hiding this comment.
Drop the comments where you don't need them, they make reading the text harder, only left those that will be relevant for beta and GA.
There was a problem hiding this comment.
done, should I remove also the empty sections?
There was a problem hiding this comment.
I still see some comments you can drop, but that's a minor thing.
| - No old or mischeduled replicas/pods for the DaemonSet, ReplicaSet or Stateful are running. | ||
|
|
||
| #### Failed | ||
| In order to introduce this condition we need to come up with a new field called `.spec.progressDeadlineSeconds` which denotes the number of seconds the controller waits before indicating(in the workload controller status) that the controller progress has stalled. |
There was a problem hiding this comment.
MinReadySeconds for RS/RC should do that trick.
| Kubernetes marks a Job as `waiting` if one of the following conditions is true: | ||
|
|
||
| - There are no Pods with phase `Running` and there is at least one Pod with phase `Pending`. | ||
| - The Job is suspended. |
There was a problem hiding this comment.
IMO Waiting and Suspended are two distinct statuses. The former is awaiting for resources on the cluster to work, the other is explicit desire of the user to stop the work.
There was a problem hiding this comment.
yup, makes sense - the running distinction should be enough
|
|
||
| Consider including folks who also work outside the SIG or subproject. | ||
| --> | ||
| We are proposing a new field called `.spec.progressDeadlineSeconds` to DaemonSet and StatefulSet whose default value will be set to the max value of int32 (i.e. 2147483647) by default, which means "no deadline". |
There was a problem hiding this comment.
Since this was mentioned as a non-goal I'd split this into a separate proposal.
There was a problem hiding this comment.
ack, I suppose it will be more manageable, but it will split splinter the effort into multiple places. I left most of the progressDeadline content here for the context while acknowledging it will be done in a separate KEP
ravisantoshgudimetla
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
Waiting for @soltysh's comments to be addressed, then I'll do a final PRR review. |
k8s-ci-robot
added
size/L
atiratree
force-pushed
the
consolidate-status
branch
from
b3811ae
to
23afbb0
Compare
|
@ravisantoshgudimetla: you cannot LGTM your own PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@ehashman - Can you do PRR |
k8s-ci-robot
added
lgtm
k8s-ci-robot
added
the
lgtm
keps/sig-apps/2804-consolidate-workload-controllers-status/README.md
Outdated
Show resolved
Hide resolved
keps/sig-apps/2804-consolidate-workload-controllers-status/README.md
Outdated
Show resolved
Hide resolved
keps/sig-apps/2804-consolidate-workload-controllers-status/README.md
Outdated
Show resolved
Hide resolved
keps/sig-apps/2804-consolidate-workload-controllers-status/README.md
Outdated
Show resolved
Hide resolved
keps/sig-apps/2804-consolidate-workload-controllers-status/README.md
Outdated
Show resolved
Hide resolved
atiratree
force-pushed
the
consolidate-status
branch
from
a06fa4c
to
59ea97c
Compare
k8s-ci-robot
removed
the
lgtm
|
@ehashman thanks. Hopefully resolved, please take a look |
| In case feature gate is disabled, the workload controllers will remove the stale conditions. | ||
| In case the feature is missing in the workload controllers the conditions will not be removed and become stale. |
There was a problem hiding this comment.
I would expect the behaviour to be the same if either the feature is not implemented (old version) or the feature gate is disabled. Otherwise, we will have changed default behaviour.
Can you confirm if this is the case? Are stale conditions already removed without this feature, or is this a net new change?
There was a problem hiding this comment.
@ehashman Unfortunately we checked the controllers and most of them currently don't do updates of the conditions in the status. So it is not easy to ensure their removal.
There was a problem hiding this comment.
I think we don't want removal, as that would change default behaviour.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ehashman, ravisantoshgudimetla, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
Oh this needs an LGTM too /lgtm |
k8s-ci-robot
added
the
lgtm
|
We can take a look at it, but I am not sure about the solution atm. Thanks for the approve |
Consolidate workload controllers status