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

install: add --copy-network and --network-dir options #212

Merged
merged 1 commit into from
Apr 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/cmdline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ pub struct InstallConfig {
pub firstboot_kargs: Option<String>,
pub insecure: bool,
pub preserve_on_error: bool,
pub network_config: Option<String>,
}

pub struct DownloadConfig {
Expand Down Expand Up @@ -136,6 +137,22 @@ pub fn parse_args() -> Result<Config> {
.help("Additional kernel args for the first boot")
.takes_value(true),
)
.arg(
Arg::with_name("copy-network")
.short("n")
.long("copy-network")
.help("Copy network config from install environment"),
)
.arg(
Arg::with_name("network-dir")
.long("network-dir")
.value_name("path")
.default_value("/etc/NetworkManager/system-connections/")
Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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.

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.

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.

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 by nmtui on each platform and not overcomplicate things.

Copy link
Member

Choose a reason for hiding this comment

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

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 by nmtui on each platform and not overcomplicate things.

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

i.e. make ifcfg files a read source but not a write target.

+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.

Copy link
Member Author

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

.takes_value(true)
.empty_values(false)
.help("For use with -n.")
.next_line_help(true), // so we can stay under 80 chars
)
// obscure options without short names
.arg(
Arg::with_name("insecure")
Expand Down Expand Up @@ -400,6 +417,15 @@ fn parse_install(matches: &ArgMatches) -> Result<Config> {
// and report it to the user
eprintln!("{}", location);

// If the user requested us to copy networking config by passing
// -n or --copy-network then copy networking config from the
// directory defined by --network-dir.
let network_config = if matches.is_present("copy-network") {
matches.value_of("network-dir").map(String::from)
} else {
None
};

// build configuration
Ok(Config::Install(InstallConfig {
device,
Expand All @@ -409,6 +435,7 @@ fn parse_install(matches: &ArgMatches) -> Result<Config> {
firstboot_kargs: matches.value_of("firstboot-kargs").map(String::from),
insecure: matches.is_present("insecure"),
preserve_on_error: matches.is_present("preserve-on-error"),
network_config,
}))
}

Expand Down
42 changes: 40 additions & 2 deletions src/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)?;
Expand All @@ -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(())
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

Since this isn't an error, let's use println!.

Copy link
Member Author

Choose a reason for hiding this comment

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

will look into this. Thanks for the suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, all of our existing status logging goes to stderr.

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 install has taken, that would go to stdout for piping to another process, and the status reports would need to not be commingled with it.

@lucab, thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 install has taken, that would go to stdout for piping to another process, and the status reports would need to not be commingled with it.

Sounds like a good use case for a --quiet.

Copy link
Member Author

@dustymabe dustymabe Apr 15, 2020

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

follow up: #214

Copy link
Member

Choose a reason for hiding this comment

The 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 install has taken, that would go to stdout for piping to another process, and the status reports would need to not be commingled with it.

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. --status-report-json /path/to/status.json. Or alternatively --output=json and that also suppresses internal printing.


// 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");
Expand Down