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

scripts: persist console kargs during installation #877

Closed
wants to merge 1 commit into from
Closed

scripts: persist console kargs during installation #877

wants to merge 1 commit into from

Conversation

bgilbert
Copy link
Contributor

@bgilbert bgilbert commented Jun 7, 2022

If the user specified console= kargs to the live system, those should probably be persisted to the destination system as well.

Copy link
Contributor Author

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

I've split this PR out of #605 and copied the relevant review comments from there.

@@ -10,6 +10,9 @@ PERSIST_KERNEL_NET_PARAMS=("ipv6.disable" "net.ifnames" "net.naming-scheme")
# List from https://www.mankier.com/7/dracut.cmdline#Description-Network
PERSIST_DRACUT_NET_PARAMS=("ip" "ifname" "rd.route" "bootdev" "BOOTIF" "rd.bootif" "nameserver" "rd.peerdns" "biosdevname" "vlan" "bond" "team" "bridge" "rd.net.timeout.carrier" "coreos.no_persist_ip" "coreos.force_persist_ip")

# Console params to persist
PERSIST_CONSOLE_PARAMS=("console")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jlebon in #605 (comment):

I have mixed feelings about this. On the one hand, I think as a heuristic this makes sense (I know I suggested it myself at least once), but OTOH this is adding more kargs magic which increases the divide between the coreos.inst path vs manual installs.

I guess one thing that helps is that in the coreos.inst path, we automatically reboot which strongly implies that the console setting should be the same.

Maybe what would help too is if we had a coreos-installer install option like --copy-console-kargs. Then we could frame it as "the coreos.inst path implies --copy-console-kargs, which can also be enabled in a manual install". (On that note, maybe we could even also have --copy-network-kargs and --copy-s390x-kargs too and drain everything here into coreos-installer proper.)

Anyway, not a blocker, just voicing some thoughts.

@@ -97,7 +100,7 @@ for item in "${cmdline[@]}"; do
firstboot_args+="${item} "
fi
done
for param in "${PERSIST_S390X_PARAMS[@]}"; do
for param in "${PERSIST_CONSOLE_PARAMS[@]}" "${PERSIST_S390X_PARAMS[@]}"; do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jlebon in #605 (comment):

For console kargs specifically, we should probably also delete all previous platform-default console kargs before persisting those from the live environment. Although the last one wins, non-last console kargs still have an effect, and it wouldn't be correct to say we're exactly matching the console kargs of the live environment.

coreos-installer doesn't support deleting kargs by key only, but with a --copy-console-kargs, it could learn to delete all the metal kargs (and not append any platform kargs) if it picks up any console kargs from the live environment.

@bgilbert
Copy link
Contributor Author

bgilbert commented Jun 7, 2022

This change was an afterthought to #605, and it clearly has broader implications. Landing it now would change the behavior of console kargs before the new defaults land in coreos/fedora-coreos-config#1181, which would be confusing, especially for OCP 4.11. So I think we should at least defer this until after the upcoming coreos-installer release.

If the user specified console= kargs to the live system, those should
probably be persisted to the destination system as well.
@bgilbert
Copy link
Contributor Author

bgilbert commented Jun 6, 2023

This change probably needs some additional consideration before landing, and isn't a priority for now. Closing.

@bgilbert bgilbert closed this Jun 6, 2023
@bgilbert bgilbert deleted the console-karg branch June 6, 2023 17:13
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.

1 participant