RFE-2962: configure ovs should use node-ip-hint set by nodeip-configuration service #3233
Conversation
|
@cybertron this will use file that will be created in openshift/baremetal-runtimecfg#185 |
tsorya
force-pushed
the
igal/ovs_config_node_ip_file
branch
from
3c9f0b8
to
b15f96e
Compare
tsorya
force-pushed
the
igal/ovs_config_node_ip_file
branch
4 times, most recently
from
299b741
to
abcb31f
Compare
There was a problem hiding this comment.
Overall this looks good, modulo the changes I requested in openshift/baremetal-runtimecfg#185
One suggestion inline, but that could be done as a followup too.
tsorya
force-pushed
the
igal/ovs_config_node_ip_file
branch
2 times, most recently
from
8979e09
to
9387835
Compare
tsorya
force-pushed
the
igal/ovs_config_node_ip_file
branch
from
9387835
to
3665aa0
Compare
tsorya
force-pushed
the
igal/ovs_config_node_ip_file
branch
from
3665aa0
to
bd4f8c2
Compare
tsorya
force-pushed
the
igal/ovs_config_node_ip_file
branch
from
bd4f8c2
to
4bd0b82
Compare
|
/retest |
1 similar comment
|
/retest |
|
@cybertron @rbbratta Can we push this one? |
|
/test ? |
|
@tsorya: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
|
/test e2e-aws-single-node |
|
/test e2e-metal-ipi-ovn-ipv6 |
|
This looks reasonable to me. I'm running a deployment locally just so I can poke around and verify that it does the right thing, but given that ci is passing I expect it will be fine. |
| if [[ -z "${ip_hint}" ]]; then | ||
| return | ||
| fi | ||
| while true; do |
There was a problem hiding this comment.
I think this should try for a bounded amount of time instead of forever. An unexpected issue could get us stuck here.
nodeip-configuration service to set bridge on interface that matches this ip
tsorya
force-pushed
the
igal/ovs_config_node_ip_file
branch
2 times, most recently
from
7bae977
to
a6aa1a0
Compare
tsorya
force-pushed
the
igal/ovs_config_node_ip_file
branch
from
a6aa1a0
to
1dcb4ec
Compare
| # Retry for 5 minutes | ||
| retries=300 |
There was a problem hiding this comment.
Does this really need to be 5 minutes? Isn't the tentative status on an IPv6 address expected to only last a few seconds? I would not expect more than 10 bind attempts throughout a time window of ~20s.
Otherwise, if there is some other issue we are going to be sitting there 5 mins masking an unrelated problem.
There was a problem hiding this comment.
Lets have a minute here.
In anycase if this will fail we will be stuck on installer while waiting for masters to be up, so waiting for a minute will not actually change anything
| echo "No ipv6 ip to bind was found" | ||
| break | ||
| fi | ||
| random_port=$(shuf -i 30000-65000 -n 1) |
There was a problem hiding this comment.
The default ephemeral port range is
❯ cat /proc/sys/net/ipv4/ip_local_port_range
32768 60999
IANA defines the range for temporary services as
49152–65535
We probably should do fine with a much more smaller sub-range of ports between 49152 and 60999.
I am fine with randomizing a port, but honestly I did not expect any collision using the default netcat port 31337. Better be safe than sorry I guess.
tsorya
force-pushed
the
igal/ovs_config_node_ip_file
branch
2 times, most recently
from
f16a43e
to
236296f
Compare
| # Finds the default interface. | ||
| # Reading $ip_hint_file and searching for interface that matches this ip. | ||
| # In case $ip_hint_file was not set use flow below. | ||
| # If the default interface is br-ex, use that and return. |
There was a problem hiding this comment.
nit: I propose this instead
# Accepts parameters $iface, $iface_default_hint_file, $ip_hint_file
# Finds the nodeip interface from the interface that matches the ip address in $ip_hint_file.
# Otherwise fallbacks to a previously used interface or to the default interface.ç
# Never use the interface that is provided inside extra_bridge_file for br-ex1.
# Never use br-ex1.
# Read $ip_hint_file and return the interface that matches this ip. Otherwise:
# If the default interface is br-ex, use that and return.
# If the default interface is not br-ex:
# Check if there is a valid hint inside iface_default_hint_file. If so, use that hint.
# If there is no valid hint, use the default interface that we found during the step
# earlier. Write the default interface to the hint file.
There was a problem hiding this comment.
Reading this I realized that this might lead to a change in the interface from what was previously used, which is what iface_default_hint_file is trying to prevent.
This was done because of bug https://bugzilla.redhat.com/show_bug.cgi?id=2057160 but I fail to see any actual problem documented in the bug.
@rbbratta @andreaskaris are you aware of what actual problems did changing the interface used actually bring up, if any?
If we are not sure about this, would it be a problem to honor iface_default_hint_file above ip_hint_file? Or are we actively seeking support to move to another interface after an upgrade (or after a reboot)?
There was a problem hiding this comment.
We should support moving IMO. We need to be aligned with nodeip service this is actual idea of it. Though it should always be the same interface.
There was a problem hiding this comment.
We should be coupled with nodeip service another way if it will set ip that is not matching interface it will cause very strange failures, those that we are trying to fix with this PR actually. Ovs should not decide from now, it should take whatever nodeip decide to be right by setting node-ip file and ovs just takes interface that matches it
There was a problem hiding this comment.
I understand that but I am not sure right now if this caused any problems in ovn-kubernetes, I guess not.
Had you have the chance to actually try out what happens when we actually move from another interface?
I don't think we have any actual verification of this.
There was a problem hiding this comment.
Interface was changed as expected but i didn't actually look on what is going on with cluster itself to say the truth. Though chance if getting another interface for the same IP is really low
There was a problem hiding this comment.
@jcaamano in 2057160 the different interfaces were on different subnets, so I assume they didn't have connectivity on the other subnets.
Feb 22 17:54:43 master-0 configure-ovs.sh[2310]: inet 192.168.111.20/24 brd 192.168.111.255 scope global dynamic noprefixroute br-ex
Feb 22 17:54:43 master-0 configure-ovs.sh[2310]: 192.168.111.0/24 dev br-ex proto kernel scope link src 192.168.111.20 metric 49
Feb 22 18:33:11 master-0 configure-ovs.sh[2775]: inet 192.168.111.20/24 brd 192.168.111.255 scope global dynamic noprefixroute br-ex
Feb 22 18:33:11 master-0 configure-ovs.sh[2775]: 192.168.111.0/24 dev br-ex proto kernel scope link src 192.168.111.20 metric 49
Feb 22 18:33:15 master-0 configure-ovs.sh[2775]: inet 192.168.221.20/24 brd 192.168.221.255 scope global dynamic noprefixroute br-ex
Feb 22 18:33:15 master-0 configure-ovs.sh[2775]: 192.168.221.0/24 dev br-ex proto kernel scope link src 192.168.221.20 metric 49
The issue would also be that moving interfaces changes IP. Changing IP breaks Etcd Certs on masters.
EtcdCertSignerControllerDegraded: [x509: certificate is valid for 10.30.1.4, not 2101::4, x509: certificate is valid for ::1, 10.30.1.4, 127.0.0.1, ::1, not 2101::4] here the certificate is valid for IPV4
I don't think we ran into a case where interface changed but IP didn't.
2057160 is tricky in that it is IPI OSP with additionalNetworkIDs. OSP additionalNetworkIDs is the only case of multi-homed IPI that I know of.
We might be able to test with a bonding scenario, if both slaves have the same static IP DHCP mapping and bond0 fails to activate somehow on boot, NM should fall back to just using one of the slaves, so IP might stay the same but interface change on reboot from bond0 to ens192 or some such.
…ck that it is bindable before exit
In order to ensure all services on a node are using the same IP, we need to have nodeip-configuration run first and allow other services to use the results to help them figure out which IP and interface they should use. Previously, ovs-configuration was a prereq for network-online and nodeip-configuration waited for network-online. Additionally, ovs-configuration waited for NetworkManager-wait-online to ensure NetworkManager was fully up before it tried to do its configuration. To change the order of these two services, I removed the network-online dependency from nodeip-configuration and replaced it with the NetworkManager-wait-online that was previously part of the ovs-configuration deps. A dependency was also added between ovs-configuration and nodeip-configuration to ensure that they run in the correct order. This way, at the end of ovs-configuration we are able to reach the network-online target just as before, but now we've also run nodeip-configuration at that point so we know all of the services should have the correct IP. Essentially nodeip-configuration was moved earlier in the boot process, but everything else has been left alone.
tsorya
force-pushed
the
igal/ovs_config_node_ip_file
branch
from
236296f
to
eb969e7
Compare
|
/test e2e-aws-ovn-workers-rhel8 |
|
@cybertron one last PTAL? |
|
This one looks good in my local dual stack test. IIRC, the bug related to the previous hint file had to do with the fact that the route priority was being used to select the interface, and when the bridge got destroyed it changed that priority in undesirable ways. Having the ip selection happen before the bridge gets torn down should fix that too. The only place it should matter anymore is for platforms that don't use nodeip-configuration, and I think those by definition don't have the multiple interface problem (except vsphere upi, which is a gap that needs to be addressed separately). I opened the previous bug because it seemed wrong that the bridge moved to a completely different subnet after reboot, but TBH I don't think anything actually broke as a result. That said, I didn't test the cluster extensively in that state either. In any case, I think this change also fixes that previous bug so it should be fine if we prioritize this new hint file over the old one. |
|
/retest |
|
/lgtm |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cybertron, flaper87, jcaamano, tsorya, yuqi-zhang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@tsorya: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
In openshift#3233 we changed the order of dependencies for ovs-configuration and nodeip-configuration. However, because nodeip-configuration doesn't run everywhere, this means ovs-configuration lost its depenedency on NetworkManager-wait-online in some cases. Since the first patch merged, some upgrade jobs have been having issues that seem to be related to that change. It's not entirely clear why, but since this was an unintended change to the dependencies it seems like a strong candidate. To fix the problem, we can just have the NetworkManager-wait-online dep in both services. When nodeip-configuration runs it will be redundant, but at least ovs-configuration will always wait for it.
RFE-2962: configure ovs should use node-ip-hint set by
nodeip-configuration service to set bridge on interface that matches
this ip
- What I did
- How to verify it
- Description for the changelog