Bug 1751978: templates/baremetal: Fix keepalived dysfunction on vrrp iface change by celebdor · Pull Request #1124 · openshift/machine-config-operator

celebdor · 2019-09-18T21:48:58Z

When using CNV or other operators that modify how the node is connected
to the network, we may end up in the case where the configured VRRP
interface no longer has an address in the network that it is configured
to hold virtual IPs in.

This patch takes a page from what we do for HAProxy and adds a monitor
side car container that checks keepalived and reloads it when necessary.

Fixes: #1751978
Signed-off-by: Antoni Segura Puimedon antoni@redhat.com

celebdor · 2019-09-18T21:49:26Z

Depends on openshift/baremetal-runtimecfg#20 being merged

celebdor · 2019-09-18T21:53:27Z

/assign runcom

phoracek · 2019-09-18T22:42:55Z

Is runtimecfg still used by anyone or it can be dropped from https://github.com/openshift/baremetal-runtimecfg/?

It is used by mdns-publisher

phoracek · 2019-09-18T22:44:11Z

there is a possibility so safe one (!) character by dropping the extra space

phoracek · 2019-09-18T22:45:57Z

You remove it and start listening to it on the next line? Why?

In case the container is restarted the socket file might be there and socat won't be able to start unless it is gone.

phoracek · 2019-09-18T22:47:13Z

There is a regression. Previously it would fail if the config does not exist.

Now instead of failing it will just wait in socat for a reload when the config is written. What's the issue with that?

openshift-ci-robot · 2019-09-18T22:52:12Z

@celebdor: This pull request references Bugzilla bug 1751978, which is valid. The bug has been moved to the POST state.

Details

In response to this:

Bug 1751978: templates/baremetal: Fix keepalived dysfunction on vrrp iface change

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kikisdeliveryservice · 2019-09-18T22:52:43Z

since this depends on openshift/baremetal-runtimecfg#20 & response to the reviewer's questions above.

/hold

kikisdeliveryservice · 2019-09-18T22:55:17Z

Also updated the PR title, please use this format in the future :)

phoracek · 2019-09-19T09:04:07Z

phoracek · 2019-09-19T09:05:54Z

yboaron · 2019-09-19T09:34:32Z

@celebdor could you please elaborate about the logic of the LivenessProbe?

Absolutely :-)

If keepalived is not running, the pgrep will be empty and the kill command will fail with exit code 1. This means the liveness check will consider it failed.

If keepalived is running, sending SIGUSR1 to the parent keepalived process will make keepalived to write /tmp/keepalived.data containing information about each vrrp group including the state. The state can be MASTER, BACKUP or FAULT. If any of the states are FAULT grep will exit with exit code 1 and the liveness test will fail.

Oops. I just realized I forgot to put put the negative. Will fix it now

yboaron · 2019-09-19T09:45:17Z

@celebdor I think that "$pid" could be empty (the keepalived container starts before monitor container and .conf is missing), should we update something in 'kill -s SIGHUP "$pid"' ?

yboaron · 2019-09-19T11:50:04Z

@celebdor, not sure that I (still :-) ) fully understand the liveness probe logic.
So, if some component (e.g: CNV ) changed network interfaces configuration, that may lead to keepalived failure (State=FAULT), and this container will be restarted by Kubelet. IIUC, the problem will be fixed only after monitor container updates .conf file, right?
Could u please explain what added value do we get from this Liveness check?

The added value is that if any of the VIP management (ingress, api or dns) gets faulty for any cause, we'll restart keepalived.

celebdor · 2019-09-20T09:04:32Z

Verified with success that the IP was moved to brext and the VIPs did as well. The downtime for reconfiguration of the interfaces was of about 90seconds.

celebdor · 2019-09-20T09:05:32Z

/hold cancel

When using CNV or other operators that modify how the node is connected to the network, we may end up in the case where the configured VRRP interface no longer has an address in the network that it is configured to hold virtual IPs in. This patch takes a page from what we do for HAProxy and adds a monitor side car container that checks keepalived and reloads it when necessary. Signed-off-by: Antoni Segura Puimedon <antoni@redhat.com>

runcom

/approve

runcom · 2019-09-20T12:51:59Z

/retest

kikisdeliveryservice · 2019-09-20T16:10:17Z

looks good.

/lgtm

openshift-ci-robot · 2019-09-20T16:10:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: celebdor, kikisdeliveryservice, phoracek, runcom, yboaron

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [kikisdeliveryservice,runcom]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2019-09-20T16:12:40Z

@celebdor: All pull requests linked via external trackers have merged. Bugzilla bug 1751978 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1751978: templates/baremetal: Fix keepalived dysfunction on vrrp iface change

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

When using CNV or other operators that modify how the node is connected to the network, we may end up in the case where the configured VRRP interface no longer has an address in the network that it is configured to hold virtual IPs in. This patch takes a page from what we do for HAProxy and adds a monitor side car container that checks keepalived and reloads it when necessary. This ports openshift#1124 to OpenStack platform, alongside with fixes from openshift#1508 and openshift#1604.

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 18, 2019

openshift-ci-robot requested review from ashcrow and ericavonb September 18, 2019 21:49

openshift-ci-robot assigned runcom Sep 18, 2019

phoracek reviewed Sep 18, 2019

View reviewed changes

kikisdeliveryservice changed the title ~~Fix keepalived dysfunction on vrrp iface change~~ Bug 1751978: templates/baremetal: Fix keepalived dysfunction on vrrp iface change Sep 18, 2019

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 18, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 18, 2019

kikisdeliveryservice requested review from kikisdeliveryservice and removed request for ashcrow September 18, 2019 22:54

phoracek mentioned this pull request Sep 18, 2019

configure host networking policy with default bridge openshift-kni/install-scripts#47

Merged

celebdor force-pushed the bz1751978 branch from a749731 to 7693643 Compare September 19, 2019 08:46

phoracek approved these changes Sep 19, 2019

View reviewed changes

yboaron reviewed Sep 19, 2019

View reviewed changes

celebdor force-pushed the bz1751978 branch 2 times, most recently from 1773fda to b2cf75e Compare September 19, 2019 10:02

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 19, 2019

yboaron reviewed Sep 19, 2019

View reviewed changes

celebdor force-pushed the bz1751978 branch from b2cf75e to 31b2673 Compare September 20, 2019 01:02

yboaron approved these changes Sep 20, 2019

View reviewed changes

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2019

celebdor force-pushed the bz1751978 branch from 31b2673 to 583f84b Compare September 20, 2019 09:07

runcom reviewed Sep 20, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 20, 2019

openshift-ci-robot assigned kikisdeliveryservice Sep 20, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2019

openshift-merge-robot merged commit 1571463 into openshift:master Sep 20, 2019

mandre mentioned this pull request Sep 11, 2020

Bug 1875005: OpenStack: Don't failover api vip if loadbalanced endpoint is responding #2077

Merged

Conversation

celebdor commented Sep 18, 2019

Uh oh!

celebdor commented Sep 18, 2019

Uh oh!

celebdor commented Sep 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Sep 18, 2019

Uh oh!

kikisdeliveryservice commented Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kikisdeliveryservice commented Sep 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yboaron Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

celebdor commented Sep 20, 2019

Uh oh!

celebdor commented Sep 20, 2019

Uh oh!

runcom left a comment

Choose a reason for hiding this comment

Uh oh!

runcom commented Sep 20, 2019

Uh oh!

kikisdeliveryservice commented Sep 20, 2019

Uh oh!

openshift-ci-robot commented Sep 20, 2019

Uh oh!

openshift-ci-robot commented Sep 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kikisdeliveryservice commented Sep 18, 2019 •

edited

Loading

yboaron Sep 19, 2019 •

edited

Loading