FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(scheduler): rollback scale / deploy when the desired number of pods can not be brought up in a timely manner by helgi · Pull Request #792 · deis/controller · GitHub

This repository was archived by the owner on May 6, 2020. It is now read-only.
/ controller Public archive

fix(scheduler): rollback scale / deploy when the desired number of pods can not be brought up in a timely manner #792

Closed
helgi wants to merge 1 commit into from
Conversation 20 Commits 1 Checks 0 Files changed
Closed

fix(scheduler): rollback scale / deploy when the desired number of pods can not be brought up in a timely manner #792

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

Filter by extension

Filter by extension .py  (1) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
6 changes: 5 additions & 1 deletion rootfs/scheduler/__init__.py
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -1010,7 +1010,11 @@ def _wait_until_pods_are_ready(self, namespace, containers, labels, desired, tim
if waited > timeout:
self.log(namespace, 'timed out ({}s) waiting for pods to come up in namespace {}'.format(timeout, namespace)) # noqa

self.log(namespace, "{} out of {} pods are in service".format(count, desired)) # noqa
self.log(namespace, "{} out of {} pods are in service".format(count, desired))
if count != desired:
# raising to allow operations to rollback
raise KubeException('Not enough pods in namespace {} came into service. '
'{} out of {}'.format(namespace, count, desired))

kmala Jun 7, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

it would be better if we can get the reason why the pod din't come to service than just saying not enough pods

helgi Jun 7, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, however in the current setup this is not bubbled up and rather a message in the deploy function is used.

I can look into improving the message but I didn't want to change the deploy command since it handles more than just this failure

helgi Jun 7, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@kmala do you have any suggestions what information we could bubble up? Note that this can be more than one pod that can fail as we do deploy batches now

kmala Jun 7, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If the pods are in a crash loop back/if they are atleast started the pods logs can be anyway found using the deis logs....i just want to see the possibility if we can get the reason for pods not starting in other scenarios may be getting it from the events.The reason we might want to show is because users may be in confused state as in why the release or scale failed.
Also i have one more question regarding this change....do we want to think the operation as failure even if just the readiness probe failed because it might be waiting for some external dependency?

helgi Jun 7, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Okay, that's fair. Most people will be able to use deis ps to do diagnostics but maybe there is a way to get more info into the Exception. Only issue could be overloading the Exception message with a lot of information since you might be deploying a batch of 10 Pods and they all fail.

Before I (personally) didn't see it as a failure but as seen in #706 and #785 that not treating this as a failure is causing problems. The scenario here is this:

I have many releases done via Deis Workflow, I'm at v28 - I do some config setting and v29 kicks off. The Pods for v29 never come up and the timeout kicks in (when waiting for the pods to be ready) - After the timeout the deploy system is made to just go start scaling down v28

At the end of all of that I lost v28 and v29 is all in a Pending state. Now my business is down.

Because of the above I've started considering this a failure state. If your readiness probe is still waiting for external dependencies then you should probably increase HEALTHCHECK_INITIAL_DELAY - I have also considered making the 2 minute wait for pod timeout as something people can configure, in addition to the INITIAL_DELAY

Does that help explain why I'm opting for failure mode?

helgi Jun 7, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That's a good point - Until we have both liveness and readiness as independently configurable then we will have to consider failure in either as a failure mode.

Even if we let people configure both then I think it is a bit of a problem if your readiness is still working but is beyond the timeout, we can't really start spinning working stuff down until we have enough pods to keep the app running. That's the biggest issue - Suppose we could add time to the timeout when we detect it is in an almost ready state, but only up to a point

kmala Jun 9, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I just saw that we hard coding the timeout to 2 minutes https://github.com/deis/controller/pull/792/files#diff-3b6e1f09121075ad67f216aa2bbfc9a6L942
shouldn't that be configurable as some deploys take more than 2 mins?

helgi Jun 9, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That's something that has been around for a longgggg time (probably v1 days) - That's just the waiting aspect for each scale operation, of which can be many in a single deploy. So far HEALTHCHECK_INITIAL_DELAY can extend that timeout tho. So if you have a delay of 120 seconds then it is a total of 240 seconds timeout per scale operation.

kmala Jun 9, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yes i know that it has been hardcoded since its inception but now that we are starting to fail the operation based on the timeout i think we should start allowing the user to configure it failing which it might become problem for them....since HEALTHCHECK_INITIAL_DELAY is not mandatory to be set for every pod we shouldn't depend on it

helgi Jun 9, 2016 •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I agree on doing that but lets do it as part of a different PR. Can you open a ticket and I will tackle it this week?

def _scale_rc(self, namespace, name, desired, timeout):
rc = self.get_rc(namespace, name).json()
Expand Down
Back | FazBrowse Home | New Git URL