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

Add docs for perma-failed Deployments #1699

Merged
merged 4 commits into from
Dec 8, 2016
Merged

Add docs for perma-failed Deployments #1699

merged 4 commits into from
Dec 8, 2016

Conversation

0xmichalis
Copy link
Contributor

@0xmichalis 0xmichalis commented Nov 16, 2016

cc: @janetkuo @kubernetes/docs

Docs for kubernetes/enhancements#122


This change is Reviewable

soon as a failure condition is noticed.

Another thing to note is transient errors eg. your rollout times out and a `ProgressDeadlineExceeded` condition is set to
your Deployment because don't have enough quota. All you need is to scale down your Deployment, or scale down other Pods
Copy link
Member

Choose a reason for hiding this comment

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

You can't scale down pods. You can scale down other controllers or delete pods.

Copy link
Member

Choose a reason for hiding this comment

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

because don't have enough quota --> because of insufficient quota

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

application runtime misconfiguration. In those cases you can specify a timeout parameter in the spec
(`spec.progressDeadlineSeconds`) that denotes the seconds you want to wait for your Deployment to report any progress.
Once the deadline is exceeded, meaning your Deployment will not report any progress for more than progressDeadlineSeconds,
the deployment controller will add a Progressing condition with Status=False and Reason=ProgressDeadlineExceeded in the
Copy link
Member

Choose a reason for hiding this comment

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

Suggest quoting things like "Status=False" with `

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

the status of the condition into a succeeded one (Status=True, Reason=NewReplicaSetAvailable).

A complete Deployment for the controller is one that:
* has minimum availability, meaning its available replicas are more than those required by the Deployment strategy (`status.
Copy link
Member

Choose a reason for hiding this comment

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

The bullet point isn't rendered well. I guess adding a new line will fix it

Copy link
Member

Choose a reason for hiding this comment

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

Remove the whitespace after status.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

* has minimum availability, meaning its available replicas are more than those required by the Deployment strategy (`status.
availableReplicas >= spec.replicas - maxUnavailable` where `maxUnavailable` is the maximum unavailable replicas that a
Deployment allows at any point in time. It depends on the deployment strategy: Recreate deployments don't use it whereas
Rolling deployments specify it in `spec.strategy.rollingUpdate.maxUnavailable`).
Copy link
Member

Choose a reason for hiding this comment

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

Rolling Update deployments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I like the distinction (why not RecreateUpdate also then instead of Recreate or Rolling instead of RollingUpdate?) but I wenr ahead and updated it.

availableReplicas >= spec.replicas - maxUnavailable` where `maxUnavailable` is the maximum unavailable replicas that a
Deployment allows at any point in time. It depends on the deployment strategy: Recreate deployments don't use it whereas
Rolling deployments specify it in `spec.strategy.rollingUpdate.maxUnavailable`).
* has all of its replicas updated to the latest revision (`spec.replicas == status.updatedReplicas`).
Copy link
Member

Choose a reason for hiding this comment

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

Can we make it more clear on what a complete Deployment means: for rolling update deployments, both points should be true; for recreate ones, only the second one needs to be true (the first one doesn't apply)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first point definitely applies for recreate deployments - in order to have a complete deployment you have to have all pods available, right?

in the status of the Deployment.

Progress for a Deployment is:
* the creation of the new replica set.
Copy link
Member

Choose a reason for hiding this comment

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

same here for the bullet point render issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

```

If you describe the Deployment, you will notice a Conditions section:
```
Copy link
Member

Choose a reason for hiding this comment

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

This part isn't rendered well either. Maybe a newline before ``` would fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok


