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.

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

Jump to bottom

make deployment status behaviour more descriptive #31226

Merged
merged 1 commit into from
Jan 27, 2022
Merged

make deployment status behaviour more descriptive #31226

merged 1 commit into from
Jan 27, 2022

Conversation

atiratree
Copy link
Member

@atiratree atiratree commented Jan 6, 2022 •
edited
Loading

There is sometimes a misunderstanding about the behaviour of Deployments and its Progressing condition [1] [2]. This PR is trying to describe in more detail behaviour and peculiarities of the current implementation.

@ravisantoshgudimetla @soltysh what is your opinion on these changes?

  1. ProgressDeadlineExceeded not set outside of Deployment rollouts kubernetes#106054
  2. Deployment Conditions - Progressing & Available not updated appropriately kubernetes#106697

TODO:

  • go over and update API definitions as well?

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. language/en Issues or PRs related to English language labels Jan 6, 2022
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Jan 6, 2022
@netlify
Copy link

netlify bot commented Jan 6, 2022 •
edited
Loading

✔️ Deploy Preview for kubernetes-io-main-staging ready!

🔨 Explore the source changes: 8f8d2bb

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/61dc98727c750d00083e5250

😎 Browse the preview: https://deploy-preview-31226--kubernetes-io-main-staging.netlify.app

@atiratree atiratree mentioned this pull request Jan 6, 2022
Closed
@sftim
Copy link
Contributor

sftim commented Jan 6, 2022

/sig apps

@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Jan 6, 2022
sftim
sftim reviewed Jan 6, 2022
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Some feedback from a docs style perspective. It's not blocking. I do though want to push back somewhat on introducing more Go-isms into end user docs.

content/en/docs/concepts/workloads/controllers/deployment.md Outdated
Comment on lines 848 to 850
* Type=Progressing
* Status=True
* Reason=NewReplicaSetCreated | FoundNewReplicaSet | ReplicaSetUpdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Although the existing text uses this style, I'd rather not see more of it. How about switching to something like:

Suggested change
* Type=Progressing
* Status=True
* Reason=NewReplicaSetCreated | FoundNewReplicaSet | ReplicaSetUpdated
* `type: Progressing`
* `status: "true"`
* `reason: NewReplicaSetCreated` | `reason: FoundNewReplicaSet` | `reason: ReplicaSetUpdated`

(I think) this is how they'd look in YAML, which is how we usually represent API details. JSON is also OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the Deployment controller also emit an Event? If so, maybe we can mention that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would rather not in this case.

  1. It emits scaling event. Eg. Scaled up replica set test to 1. But this is not reflective of a rollout / deployment and occurs in plain scaling as well.
  2. If you create Deployment with 0 replicas there is no event emitted.

content/en/docs/concepts/workloads/controllers/deployment.md Outdated
@@ -842,6 +842,13 @@ Kubernetes marks a Deployment as _progressing_ when one of the following tasks i
* The Deployment is scaling down its older ReplicaSet(s).
* New Pods become ready or available (ready for at least [MinReadySeconds](#min-ready-seconds)).

When the rollout becomes progressing, the Deployment controller adds a DeploymentCondition with the following
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When the rollout becomes progressing, the Deployment controller adds a DeploymentCondition with the following
When the rollout becomes “progressing”, the Deployment controller adds a condition with
the following

DeploymentCondition isn't part of the Kubernetes API; it's an implementation detail of the Golang implementation.
In theory someone could drop in their own conformant Deployment controller and use that. We should write docs that are still relevant for that case.

content/en/docs/concepts/workloads/controllers/deployment.md Outdated
Comment on lines 866 to 868
* Type=Progressing
* Status=True
* Reason=NewReplicaSetAvailable
Copy link
Contributor

Choose a reason for hiding this comment

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

see previous comment - let's try to show how things would look in the API.

content/en/docs/concepts/workloads/controllers/deployment.md Outdated
@@ -853,6 +860,15 @@ updates you've requested have been completed.
* All of the replicas associated with the Deployment are available.
* No old replicas for the Deployment are running.

When the rollout completes, the Deployment controller sets a DeploymentCondition with the following
Copy link
Contributor

Choose a reason for hiding this comment

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

see previous comment - let's try to show how things would look in the API.

content/en/docs/concepts/workloads/controllers/deployment.md Outdated
* Status=True
* Reason=NewReplicaSetAvailable

This condition will stay `True`, until a new rollout is initiated (even when availability of replicas changes, in that case see `Type=Available` condition).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This condition will stay `True`, until a new rollout is initiated (even when availability of replicas changes, in that case see `Type=Available` condition).
This `Progressing` condition will retain a status value of `"True"` until a new rollout
is initiated. The condition holds even when availability of replicas changes (which
does instead affect the `Available` condition).

?

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 10, 2022
@atiratree
Copy link
Member Author

@sftim Thank you for the review. I have hopefully fixed all the comments.

I have switched the condition references to the YAML format and also applied this formatting to other places in this file.

soltysh
soltysh approved these changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 27, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 707d21e49d5c658bd74afeecec0327d1deb31639

@sftim
Copy link
Contributor

sftim commented Jan 27, 2022

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2022
@k8s-ci-robot k8s-ci-robot merged commit c93f096 into kubernetes:main Jan 27, 2022
@atiratree atiratree mentioned this pull request Apr 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sftim sftim sftim left review comments

@soltysh soltysh soltysh approved these changes

@daminisatya daminisatya Awaiting requested review from daminisatya

Assignees

@soltysh soltysh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@atiratree @sftim @k8s-ci-robot @soltysh