The router should update config immediately on startup

[pweil-]

These changes address the following:

1. The router does not start automatically. A commit was only happen when a watch event was processed. If the watch configuration was wrong then a config is never written and haproxy is never started. A new commit is added to the plugin to write an empty state config file after creating a new router
2. Delay probes for 60 seconds to allow the router time to write the config file (with the new initial commit) and be started before failing containers.

@pmorie PTAL at the plugin changes
@deads2k - this is the PR that I was mentioning in IRC that should help with e2e transient failures
@jimmidyson @sdodson - as discussed on #1575 and #1504 (comment)

[pweil-]

[test]

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1738/)

[smarterclayton]

60 seconds is really long? This points to a fundamental flaw in liveness probes if we have to jack it up that high. Shouldn't we be writing the config before we answer the first liveness probe?

[jimmidyson]

The 60 seconds came from when we weren't writing the config file out until first watch event received - it was a safe number to prevent pod being killed as liveness probe failed. Now this pr writes config before first watch event we should probably be safe to reduce or even remove this delay.

[smarterclayton]

Which reminds me, we probably want two health checks (one a readiness probe on the Openshift router process, one the liveness check on the router ports themselves). The Openshift-router should know when it's ready and report that up to other listeners.

On Apr 7, 2015, at 11:44 PM, Jimmi Dyson notifications@github.com wrote:

The 60 seconds came from when we weren't writing the config file out until first watch event received - it was a safe number to prevent pod being killed as liveness probe failed. Now this pr writes config before first watch event we should probably be safe to reduce or even remove this delay.

—
Reply to this email directly or view it on GitHub.

[pweil-]

I will remove this delay and we can see if we still are seeing transient errors on the e2e since it works fine for me locally. Stand by for update.

I'll create and test a new PR for liveness probe

[pweil-]

@jimmidyson @smarterclayton updated

[smarterclayton]

LGTM [merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1503/) (Image: devenv-fedora_1229)

[openshift-bot]

Evaluated for origin up to 1c6002b

[jimmidyson]

@smarterclayton - how can we get an updated released image for the router once this is merged?

[smarterclayton]

You can use the --latest flag on the master to force newer images for now. I was going to cut a release soon.

----- Original Message -----

@smarterclayton - how can we get an updated released image for the router
once this is merged?

---

Reply to this email directly or view it on GitHub:
#1603 (comment)

[pweil-]

I think we're still going to need an initial delay. I am seeing the killed container behavior in an e2e test I was running to fix a bug I found. I'll give it a test to make sure and open a PR

[smarterclayton]

And we know why we're getting killed, for sure?

On Apr 8, 2015, at 2:01 PM, Paul notifications@github.com wrote:

I think we're still going to need an initial delay. I am seeing the killed container behavior in an e2e test I was running to fix a bug I found. I'll give it a test to make sure and open a PR

—
Reply to this email directly or view it on GitHub.

[jimmidyson]

Damn. I did say we should probably be safe ;) That would really smack of a problem upstream though right with liveness probes being fired too eagerly?

[jimmidyson]

@smarterclayton - previously it was definitely liveness probe from the logs. Not sure if @pweil- is having the same problem now though.

[pweil-]

I need to apply head to desk repeatedly. I need to rebase my branch, I started working on it before this was merged. 😵

Will report back if I can still reproduce it after that

[pweil-]

Still seeing this after a rebase and rebuild of the haproxy image. It is failing in the logs over an over again with a probe failure.

I don't think this is necessarily a probe issue though, I think it is a plugin issue. The template router plugin reads state and and has to process 6 template files before it calls the reload script so any of those items could cause a delay. Adding the .Commit ensured that it had a file pre-watch events but if the probe occurs during or before it is finished processing the templates we're still broken.

@smarterclayton what do you think about this:

1. Provide a default empty config file in the image
2. In the method I changed for adding the commit make a direct call to reload
3. Follow the reload with a Commit call to save state, process templates, reload

Use case 1: brand new router - the default config file should start up instantly
Use case 2: restart of failed router - it should have already overwritten the default config so we're at the mercy of how long haproxy takes to start unless we want to always dump a default over top of the current config in the case of a restart and let item 3 above repopulate it

/cc @pmorie

[pweil-]

Brainstorming a bit more....even if we updated the plugin to be able to write a default config over the existing config in use case 2 (above) we're still at the mercy of how long it takes to read file.default and write file.actual. My suggestion is that we find a reasonable delay (say 10 seconds) that should cover most use cases and use the code with .Commit rather than complicate the plugin for future authors.

[jimmidyson]

Is there a default initial delay for liveness probes? It seems pretty short for the general case if so. Pretty much every java process ever for example is going to have to set initial delay. Is that expected?

[pweil-]

no, not that I could find. The probe returns success until Now - Created > Delay. Once it has passed that mark it probes the container for real.

[pweil-]

Well, between this PR and an InitialDelay of 10 seconds I have been unable to reproduce the probe failure after running e2e 10 times. I'm going to open a PR for that change unless there are any objections or alternate solutions.

[smarterclayton]

It seems weird that probes have to have a delay. The delay might depend on how loaded the host is. I thought there was a better mechanism planned to start this check, but maybe I'm wrong.

----- Original Message -----

no, not that I could find. The probe returns success until Now - Created >
Delay. Once it has passed that mark it probes the container for real.

---

Reply to this email directly or view it on GitHub:
#1603 (comment)

[smarterclayton]

----- Original Message -----

Still seeing this after a rebase and rebuild of the haproxy image. It is
failing in the logs over an over again with a probe failure.

I don't think this is necessarily a probe issue though, I think it is a
plugin issue. The template router plugin reads state and and has to process
6 template files before it calls the reload script so any of those items
could cause a delay. Adding the .Commit ensured that it had a file
pre-watch events but if the probe occurs during or before it is finished
processing the templates we're still broken.

@smarterclayton what do you think about this:

1. Provide a default empty config file in the image

If we did this, that fits the definition of "alive" (it's doing something). However.... if you have a huge set of routes, do you want to report "ready" before those are processed and you are brought into rotation? Probably not.

1. In the method I changed for adding the commit make a direct call to
   reload
2. Follow the reload with a Commit call to save state, process templates,
   reload

Use case 1: brand new router - the default config file should start up
instantly
Use case 2: restart of failed router - it should have already overwritten
the default config so we're at the mercy of how long haproxy takes to start
unless we want to always dump a default over top of the current config in
the case of a restart and let item 3 above repopulate it

It's likely in a restart you want to not signal ready until you can really take traffic (otherwise you'll start rejecting requests randomly). I think in this case haproxy shouldn't even come up until the config is in place (although if you have the last config, it's probably ok to load it).

/cc @pmorie

---

Reply to this email directly or view it on GitHub:
#1603 (comment)