This is how the status of the Deployment looks like if you would run `kubectl get deployment nginx-deployment -o yaml`
(spec of the object is omitted for brevity):
```
Copy link
Member

Choose a reason for hiding this comment

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

render issue again

There will be times your Deployment will get stuck trying to deploy its newest ReplicaSet without ever completing,
due to insufficient quota, readiness probe failures, image pull error, insufficient permissions, limit ranges, or any
application runtime misconfiguration. In those cases you can specify a timeout parameter in the spec
(`spec.progressDeadlineSeconds`) that denotes the seconds you want to wait for your Deployment to report any progress.
Copy link
Member

Choose a reason for hiding this comment

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

link to the section below for spec.progressDeadlineSeconds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -556,6 +675,15 @@ the rolling update starts, such that the total number of old and new Pods do not
the new Replica Set can be scaled up further, ensuring that the total number of Pods running
at any time during the update is at most 130% of desired Pods.

### Progress Deadline Seconds

`.spec.progressDeadlineSeconds` is an optional field that specifies the number of seconds you want
Copy link
Member

Choose a reason for hiding this comment

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

Suggest linking back to the "failed deployment" section somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@janetkuo
Copy link
Member

We can add sub-sections in "failed deployment"

`Reason=ProgressDeadlineExceeded` in the status of the Deployment. For more information on status conditions, see
[here](https://github.com/kubernetes/kubernetes/blob/{{page.githubbranch}}/docs/devel/api-conventions.md#typical-status-properties).

Note that at the moment, Kubernetes will do nothing more than just report with a condition with `Reason=ProgressDeadlineExceeded`.
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not talk about "future features" in our technical docs. It makes tracking changes really hard. Please change this paragraph, or at least remove the sentence starting with "In the future..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -454,6 +454,133 @@ nginx-deployment-3066724191 0 0 1h
Note: You cannot rollback a paused Deployment until you resume it.


## Failed Deployment

There will be times your Deployment will get stuck trying to deploy its newest ReplicaSet without ever completing,
Copy link
Contributor

Choose a reason for hiding this comment

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

Reword to:

"Your Deployment may get stuck trying to deploy its newest ReplicaSet without ever completing, due to:

  • Insufficient quota
  • readiness probe failures
  • image pull error
  • insufficient permissions
  • limit ranges
  • application runtime misconfigurations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

is set to your Deployment because of insufficient quota. All you need is to scale down your Deployment, or scale down other
controllers you may be running, or increase quota in your namespace, either by asking your administrator or by doing it yourself
assuming you have the right permissions. Once the deployment controller notices that your Deployment is complete, it will
transition the status of the Progressing condition into a succeeded one (`Status=True` and `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.

The sentence "All you need..." is very difficult to read. Suggest breaking it into two: "To fix this issue, scale down your Deployment, scale down other controllers you may be running, or increase quota in your namespace. You can do this by either by asking your administrator or doing it yourself assuming you have the right permissions."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@jaredbhatti
Copy link
Contributor

Please fix the open docs issues.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 28, 2016
@0xmichalis
Copy link
Contributor Author

@jaredbhatti thanks for the review, addressed all of your comments.

Your Deployment may get stuck trying to deploy its newest ReplicaSet without ever completing, due to:

* insufficient quota

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the whitespace from in between each bullet, and capitalize the first letter of each.


* application runtime misconfiguration

