-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wait for usable network connectivity before running discovery-* #18
Conversation
Hello, thanks for your contribution! How does this differ from a service that ships with RHEL7/CentOS7 and which is already enabled on our image?
Also discovery-register can start without network, it will just fail with the initial request, but it re-sends every minute. Since smart-proxy binds to all interfaces it should be also fine with starting before network was initialized. The only issue is the TFTP, we have a different PR that tries to solve it, but does not yet work: #14 |
I mean your patch is still valid, but can you delete unnecessary More reading on this topic is at: http://www.freedesktop.org/wiki/Software/systemd/NetworkTarget/ |
The bug might be in the fact that we have the service enabled, but we need to order our services after that online service. |
/usr/lib/systemd/system/NetworkManager-wait-online.service calls nm-online with parameter -s: man 1 nm-online:
"Wait for NetworkManager startup to complete, rather than waiting for network connectivity specifically." is not sufficient. nm-connected.service calls nm-online without -s (and a higher timeout) which is much more meaningful before running discovery-*. discovery-register tries every 30 seconds, and fails because of "getaddrinfo: Name or service not known" when trying to resolv our Katello/Foreman hostname. The retry works when using IP addresses. When using DNS the first lookup fails (because /etc/resolv.conf is missing / incomplete) and subsequent requests fail also because ruby does not re-initialize (res_init()) the resolver, which still uses the broken resolv.conf information (https://sourceware.org/bugzilla/show_bug.cgi?id=984). nm-connected.service is just a drop-in to ensure all discovery-* services have proper network connectivity available the time they are started. My PR also solves the TFTP issue without depending on file "semaphores". |
Ok that looks good. Can you please squash into one commit with msg:
Also couple of other comments will follow. |
@@ -15,6 +15,9 @@ systemctl enable NetworkManager-wait-online.service | |||
echo " * enabling nm-prepare service" | |||
systemctl enable nm-prepare.service | |||
|
|||
echo " * enabling nm-connected service" | |||
systemctl enable nm-connected.service | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also remove systemctl enable NetworkManager-wait-online.service
block please as this replaces it completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without NetworkManager-wait-online.service we effectively lose the synchronization point in network-online.target. Or do I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I thought that since nm-connected
have Before=network-online.target
we don't need that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
/usr/lib/systemd/system/network-online.target.wants/NetworkManager-wait-online.service comes from the NetworkManager package and is NOT provided by NetworkManager-wait-online.service [Install]-section. I remove systemctl enable nm-connected.service
and give it a try.
Unfortunately we cannot remove the dance around TFTP downloading. We need to put But there is one bug still, can you replace If you find a different way to make sure we have the environment variables ready for both services above, send your proposal as a different commit please (with the same issue number). |
Ok it works fine, if you can do the |
Or we can leave |
Even with PathChanged a race condition exists: discovery-setup.service is pulled by multi-user.target (after all units up to nm-connected.service triggered). The unit does not complain about an missing EnvironmentFile (there is a - in |
Yeah |
I suggest to move the kernel command line parsing out of discovery-setup and put it into get-zip-server_config. I'm going prepare a proposed patchset within one day. |
This way round PathExists does not need to be modifed. |
Ok, also I was wondering why we use dhclient script and not Later, |
I'd still rather see
Later, |
I'd also removed |
Maybe the dhclient script was intended with some compatibility in mind? On the other hand get-zip-server_config is not "pure-dhclient-ready" because it uses DHCP4_… variables instead of dhclients new_dhcp4_…. Im going to rearrange the functionality to be placed into /etc/NetworkManager/dispatcher.d. |
I've reorganized the startup of the discovery-* scripts. Their units are not longer "WantedBy" multi-user.target. Instead the chain starting at discovery-setup.service is triggered by "PathExists=/etc/default/discovery-zip-server" in discovery-setup.path. discovery-setup.service wants discovery-autostart.service which wants discovery-register.service. Other dependencies are dropped (including my nm-connected.service). /etc/default/discovery-zip-server is created by /etc/NetworkManager/dispatcher.d/15-get-zip-server; a "pure" NM dispatcher.d script. This script only processes "up" events for the "BOOTIF" interface. nm-prepare was also prone to race conditions. I replaced the systemd unit with a more appropriate udev rule and modified the script. |
Re: dhclient compatibility - no backward compatibility needed, our image only supports CentOS7/RHEL7+. Taking a look, btw can you rebase it looks like there is a small conflict. Thanks. |
defroute=no | ||
fi | ||
script=/etc/sysconfig/network-scripts/ifcfg-$dev | ||
cat > $script <<EONS | ||
DEVICE=$dev | ||
TYPE=Ethernet | ||
BOOTPROTO=dhcp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn, this whole file has been completely rewritten in d4cb4ea and it also triggers via udev now. You can basically drop your changes as it works the same way (accepts a parameter = device name). It now does not create rh-cfg plugin configurations but directly NetworkManager ini-style profiles instead. I also use systemd-cat
utility to redirect output to journal.
Ok this looks good. Please note the new |
During rebase, please reword the commit message to |
Oh one note, |
Sounds good. Introduce /usr/share/fdi/commonfunc.sh with functions exportKCL and normalizeHwAddr. |
KCL_BOOTIF is the variable based on the kernel cmd line (including ARP type 01). |
Sounds great. Later, |
Image based in Centos 7 tested successfully with and without fdi.initnet=all. |
fi | ||
|
||
# enable ssh | ||
if [[ "$(cat /proc/cmdline | grep -io 'fdi.ssh=\S*')" == "fdi.ssh=1" ]]; then | ||
if [ "$KCL_FDI_SSH" == "1" ]]; then | ||
systemctl start sshd | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not work for me, sshd is not started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like commonfunc.sh does work, but rc.local is not enabled. This needs to be enabled explicitly on systemd systems AFAIK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually make sure it's executable and then systemctl enable rc-local.service
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait its a syntax error actually: Jul 31 10:51:55 fdi rc.local[1107]: /etc/rc.d/rc.local: line 12: [: missing
]'`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to fix this. Sorry for not testing SSH access.
Ok except the ssh, it works fine. Please fix and amend into a single commit and I will retest multiple times to see if race conditions are really gone (so far so good). Looks great! |
Almost there, I see a parsing error during fetch script startup:
But network seems to be fine. |
# cmdline first and fall back to DHCP next-server passed in through | ||
# ENV | ||
|
||
local extdir="/opt/extension" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry; transparent proxy gave me an outdated fdi-image.tar during download to my test system.
Please wait; I re-test the whole functionality. |
Tested on KVM & HW. |
systemctl enable discovery-setup.path | ||
systemctl enable discovery-setup.service | ||
systemctl enable discovery-autostart.service | ||
systemctl enable discovery-register.service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good but discovery-register does not actually start on my instances:
[root@fdi ~]# systemctl status discovery-register
discovery-register.service - Register this host in Foreman
Loaded: loaded (/etc/systemd/system/discovery-register.service; disabled)
Active: inactive (dead)
I will merge this because I am currently working on TUI support which actually starts this service manually after TUI splash screen appears.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm setup and autostart are gone/renamed, but I still see usage here:
root/etc/systemd/system/discovery-start-extensions.service
4:After=basic.target network.target network-online.target nss-lookup.target discovery-setup.service
root/etc/systemd/system/discovery-register.service
4:After=basic.target network.target network-online.target nss-lookup.target discovery-autostart.service foreman-proxy.service
Can you delete these and also make sure register service starts?
…nt to download extensions
Merged as 7618547, thank you! |
FYI found one additional issue, not sure why I see it now but TFTP download via CURL does not work because firewall blocks client to send ack packets. This enables it:
|
This is strange. I rebuilt the FDI (fdi-centos7.ks) from a fresh clone. The TFTP download succeeded without any hickups. ip_conntrack_tftp is not loaded in the kernel of the running FDI. iptables -L shows no active rules. Where is this firewall activated at your side? I switched to curl to reduce the needed packages and to be able to implement HTTP downloads easily in the future for higher throughput. |
Sorry, I realized that I actually activated firewalld in my patch. So it Later, |
In systems with many (network & block) devices or in networks with slow DHCP servers the discovery-* services start without a proper resolv.conf & network connectivity.
This leads to:
-failing TFTP downloads of extension zips (server not reachable or name resolution error in case address supplied via fdi.zipserver is a hostname).
-discovery-registers is not able to register against katello/foreman because /etc/resolv.conf is not usable when the ruby script starts / tries name resolution for the first time (resolv.conf ist not re-read by Ruby / the script)
The directive "After=…network-online.target…" in the current discovery-*service definitions is not sufficient. This target (at least in Centos 7) is reached as soon NetworkManager is started.
This diff adds another service (and correspondign Requires to discovery-*.services) which returns as soon NetworkManager thinks the system is connected to a network.