Skip to content
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
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,8 @@ jobs:
sudo mkdir -p /run/sshd

sudo install -m 0755 target/release/system-reinstall-bootc /usr/bin/system-reinstall-bootc
sudo podman pull quay.io/fedora/fedora-bootc:42
sudo bootc-integration-tests system-reinstall quay.io/fedora/fedora-bootc:42 --test-threads=1
# These tests may mutate the system live so we can't run in parallel
sudo bootc-integration-tests system-reinstall localhost/bootc --test-threads=1
Comment thread
ckyrouac marked this conversation as resolved.

# And the fsverity case
sudo podman run --privileged --pid=host localhost/bootc-fsverity bootc install to-existing-root --stateroot=other \
Expand Down
5 changes: 1 addition & 4 deletions system-reinstall-bootc/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,7 @@ fn run() -> Result<()> {
podman::ensure_podman_installed()?;

//pull image early so it can be inspected, e.g. to check for cloud-init
let mut pull_image_command = podman::pull_image_command(&config.bootc_image);
pull_image_command
.run_with_cmd_context()
.context(format!("pulling image {}", &config.bootc_image))?;
podman::pull_if_not_present(&config.bootc_image)?;

println!();

Expand Down
25 changes: 24 additions & 1 deletion system-reinstall-bootc/src/podman.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,35 @@ pub(crate) fn reinstall_command(image: &str, ssh_key_file: &str) -> Result<Comma
Ok(command)
}

pub(crate) fn pull_image_command(image: &str) -> Command {
fn pull_image_command(image: &str) -> Command {
let mut command = Command::new("podman");
command.args(["pull", image]);
command
}

fn image_exists_command(image: &str) -> Command {
let mut command = Command::new("podman");
command.args(["image", "exists", image]);
command
}

pub(crate) fn pull_if_not_present(image: &str) -> Result<()> {
let result = image_exists_command(image).status()?;

if result.success() {
println!("Image {} is already present locally, skipping pull.", image);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder if we need this, feels like a "silence is golden" type of thing?

return Ok(());
} else {
println!("Image {} is not present locally, pulling it now.", image);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe I like brevity too much but how about just: println!("Pulling: {image}")

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm usually in favor of brevity too but my reasoning for this is since system-reinstall-bootc is supposed to be a user friendly tool then being super clear about what is happening might be right. Not a strong opinion if you think less output is better

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't have a strong opinion either honestly on this specific one

I do though have a more strongly held opinion that at some point we probably want to make the whole output here more consistent/formatted.

One possibility here is basically how Github Actions/Ansible and other things do it with clearly delineated "tasks" that have their own description.
(We could probably get a double win by doing this for install.rs too)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

filed this to track: #1356

println!();
pull_image_command(image)
.run_with_cmd_context()
.context(format!("pulling image {}", image))?;
}

Ok(())
}

/// Path to the podman installation script. Can be influenced by the build
/// SYSTEM_REINSTALL_BOOTC_INSTALL_PODMAN_PATH parameter to override. Defaults
/// to /usr/lib/system-reinstall-bootc/install-podman
Expand Down
31 changes: 28 additions & 3 deletions tests-integration/src/system_reinstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use std::{

use crate::install;

const TIMEOUT: u64 = 120000;
const TIMEOUT: u64 = 60000;

fn get_deployment_dir() -> Result<std::path::PathBuf> {
let base_path = Path::new("/ostree/deploy/default/deploy");
Expand Down Expand Up @@ -62,11 +62,14 @@ pub(crate) fn run(image: &str, testargs: libtest_mimic::Arguments) -> Result<()>
)?;

// Basic flow stdout verification
p.exp_string(
format!("Image {image} is already present locally, skipping pull.").as_str(),
)?;
p.exp_regex("Found only one user ([^:]+) with ([\\d]+) SSH authorized keys.")?;
p.exp_string("Would you like to import its SSH authorized keys")?;
p.exp_string("into the root user on the new bootc system?")?;
p.exp_string("Then you can login as root@ using those keys. [Y/n]")?;
p.send_line("a")?;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This was "a" preexisting bug?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

sort of, "a" still works because send_line sends a newline and the default is "Y". Just more correct to send a "y".

p.send_line("y")?;

p.exp_string("Going to run command:")?;

Expand Down Expand Up @@ -133,13 +136,35 @@ pub(crate) fn run(image: &str, testargs: libtest_mimic::Arguments) -> Result<()>
)?;

p.exp_regex("Found only one user ([^:]+) with ([\\d]+) SSH authorized keys.")?;
p.send_line("a")?;
p.exp_string("[Y/n]")?;
p.send_line("y")?;
p.exp_string("NOTICE: This will replace the installed operating system and reboot. Are you sure you want to continue? [y/N]")?;
p.send_line("y")?;
p.exp_string("Insufficient free space")?;
p.exp_eof()?;
Ok(())
}),
Trial::test("image pull check", move || {
let sh = &xshell::Shell::new()?;
install::reset_root(sh, image)?;

// Run system-reinstall-bootc
let mut p: PtySession = rexpect::spawn(
"/usr/bin/system-reinstall-bootc quay.io/centos-bootc/centos-bootc:stream10",
Some(600000), // Increase timeout for pulling the image
)?;

p.exp_string("Image quay.io/centos-bootc/centos-bootc:stream10 is not present locally, pulling it now.")?;
p.exp_regex("Found only one user ([^:]+) with ([\\d]+) SSH authorized keys.")?;
p.exp_string("[Y/n]")?;
p.send_line("y")?;
p.exp_string("NOTICE: This will replace the installed operating system and reboot. Are you sure you want to continue? [y/N]")?;
p.send_line("y")?;
p.exp_string("Operation complete, rebooting in 10 seconds. Press Ctrl-C to cancel reboot, or press enter to continue immediately.")?;
p.send_control('c')?;
p.exp_eof()?;
Ok(())
}),
];

libtest_mimic::run(&testargs, tests.into()).exit()
Expand Down