In those cases you can specify a timeout parameter in the spec ([spec.progressDeadlineSeconds](#progress-deadline-seconds))
Copy link
Contributor

Choose a reason for hiding this comment

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

Comma after "cases"

* application runtime misconfiguration

In those cases you can specify a timeout parameter in the spec ([spec.progressDeadlineSeconds](#progress-deadline-seconds))
that denotes the seconds you want to wait for your Deployment to report any progress. Once the deadline is exceeded, meaning
Copy link
Contributor

Choose a reason for hiding this comment

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

"the seconds you want to wait" -> "some number of seconds to wait for your Deployment to report and progress."

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, put in a paragraph break after "progress"; you need to insert the text from line 509 afterward.


In those cases you can specify a timeout parameter in the spec ([spec.progressDeadlineSeconds](#progress-deadline-seconds))
that denotes the seconds you want to wait for your Deployment to report any progress. Once the deadline is exceeded, meaning
your Deployment will not report any progress for more than progressDeadlineSeconds, the deployment controller will add a
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this sentence is trying to say. Did you get stuck between drafts/edits? I suggest this rewording:

"Once the deadline has been exceeded, the Deployment controller adds the following attributes to the Deployment's status:

  • Type=Progressing
  • Status=False
  • Reason=ProgressDeadlineExceeded

See the Kubernetes API conventions for more information on status conditions.

your Deployment will not report any progress for more than progressDeadlineSeconds, the deployment controller will add a
condition with `Type=Progressing`, `Status=False`, and `Reason=ProgressDeadlineExceeded` in the status of the Deployment.
For more information on status conditions, see [here](https://github.com/kubernetes/kubernetes/blob/{{page.githubbranch}}/docs/devel/api-conventions.md#typical-status-properties).
Note that at the moment, Kubernetes will do nothing more than just report with a condition with `Reason=ProgressDeadlineExceeded`.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Note that in version 1.5, Kubernetes will take no action on a stalled Deployment other than to report a status condition with Reason=ProgressDeadlineExceeded."


* scaling down old pods.

To make the controller report lack of progress for a Deployment after 10 minutes:
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines from 509 to 513 are the most important part of the entire section. They need to be moved up to the top of the section, immediately after the word "progress" on line 474.

"nginx-deployment" patched
```

If you describe the Deployment, you will notice a Conditions section:
Copy link
Contributor

Choose a reason for hiding this comment

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

This section is about how to detect and deal with a stalled Deployment. It isn't necessary to explain what a successful deployment looks like first. Remove everything from between lines 516 and 530.

required new replicas are available (see the Reason of the condition for the particulars - in our case
`Reason=NewReplicaSetAvailable` means that the deployment is complete).

For example, if you have insufficient pod quota and your Deployment tries to create more pods than allowed then
Copy link
Contributor

Choose a reason for hiding this comment

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

Move everything from line 532 to line 541 up, above line 477.

Note that at the moment, Kubernetes will do nothing more than just report with a condition with `Reason=ProgressDeadlineExceeded`.

Another thing to note is transient errors eg. your rollout times out and a condition with `Reason=ProgressDeadlineExceeded`
is set to your Deployment because of insufficient quota. To fix this issue, scale down your Deployment, or scale down other
Copy link
Contributor

Choose a reason for hiding this comment

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

Paragraph break after "quota." Insert the following:

"You can check to see if your Deployment has encountered quota problems by inspecting the Deployment with kubectl get Deployment."

Then move everything from line 544 to 572 underneath that sentence.

unavailableReplicas: 2
```

Eventually, once the Deployment progress deadline is exceeded, the status and reason of the Progressing condition
Copy link
Contributor

Choose a reason for hiding this comment

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

Everything from line 575 to line 591 needs to be moved up to line 475.

@0xmichalis
Copy link
Contributor Author

@devin-donnelly thanks for the review, I think I have addressed most if not all of your comments, ptal

required new replicas are available (see the Reason of the condition for the particulars - in our case
`Reason=NewReplicaSetAvailable` means that the deployment is complete).

A complete Deployment is one that:
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't think this section belongs here -- Everything between lines 567-585. Why are we describing a complete deployment, and deployment progress, in a section on failed deployments? I think this is either described elsewhere, or should be.

All you need is the last paragraph (line 587 to line 591) under a subheading called "Operating on a failed deployment".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that this section is the only one that mentions complete deployments. My reasoning for having it here was that you need to describe both concepts together in order to make them understood (what's a complete deployment, what's a failed deployment). That being said I am going to create a separate section for complete deployments prior to failed deployments.

@devin-donnelly
Copy link
Contributor

devin-donnelly commented Dec 2, 2016

@Kargakis, One more structural change. It's much better now, thanks.

@devin-donnelly
Copy link
Contributor

@janetkuo , are you doing the tech review on this one?


### Progressing Deployment

Progress for a Deployment is:
Copy link
Contributor

Choose a reason for hiding this comment

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

"If a Deployment is progressing, that means it is performing one of the following tasks:"


Progress for a Deployment is:

* the creation of the new replica set.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Creating a new replica set"

Copy link
Member

Choose a reason for hiding this comment

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

Should use ReplicaSet instead of replica set, based on the latest style guide http://kubernetes.io/docs/contribute/style-guide/#use-camel-case-for-api-objects

Progress for a Deployment is:

* the creation of the new replica set.
* scaling up new pods.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Scaling up new pods"

Copy link
Member

Choose a reason for hiding this comment

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

Only ReplicaSets can be scaled up/down


* the creation of the new replica set.
* scaling up new pods.
* scaling down old pods.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Scaling down old pods"


### Complete Deployment

A complete Deployment is one that:
Copy link
Contributor

Choose a reason for hiding this comment

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

"If a Deployment is complete, it has the following characteristics:"


A complete Deployment is one that:

* has minimum availability, meaning its available replicas are more than those required by the Deployment strategy
Copy link
Contributor

Choose a reason for hiding this comment

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

"* The Deployment has minimum availability. Minimum availability means that the Deployment's number of available replicas exceeds the number required by the Deployment strategy."

The rest is way too long for a bullet point, and I think it's not really necessary.

Copy link
Member

Choose a reason for hiding this comment

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

"equals or exceeds" (>=, not >)

(`status.availableReplicas >= spec.replicas - maxUnavailable` where `maxUnavailable` is the maximum unavailable replicas
that a Deployment allows at any point in time. It depends on the deployment strategy: Recreate deployments don't use it whereas
Rolling Update deployments specify it in [spec.strategy.rollingUpdate.maxUnavailable](#max-unavailable)).
* has all of its replicas updated to the latest revision (`spec.replicas == status.updatedReplicas`).
Copy link
Contributor

Choose a reason for hiding this comment

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

"* All of the replicas associated with the Deployment have been updated to the latest version you've specified, meaning any updates you've requested have been completed."

Rolling Update deployments specify it in [spec.strategy.rollingUpdate.maxUnavailable](#max-unavailable)).
* has all of its replicas updated to the latest revision (`spec.replicas == status.updatedReplicas`).

Note that clients should always take into account generation/observedGeneration for ensuring that the latest state of a
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what exactly this paragraph is trying to say. It needs some major revisions.

  • Who are "clients" in this case? Users? If so, address the user directly as "you."
  • What is generation/observedGeneration? Who provides the observedGeneration? Looks like a status field, so I guess Kubernetes provides that; what is generation?
  • By "latest state of a Deployment has been observed by the controller", do you mean "What the user sees in kubectl actually reflects reality?" I'm having trouble parsing what this sentence means.

Copy link
Contributor Author

@0xmichalis 0xmichalis Dec 6, 2016

Choose a reason for hiding this comment

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

Who are "clients" in this case? Users? If so, address the user directly as "you."

Client is anything that talks to the api server; a user that uses {kubectl, external client, directly via the API} or a controller (either internal, or external set up by an admin).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is generation/observedGeneration? Who provides the observedGeneration? Looks like a status field, so I guess Kubernetes provides that; what is generation?

generation can be found in the metadata of an object, and some objects also implement observedGeneration in the status. Generation is updated every time the spec of the object is updated (ie. user changes something) and observedGeneration is updated by the respective controller of the object (controller observes the change that was done by the user).

By "latest state of a Deployment has been observed by the controller", do you mean "What the user sees in kubectl actually reflects reality?" I'm having trouble parsing what this sentence means.

Once a client (again, either user or a controller) observes that metadata.generation == status.observedGeneration, then they can be sure that the current spec of the object has been observed by its controller).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this section in favor of a TODO

@devin-donnelly
Copy link
Contributor

I like the new section organization; I have some revisions/comments to the text in the new sections.

@devin-donnelly
Copy link
Contributor

Thanks, @Kargakis . I'm just going to do some minor cleanup and get it assigned for tech review, and we'll be ok to merge.

* Limit ranges
* Application runtime misconfiguration

In those cases, you can specify a timeout parameter in the spec ([spec.progressDeadlineSeconds](#progress-deadline-seconds))
Copy link
Member

Choose a reason for hiding this comment

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

use `` to quote spec.progressDeadlineSeconds

Copy link
Member

Choose a reason for hiding this comment

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

This sounds like when a Deployment failed, you need to specify a timeout for it. But we actually want the users to specify it beforehand.

Rephrase it so that it's clear that users can use this timeout parameter to specify how the system should determine whether a Deployment is considered "failed to progress".

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in my own commit.

equals or exceeds the number required by the Deployment strategy.
* All of the replicas associated with the Deployment have been updated to the latest version you've specified, meaning any
updates you've requested have been completed.

Copy link
Member

Choose a reason for hiding this comment

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

Add a line here for how to easily determine a Deployment is completed (such as from the output of kubectl rollout status)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have already added such a sentense in the previous section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an additional line here that also points out the exit code of rollout status in case the Deployment completes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added grammar fix.

In those cases, you can specify a timeout parameter in the spec ([spec.progressDeadlineSeconds](#progress-deadline-seconds))
that denotes the number of seconds to wait for your Deployment to report any progress.

The following `kubectl` command sets the spec with `progressDeadlineSeconds` to make the controller report lack of progress for a Deployment after 10 minutes:
Copy link
Member

Choose a reason for hiding this comment

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

link to progressDeadlineSeconds below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We link to there just above?

Copy link
Contributor

Choose a reason for hiding this comment

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

Link at first mention is fine; doesn't need to be linked at every mention.

```

Once the deadline has been exceeded, the Deployment controller adds a DeploymentCondition with the following attributes to
the Deployment's status:
Copy link
Member

Choose a reason for hiding this comment

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

Should we make it clear that it's under .status.conditions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Added code formatting.


See the [Kubernetes API conventions](https://github.com/kubernetes/kubernetes/blob/{{page.githubbranch}}/docs/devel/api-conventions.md#typical-status-properties) for more information on status conditions.

Note that in version 1.5, Kubernetes will take no action on a stalled Deployment other than to report a status condition with
Copy link
Member

Choose a reason for hiding this comment

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

What does "take no action" mean? Please clarify it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will do nothing more than update the status.condition with type=Progressing to Status=False, Reason=ProgressDeadlineExceeded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't "other than ..." clear enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so.


### Operating on a failed deployment

All actions that apply to a complete Deployment also apply to a failed Deployment. You can scale it up/down, rollback
Copy link
Member

Choose a reason for hiding this comment

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

roll back


All actions that apply to a complete Deployment also apply to a failed Deployment. You can scale it up/down, rollback
to a previous revision, or even pause it if you need to apply multiple tweaks in the Deployment pod template. Note
that progress for a Deployment is not estimated while the Deployment is paused so you can safely pause a Deployment
Copy link
Member

Choose a reason for hiding this comment

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

"progress for a Deployment is not estimated" would you clarify this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The controller wont check if the Deployment failed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This actually belongs way higher in the document; it's kind of arbitrary here. Fixed in my own commit.

[failed progressing](#failed-deployment) - surfaced as a condition with `Type=Progressing`, `Status=False`.
and `Reason=ProgressDeadlineExceeded` in the status of the resource. The deployment controller will keep
retrying the Deployment. In the future, once automatic rollback will be implemented, the deployment
controller will rollback a Deployment as soon as it observes such a condition. If specified, this field
Copy link
Member

Choose a reason for hiding this comment

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

roll back

and `Reason=ProgressDeadlineExceeded` in the status of the resource. The deployment controller will keep
retrying the Deployment. In the future, once automatic rollback will be implemented, the deployment
controller will rollback a Deployment as soon as it observes such a condition. If specified, this field
needs to be greater than `.spec.minReadySeconds`.
Copy link
Member

Choose a reason for hiding this comment

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

Is this line related to the previous line about auto rollback? If not, separate them (move auto rollback to the end); if so, make it clear that they're relevant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not.

@devin-donnelly
Copy link
Contributor

Hi @Kargakis , would you mind having a look at Janet's feedback from a technical perspective? We're right up against our deadline for 1.5, and we need to get this merged ASAP. Thanks.

@0xmichalis
Copy link
Contributor Author

Hi @devin-donnelly @janetkuo I believe I have addressed most if not all comments, ptal

@devin-donnelly devin-donnelly merged commit fe46050 into kubernetes:release-1.5 Dec 8, 2016
@0xmichalis 0xmichalis deleted the progressdeadlineseconds branch December 8, 2016 22:37
@0xmichalis
Copy link
Contributor Author

@janetkuo when will these changes land in the master branch?

@janetkuo
Copy link
Member

janetkuo commented Dec 9, 2016

@Kargakis yes we'll merge the change in 1.5 branch into master after 1.5 launch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants