-
Notifications
You must be signed in to change notification settings - Fork 92
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
install: add --copy-network and --network-dir options #212
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ | |
|
||
use error_chain::{bail, ChainedError}; | ||
use nix::mount; | ||
use std::fs::{create_dir_all, read_dir, File, OpenOptions}; | ||
use std::fs::{copy as fscopy, create_dir_all, read_dir, File, OpenOptions}; | ||
use std::io::{copy, Read, Seek, SeekFrom, Write}; | ||
use std::os::unix::fs::FileTypeExt; | ||
use std::path::Path; | ||
|
@@ -111,7 +111,11 @@ fn write_disk( | |
udev_settle()?; | ||
|
||
// postprocess | ||
if ignition.is_some() || config.firstboot_kargs.is_some() || config.platform.is_some() { | ||
if ignition.is_some() | ||
|| config.firstboot_kargs.is_some() | ||
|| config.platform.is_some() | ||
|| config.network_config.is_some() | ||
{ | ||
let mount = mount_partition_by_label(&config.device, "boot", mount::MsFlags::empty())?; | ||
if let Some(ignition) = ignition { | ||
write_ignition(mount.mountpoint(), ignition)?; | ||
|
@@ -122,6 +126,9 @@ fn write_disk( | |
if let Some(platform) = config.platform.as_ref() { | ||
write_platform(mount.mountpoint(), platform)?; | ||
} | ||
if let Some(network_config) = config.network_config.as_ref() { | ||
copy_network_config(mount.mountpoint(), network_config)?; | ||
} | ||
} | ||
|
||
Ok(()) | ||
|
@@ -233,6 +240,37 @@ fn write_platform(mountpoint: &Path, platform: &str) -> Result<()> { | |
Ok(()) | ||
} | ||
|
||
/// Copy networking config if asked to do so | ||
fn copy_network_config(mountpoint: &Path, net_config_src: &str) -> Result<()> { | ||
eprintln!("Copying networking configuration from {}", net_config_src); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this isn't an error, let's use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will look into this. Thanks for the suggestion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, all of our existing status logging goes to stderr. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I'll leave this as is in this PR, but it's probably worth us having a followup where we convert our non-error status logging to non stderr, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? Status reports aren't the primary output of the program, so it makes sense to me that they would go to stderr. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't agree. If we ever add e.g. a JSON report describing what actions @lucab, thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sounds like a good use case for a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either way maybe we should move this conversation into an issue since it no longer really reflects a change we are going to make in this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. follow up: #214 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What you're talking about is one of the most long-standing things in how Unix pipelines work - the fact they're just connected by bytestreams and there's no common APIs or structure. A lot of successor projects try to address this, from Windows Powershell to nushell to many others. (In fact long ago I tried to write one myself...) Practically speaking, I think it's better to do e.g. |
||
|
||
// get the path to the destination directory | ||
let net_config_dest = mountpoint.join("coreos-installer-network"); | ||
|
||
// make the directory if it doesn't exist | ||
create_dir_all(&net_config_dest).chain_err(|| { | ||
format!( | ||
"creating destination networking config directory {}", | ||
net_config_dest.display() | ||
) | ||
})?; | ||
|
||
// copy files from source to destination directories | ||
for entry in | ||
read_dir(&net_config_src).chain_err(|| format!("reading directory {}", net_config_src))? | ||
{ | ||
let entry = entry.chain_err(|| format!("reading directory {}", net_config_src))?; | ||
let srcpath = entry.path(); | ||
let destpath = net_config_dest.join(entry.file_name()); | ||
if srcpath.is_file() { | ||
eprintln!("Copying {} to installed system", srcpath.display()); | ||
bgilbert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fscopy(&srcpath, &destpath).chain_err(|| "Copying networking config")?; | ||
} | ||
} | ||
|
||
Ok(()) | ||
} | ||
|
||
/// Clear the partition table. For use after a failure. | ||
fn clear_partition_table(dest: &mut File) -> Result<()> { | ||
eprintln!("Clearing partition table"); | ||
|
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 default value will likely need to be different for RHCOS (for now). I'm thinking we can just carry that as a patch in the RPM in downstream. Does that sound OK?
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.
We also do want to copy that path for RHCOS too. So it's more like disabling ifcfg copying on FCOS. We could do something like look at the target OS install and conditionalize based on that...since we do want to allow people to use the upstream/Fedora
coreos-installer
binary to install RHCOS.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.
Or to rephrase, FCOS supports NM config files; RHCOS supports NM config files and ifcfg files. It's not "RHCOS only supports ifcfg files".
Perhaps simplest is just to make this a boolean value, and copy files from both paths if they exist and the option is specified.
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.
Using the FCOS installer to install RHCOS is probably a little risky long term, though for this case the user could still pass
-n /etc/sysconfig/network-scripts/
and get it to work.Since the use case here is to propagate forward the files generated by
nmtui
I think we should support the default format that is spit out bynmtui
on each platform and not overcomplicate things.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.
Hmm. I suppose you're right that NM will save in ifcfg files by default on RHCOS. I wonder how hard that'd be to change though...i.e. make ifcfg files a read source but not a write target.
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.
+1, that's what it does for nm keyfiles today IIUC. I'd have to dig into the plugin to see if it's possible. For now I'm going to stick with this and I'll come back and change it if I have enough time between now and this Friday.
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.
opened this against NM upstream to see if there's any way to do what we want: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/415