Skip to content
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

systemd: remove rudimentary network checking code; depend on NetworkManager #127

Merged
merged 3 commits into from
Jan 7, 2020

Conversation

dustymabe
Copy link
Member

Previously you could prematurely continue into /usr/bin/coreos-installer
before DNS was up. This commit improves network checking such that DNS
is checked (if needed) before continuing.

Previously you could prematurely continue into /usr/bin/coreos-installer
before DNS was up. This commit improves network checking such that DNS
is checked (if needed) before continuing.
@dustymabe
Copy link
Member Author

hold for now, testing

# - This case implies we are going to contact a remote
# service (most likely fedora infra) so we need to
# test DNS is working befrore continuing. In this case
# we'll check the fedora hotspot globally accessible URL:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. NetworkManager has this functionality built in. And on RHEL it uses a RHEL endpoint.

Not blocking this but...is it really worth re-coding that check here?

I think a much simpler common network test is "do we have a default route" - GLib supports this.

What's the harm in having the pull fail later?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cgwalters I'm +1 for letting networkmanager handle things here (especially since we're in the real root), but what is a sufficient requirement to add? Currently we have:

Wants=network-online.target

Which doesn't seem to be enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also will the NM check block on having DNS set up?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the harm in having the pull fail later?

The installer seems to barf and not do any retries (probably should be fixed). I get a failed install.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#128

Thanks! I included that commit in this PR and tested everything together. I'd like to keep my original commit for history, even though it's not needed, in case we need to come back to it for any reason.

@dustymabe dustymabe marked this pull request as ready for review January 7, 2020 20:12
cgwalters added a commit to cgwalters/coreos-installer that referenced this pull request Jan 7, 2020
From https://www.freedesktop.org/wiki/Software/systemd/NetworkTarget/
"Cut the crap! How do I make sure that my service starts after the network is really online?"

We were just missing this one.  Came out of a discussion
about testing for network availability in
coreos#127
From https://www.freedesktop.org/wiki/Software/systemd/NetworkTarget/
"Cut the crap! How do I make sure that my service starts after the network is really online?"

We were just missing this one.  Came out of a discussion
about testing for network availability in
coreos#127
@dustymabe dustymabe changed the title systemd: make "network up" checking more robust systemd: remove rudimentary network checking code; depend on NetworkManager Jan 7, 2020
@dustymabe
Copy link
Member Author

This is ready for further review

Now that we have After=network-online.target added in 35b41f9
NetworkManager is handling this for us and we don't need this
crude network online checking.
@dustymabe dustymabe merged commit 10f5d69 into coreos:master Jan 7, 2020
@dustymabe
Copy link
Member Author

Thanks @cgwalters for the help with this

lucab added a commit to lucab/coreos-installer that referenced this pull request Jan 9, 2020
This adds some retries to curl when downloading Ignition config.
It should give us some room for resiliency in case of transient
network hiccups.
Proactive measure, this has not yet been observed/reported as a
direct cause of failures in the wild.

Related: coreos#127
lucab added a commit to lucab/coreos-installer that referenced this pull request Jan 13, 2020
This adds some retries to curl when downloading Ignition config.
It should give us some room for resiliency in case of transient
network hiccups.
Proactive measure, this has not yet been observed/reported as a
direct cause of failures in the wild.

Related: coreos#127
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants