-
Notifications
You must be signed in to change notification settings - Fork 243
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
fetch-offline: new stage for conditional networking #956
Conversation
Requires: The no network path works well (e.g. testing this with QEMU via |
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 looks great!
@@ -33,23 +33,23 @@ func translateFilesystem(old old_types.Filesystem) (ret types.Filesystem) { | |||
return | |||
} | |||
|
|||
func translateConfigReference(old old_types.ConfigReference) (ret types.ConfigReference) { | |||
func translateConfigReference(old old_types.ConfigReference) (ret types.Resource) { |
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.
The elegance of this approach is definitely underscored by the fact that 3 separate types are now transparently converted to the same thing.
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 really like having the ability to add another resource easily.
We want to move to a model where networking isn't unconditionally brought up, but instead only if Ignition requires it. Works with: coreos/fedora-coreos-config#321 coreos/ignition-dracut#164 coreos/ignition#956
We want to move to a model where networking isn't unconditionally brought up, but instead only if Ignition requires it. But I think we'll still have to keep supporting `ignition_network_kcmdline` since it's kind of part of our API now that it can be overridden via `ignition.firstboot`. Works with: coreos/fedora-coreos-config#321 coreos/ignition-dracut#164 coreos/ignition#956
OK, with the latest version of these patches both networking and non-networking paths work! I also verified that the live ISO now by default no longer pulls in networking. |
c200ec8
to
6c19181
Compare
This is now rebased on top of the proposal from coreos/fedora-coreos-tracker#460. Ready for review! |
Any thoughts on this? This will obsolete the |
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 don't know the Ignition codebase really well, but this LGTM!
I had to follow all the chain of stuff again to be sure I understood this. It's actually pretty dizzying how the kargs have been shuffled/split from cosa to (afterburn, config repos) then with this they'll move into (afterburn, ignition). But, it does make sense in the end!
I'll throw my hat in as another LGTM. |
}, | ||
} | ||
assertNeedsNet(t, &cfg) | ||
cfg.Ignition.Config.Replace.Source = nil |
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.
Could we test a merged/replaced config from a data URL which contains a network dependency?
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 looked into this, but the issue is that config rendering is part of the engine code, so the introspection code won't pick this up. I think it'd require moving this to a blackbox test and tweaking the blackbox test harness so that we can run the "fetch-offline" (and "fetch" too for that matter) stages explicitly. This would go well with #950 too; mind if we punt on this for now?
(That said, I did verify that this case is covered.)
1e9aae7
to
862ff89
Compare
Add a new `fetch-offline` stage which can optionally be run before the `fetch` stage. The major difference between the two is that the former tries to operate in offline mode: if it encounters any resource which requires network access, it quietly exits, creating a stamp file on the way out. This allows OS integrators to make use of this to only bring up networking if Ignition actually requires it. It does not solve the harder problem of "partially" up networking, where some fetches might succeed and some others might fail. However, it provides an incremental step to get there by reusing the same signalling mechanism.
On CloudStack/OpenStack, we fetch from three different sources simultaneously: two config drives, and the metadata service. Error-handling for these goroutines was causing `ErrNeedNet` from the latter to be ignored and so we weren't correctly propagating it back to the caller (which keys off of it to signal to the OS that networking is needed). Do a simple hack where we check if `ErrNeedNet` was hit and if none of the fetchers succeed, then we return that instead. (The better fix of course is to not try to parallel guess the metadata source like this, but that's a much bigger issue.) Fixes: coreos#956 Fixes: coreos#1056
Add a new
fetch-offline
stage which can optionally be run before thefetch
stage. The major difference between the two is that the formertries to operate in offline mode: if it encounters any resource which
requires network access, it quietly exits, creating a stamp file on the
way out.
This allows OS integrators to make use of this to only bring up
networking if Ignition actually requires it. It does not solve the
harder problem of "partially" up networking, where some fetches might
succeed and some others might fail. However, it provides an incremental
step to get there by reusing the same signalling mechanism.