Improve ipfailover configurability #11644

pecameron · 2016-10-28T14:57:37Z

Openshift 3.5 feature.

Add options to 'oadm ipfailover' to configure the check script and
notify scripts and also control the period the check script runs.

Keepalived periodically checks whether the application is running properly.
In the default case the test is a simple verification that something is listening
on the watch port. This PR permits the user to supply an additional check script
that is run in the ipfailover container context to verify that the application is
operating properly. For example, a web server can be tested by accessing the
watch port and verifying the response.

Whenever a node changes state to MASTER, BACKUP, or FAULT a notify
script can be called. This script has 3 parameters filled in by keepalived:
$1 - "GROUP"|"INSTANCE"
$2 - name of group or instance
$3 - target state of transition ("MASTER"|"BACKUP"|"FAULT")

--check-script="check_script"
The check script is a script in the keepalived container that verifies
the service is running properly. The script must return 0 for OK and 1 for FAIL.
These checks are in addition to verifying that the watch port is listening.

--notify-script="notify_script"
The notify script is a script in the keepalived container that is
called whenever the keepalived state transitions to (MASTER|BACKUP|FAULT)

--check-interval=
The check script is run every seconds. Default is 2.

Note: the scripts name is the full path to the script.

Fixes bug 1362163
https://trello.com/c/228zu7Br/267-5-improve-the-configurability-of-the-ipfailover-container-operations

Signed-off-by: Phil Cameron [email protected]

smarterclayton · 2016-10-28T14:59:49Z

Is this a check script for the other container, or for the processes in the keepalived container itself?

pecameron · 2016-10-28T14:59:50Z

@knobunc Here is the first draft. It covers the wants in the trello card. PTAL

smarterclayton · 2016-10-28T15:00:22Z

The descriptions in the PR aren't enough for me to understand what the scripts are supposed to do. More detail in the doc.

stevekuznetsov

I think it is too late for this, but generating a script with a script ... there's got to be a better way.

stevekuznetsov · 2016-10-28T15:04:27Z

images/ipfailover/keepalived/conf/settings.sh

+# When the DC supplies an (non null) iptables chain
+# (OPENSHIFT_HA_IPTABLES_CHAIN) make sure the rule to pass keepalived
+# multicast (224.0.0.18) traffic is in the table.
+HA_IPTABLES_CHAIN=${OPENSHIFT_HA_IPTABLES_CHAIN:-""}


On this line and all the others:

HA_IPTABLES_CHAIN="${OPENSHIFT_HA_IPTABLES_CHAIN:-""}" ^ ^

@stevekuznetsov I shifted OPENSHIFT_HA_IPTABLES_CHAIN to here to be more consistent.
I didn't create this file and am just adding a few fields. I am not sure how far to go with this.

This is work in progress. Thanks for your comments and I will modify the code as suggested. I am not sure how far I should go beyond adding this feature. Three is discussion on converting this to a go program and using the go template to generate the file. However, that is future work (if we decide to do it.)

The scripting here generates the config file for keepalived.

stevekuznetsov · 2016-10-28T15:05:02Z

images/ipfailover/keepalived/lib/config-generators.sh

@@ -10,7 +10,7 @@ source "$(dirname "${BASH_SOURCE[0]}")/utils.sh"

 # Constants.
 readonly CHECK_SCRIPT_NAME="chk_${HA_CONFIG_NAME//-/_}"
-readonly CHECK_INTERVAL_SECS=2
+readonly CHECK_INTERVAL_SECS=${HA_CHECK_INTERVAL}


readonly CHECK_INTERVAL_SECS="${HA_CHECK_INTERVAL}" ^ ^

stevekuznetsov · 2016-10-28T15:05:51Z

images/ipfailover/keepalived/lib/config-generators.sh

@@ -59,6 +59,11 @@ function generate_script_config() {
  if [ "$port" = "0" ]; then
    echo "   script \"true\""
  else
+    if [[ -n $HA_CHECK_SCRIPT ]]; then


if [[ -n "${HA_CHECK_SCRIPT}" ]]; then ^ ^ ^^

stevekuznetsov · 2016-10-28T15:06:43Z

images/ipfailover/keepalived/lib/config-generators.sh

@@ -59,6 +59,11 @@ function generate_script_config() {
  if [ "$port" = "0" ]; then
    echo "   script \"true\""
  else
+    if [[ -n $HA_CHECK_SCRIPT ]]; then
+      echo "   if [[ -f $HA_CHECK_SCRIPT ]]; then"


Are you generating a script here? This is pretty insane. This expansion of $HA_CHECK_SCRIPT will happen at echo time, not at runtime.

@stevekuznetsov This is creating a config file for keepalived. One of the parameters to keepalived is the name of a check script that keepalived should call. The expansion must happen when the config file is created.

stevekuznetsov · 2016-10-28T15:07:04Z

images/ipfailover/keepalived/tests/verify_failover_image.sh

@@ -1,5 +1,6 @@
 #!/bin/bash -e

+# set -x


pecameron · 2016-10-28T15:41:17Z

@smarterclayton - I added more to the description. See the trelo card and bug
https://trello.com/c/228zu7Br/267-5-improve-the-configurability-of-the-ipfailover-container-operations

pecameron · 2016-11-09T21:43:58Z

@stevekuznetsov Is there more information you need here? Are there any further changes needed?

stevekuznetsov · 2016-11-10T14:33:08Z

images/ipfailover/keepalived/conf/settings.sh

@@ -65,7 +89,7 @@ ADMIN_EMAILS=(${OPENSHIFT_HA_ADMIN_EMAILS:-"root@localhost"})
 EMAIL_FROM="[email protected]"

 #  IP address of the SMTP server.
-SMTP_SERVER=${OPENSHIFT_HA_SMTP_SERVER:-"127.0.0.1"}
+SMTP_SERVER="${OPENSHIFT_HA_SMTP_SERVER:-"127.0.0.A1"}"


@stevekuznetsov Oops, I missed that. Its now fixed.

stevekuznetsov

One typo, the rest of the Bash looks ... ok ... to me. I think it would be reasonable to open a card or an issue to investigate if we can achieve the same features without generating a Bash script from within a Bash script.

pecameron · 2016-11-18T14:00:22Z

@stevekuznetsov You have "Changes requested". What additional changes would you like to have made?

stevekuznetsov · 2016-11-18T14:07:34Z

@pecameron There is one more comment about an IP address #11644 (review)

I would also like to see if we can open an issue to investigate an alternative approach to this than generating a script from within a script.

knobunc · 2016-11-18T14:11:45Z

@stevekuznetsov Yeah, @pecameron already talked to me about the possibility of changing over to a template file, or something less horrific. We opened the card https://trello.com/c/9w6pIkIU

pecameron · 2016-11-18T14:30:20Z

@stevekuznetsov @knobunc This code is generating the keepalived config file. keepalived reads this file to configure itself. It may look like a script but it is the config file. Even if we move to a template, the resulting file will still look the same as this file. It will look like a script and it will still be the same config file.

OK, I am missing something here. Do we want to change keepalived to accept its configuration in a different format?

knobunc · 2016-11-18T14:47:38Z

@pecameron Longer-term, we probably want to move it to go with a template. But that will be done as a feature later.

pecameron · 2016-11-18T15:06:37Z

@stevekuznetsov I made the changes. PTAL

stevekuznetsov · 2016-11-18T15:26:09Z

I do admit I'm not well versed on this but diffs like this are clearly writing Bash code into a file somewhere. Whether that's a Bash script to be run later or a config file that allows for Bash inline... we're generating executable Bash code dynamically. I feel like that is a very fragile approach, and, yes, it should be something that we think about later, it does not block this PR.

pecameron · 2016-11-18T21:19:52Z

@knobunc There is 1 approved on this, Is there something to do before you can approve it as well? Is this ready to merge?

knobunc · 2016-11-18T22:05:35Z

@pecameron it is ready to merge. I'm giving master until Monday, then I have a few to put in.

knobunc

LGTM

knobunc · 2016-11-21T16:18:08Z

[test]

knobunc · 2016-11-22T13:45:38Z

Failed:

In suite "github.com/openshift/origin/test/cmd/router", test case "test/cmd/router.sh:75: executing 'oadm ipfailover --virtual-ips=\"1.2.3.4\" --check-interval=1177 --dry-run -o yaml' expecting success and text 'value" failed:
=== BEGIN TEST CASE ===
test/cmd/router.sh:75: executing 'oadm ipfailover --virtual-ips="1.2.3.4" --check-interval=1177 --dry-run -o yaml' expecting success and text 'value: 1177'
FAILURE after 0.554s: test/cmd/router.sh:75: executing 'oadm ipfailover --virtual-ips="1.2.3.4" --check-interval=1177 --dry-run -o yaml' expecting success and text 'value: 1177': the output content test failed
Standard output from the command:

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_check/8687/consoleText

stevekuznetsov · 2016-11-22T13:56:00Z

test/cmd/router.sh

@@ -72,6 +72,9 @@ os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --dry-
 os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --dry-run -o name' 'deploymentconfig/ipfailover'
 os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --dry-run -o yaml' '1.2.3.4'
 os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --iptables-chain="MY_CHAIN" --dry-run -o yaml' 'value: MY_CHAIN'
+os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --check-interval=1177 --dry-run -o yaml' 'value: 1177'


Stronger checks here, if possible, would look like:

$ oadm ipfailver .... # create an API object $ oc get <object> -o jsonpath='{.path.to.check-interval}' # expecting 1177

Not sure if this is possible or how the ipfailover configuration is serialized in etcd, but that sort of check is more declarative and less fragile than grepping through some YAML.

pecameron · 2016-11-22T13:58:22Z

I don't get this. The yaml does have 1177 and it didn't match. Do you
know what is wrong with this? Could it be the " " around the 1177?

FAILURE after 0.554s: test/cmd/router.sh:75: executing 'oadm ipfailover 
--virtual-ips="1.2.3.4" --check-interval=1177 --dry-run -o yaml' 
expecting success and text 'value: 1177': the output content test failed
Standard output from the command:
apiVersion: v1
items:
- apiVersion: v1
   kind: ServiceAccount
   metadata:
     creationTimestamp: null
     name: ipfailover
- apiVersion: v1
   kind: DeploymentConfig
   metadata:
     creationTimestamp: null
     labels:
       ipfailover: ipfailover
     name: ipfailover
   spec:
     replicas: 1
     strategy:
       resources: {}
       type: Recreate
     template:
       metadata:
         creationTimestamp: null
         labels:
           ipfailover: ipfailover
       spec:
         containers:
         - env:
           - name: OPENSHIFT_HA_CHECK_INTERVAL
             value: "1177" <<<<<<<<<<--------- It is here
           - name: OPENSHIFT_HA_CHECK_SCRIPT

stevekuznetsov · 2016-11-22T14:01:59Z

Yes, it must be exactly that -- you are looking for the literal text value: 1177 in the YAML, but it does not exist. This is why I made the comment that I did about how to make this test more robust.

stevekuznetsov · 2016-11-22T14:03:12Z

In addition, you're not checking that it's the check interval value that is set to 1177, nor that it isn't set to 11775, or that not-value is set to 1177... etc

pecameron · 2016-11-22T14:04:23Z

Steve,
How do I code the line in the test?
oadm ipfailover --virtual-ips="1.2.3.4" --check-interval=1177 --dry-run
-o yaml' 'value: 1177'
would
oadm ipfailover --virtual-ips="1.2.3.4" --check-interval=1177 --dry-run
-o yaml' 'value: "1177"'

What do you think?

phil

=== BEGIN TEST CASE ===
test/cmd/router.sh:75: executing 'oadm ipfailover
--virtual-ips="1.2.3.4" --check-interval=1177 --dry-run -o yaml'
expecting success and text 'value: 1177'
FAILURE after 0.554s: test/cmd/router.sh:75: executing 'oadm ipfailover
--virtual-ips="1.2.3.4" --check-interval=1177 --dry-run -o yaml'
expecting success and text 'value: 1177': the output content test failed
Standard output from the command:
apiVersion: v1
items:
...

apiVersion: v1
kind: DeploymentConfig
...
- env:
- name: OPENSHIFT_HA_CHECK_INTERVAL
value: "1177" <<<<<---------
- name: OPENSHIFT_HA_CHECK_SCRIPT
- name: OPENSHIFT_HA_CONFIG_NAME

On 11/22/2016 08:56 AM, Steve Kuznetsov wrote:

@stevekuznetsov commented on this pull request.

In test/cmd/router.sh
#11644 (review):

@@ -72,6 +72,9 @@ os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --dry-
os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --dry-run -o name' 'deploymentconfig/ipfailover'
os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --dry-run -o yaml' '1.2.3.4'
os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --iptables-chain="MY_CHAIN" --dry-run -o yaml' 'value: MY_CHAIN'
+os::cmd::expect_success_and_text 'oadm ipfailover --virtual-ips="1.2.3.4" --check-interval=1177 --dry-run -o yaml' 'value: 1177'

Stronger checks here, if possible, would look like:

$ oadm ipfailver ....# create an API object
$ oc get -o jsonpath='{.path.to.check-interval}' # expecting 1177

Not sure if this is possible or how the ipfailover configuration is
serialized in etcd, but that sort of check is more declarative and
less fragile than |grep|ping through some YAML.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11644 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ANUgep7ZXyhAFgYI7Q0qQ7zzqArU0cBMks5rAvR0gaJpZM4Kjh5W.

stevekuznetsov · 2016-11-22T14:20:45Z

Yes, the following would work:

os::cmd::expect_success_and_text "..." 'value: "1177"'

But, for all of the reasons above I still think it is much better to not do --dry-run but actually serialize the object to etcd and use a jsonpath to get the value you want, then check that and that only. See an example in routes.sh.

knobunc · 2016-11-30T19:23:20Z

[test]

knobunc · 2016-11-30T21:14:57Z

[test] yum failed

pecameron · 2016-12-08T20:52:38Z

@knobunc Still need help with this...
Thanks...

knobunc · 2016-12-08T21:00:26Z

# cover github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/client/restclient
[WARNING] `go test` had the following output to stderr:
cover: internal error: block 268 overlaps block 270
	/go/src/github.com/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/client/restclient/request.go:#27803,#27927 /go/src/github.com/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/client/restclient/request.go:#27848,#27867

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_check/8905/consoleText

smarterclayton · 2016-12-08T21:08:44Z

That's a spurious error. Doesn't actually cause failure.

pecameron · 2016-12-09T13:27:17Z

So, what do we do now?

…

On 12/08/2016 04:08 PM, Clayton Coleman wrote: That's a spurious error. Doesn't actually cause failure. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11644 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANUgelizNLrMN0_xmwGO_RpJq-rP8yobks5rGHHmgaJpZM4Kjh5W>.

knobunc · 2016-12-09T13:35:31Z

[test]

pecameron · 2016-12-13T14:19:28Z

[test]

knobunc · 2016-12-13T16:07:55Z

/go/src/github.com/openshift/origin/test/cmd/router.sh: line 82: unexpected EOF while looking for matching ''`

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_check/9400/consoleText

Add options to 'oadm ipfailover' to configure the check script and notify scripts and also control the period the check script runs. Keepalived periodically checks whether the application is running properly. In the default case the test is a simple verification that something is listening on the watch port. This PR permits the user to supply an additional check script that is run in the ipfailover container context to verify that the application is operating properly. For example, a web server can be tested by accessing the watch port and verifying the response. Whenever a node changes state to MASTER, BACKUP, or FAULT a notify script can be called. This script has 3 parameters filled in by keepalived: $1 - "GROUP"|"INSTANCE" $2 - name of group or instance $3 - target state of transition ("MASTER"|"BACKUP"|"FAULT") --check-script="check_script" The check script is a script in the keepalived container that verifies the service is running properly. These checks are in addition to verifying that the watch port is listening. --notify-script="notify_script" The notify script is a script in the keepalived container that is called whenever the keepalived state transitions to (MASTER|BACKUP|FAULT) --check-interval=<n> The check script is run every <n> seconds. Default is 2. Note: the scripts name is the full path to the script. Resolves 1362163 Signed-off-by: Phil Cameron <[email protected]>

openshift-bot · 2016-12-13T16:13:37Z

Evaluated for origin test up to bce0038

openshift-bot · 2016-12-13T17:23:24Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12335/) (Base Commit: fb0bbde)

pecameron · 2016-12-13T18:30:52Z

@knobunc are we ready to merge this?

knobunc · 2016-12-15T18:57:11Z

[merge]

knobunc · 2016-12-16T16:29:07Z

[merge]

pecameron · 2016-12-19T14:55:21Z

@knobunc This failed again, passed once before you did the merge and failed after. Maybe another merge will work...

pecameron · 2016-12-19T19:24:54Z

@knobunc https://bugzilla.redhat.com/show_bug.cgi?id=1362163 comment #6 is for the doc changes. The code is not yet in...

knobunc · 2016-12-20T14:29:20Z

[merge] last flaked with #11016 (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12459/)

knobunc · 2016-12-21T15:38:10Z

[merge] last flaked with #10228 (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_conformance/9749/consoleText)

openshift-bot · 2016-12-21T15:45:17Z

Evaluated for origin merge up to bce0038

openshift-bot · 2016-12-21T16:13:34Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12567/) (Base Commit: 05e4f0e) (Image: devenv-rhel7_5584)

stevekuznetsov suggested changes Oct 28, 2016

View reviewed changes

pecameron force-pushed the bz1362163 branch from 16508f4 to 1ee955b Compare October 28, 2016 16:06

stevekuznetsov reviewed Nov 10, 2016

View reviewed changes

stevekuznetsov suggested changes Nov 10, 2016

View reviewed changes

pecameron force-pushed the bz1362163 branch from 1ee955b to 2ca6f13 Compare November 18, 2016 14:21

stevekuznetsov approved these changes Nov 18, 2016

View reviewed changes

knobunc approved these changes Nov 18, 2016

View reviewed changes

pecameron force-pushed the bz1362163 branch from 2ca6f13 to 8b9e83a Compare November 21, 2016 20:29

knobunc self-assigned this Nov 22, 2016

stevekuznetsov reviewed Nov 22, 2016

View reviewed changes

pecameron force-pushed the bz1362163 branch from 8b9e83a to dc5367d Compare November 22, 2016 16:38

pecameron force-pushed the bz1362163 branch from dc5367d to 40864f3 Compare December 13, 2016 14:18

pecameron force-pushed the bz1362163 branch from 40864f3 to bce0038 Compare December 13, 2016 16:11

stevekuznetsov mentioned this pull request Dec 13, 2016

Revert parallel Docker builds #12246

Closed

openshift-bot merged commit 3164ce5 into openshift:master Dec 21, 2016

Improve ipfailover configurability #11644

Improve ipfailover configurability #11644

Conversation

pecameron commented Oct 28, 2016 • edited Loading

smarterclayton commented Oct 28, 2016

pecameron commented Oct 28, 2016

smarterclayton commented Oct 28, 2016

stevekuznetsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pecameron commented Oct 28, 2016

pecameron commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov left a comment

Choose a reason for hiding this comment

pecameron commented Nov 18, 2016

stevekuznetsov commented Nov 18, 2016

knobunc commented Nov 18, 2016

pecameron commented Nov 18, 2016

knobunc commented Nov 18, 2016

pecameron commented Nov 18, 2016

stevekuznetsov commented Nov 18, 2016

pecameron commented Nov 18, 2016

knobunc commented Nov 18, 2016

knobunc left a comment

Choose a reason for hiding this comment

knobunc commented Nov 21, 2016

knobunc commented Nov 22, 2016

Choose a reason for hiding this comment

pecameron commented Nov 22, 2016 • edited by stevekuznetsov Loading

stevekuznetsov commented Nov 22, 2016

stevekuznetsov commented Nov 22, 2016

pecameron commented Nov 22, 2016

stevekuznetsov commented Nov 22, 2016

knobunc commented Nov 30, 2016

knobunc commented Nov 30, 2016

pecameron commented Dec 8, 2016

knobunc commented Dec 8, 2016

smarterclayton commented Dec 8, 2016

pecameron commented Dec 9, 2016 via email

knobunc commented Dec 9, 2016

pecameron commented Dec 13, 2016

knobunc commented Dec 13, 2016

openshift-bot commented Dec 13, 2016

openshift-bot commented Dec 13, 2016

pecameron commented Dec 13, 2016

knobunc commented Dec 15, 2016

knobunc commented Dec 16, 2016

pecameron commented Dec 19, 2016

pecameron commented Dec 19, 2016

knobunc commented Dec 20, 2016

knobunc commented Dec 21, 2016

openshift-bot commented Dec 21, 2016

openshift-bot commented Dec 21, 2016 • edited Loading

pecameron commented Oct 28, 2016 •

edited

Loading

pecameron commented Nov 22, 2016 •

edited by stevekuznetsov

Loading

openshift-bot commented Dec 21, 2016 •

edited

Loading