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

Add platform-specific console configuration metadata #1181

Closed
wants to merge 1,598 commits into from
Closed

Add platform-specific console configuration metadata #1181

wants to merge 1,598 commits into from

Conversation

bgilbert
Copy link
Contributor

@bgilbert bgilbert commented Aug 24, 2021

coreos-assembler create_disk.sh and gf-set-platform will read this. create_disk.sh will also save a JSON version for the relevant architecture to /boot/coreos/platforms.json for coreos-installer.

We'll need to post advance warning to coreos-status before landing this.

@dustymabe
Copy link
Member

I know this depends on the backend implementation, but.. any chance we could or should use yaml here (supports comments), which may be quite useful in the future help add context for why a platform might need particular kernel arguments.

@bgilbert
Copy link
Contributor Author

YAML precludes the use of jq. Unrecognized fields are ignored, though, so we could always add a comment field.

@cgwalters
Copy link
Member

I may have missed this, but it seems odd to ship things in the OS that we're not actually using at runtime. We've been using image.yaml as kind of a dumping ground for "configure os-specific things for coreos-assembler". We could also invent bootloader.yaml too in this repo e.g.

For jq a pattern used in cosa is to convert to json in a temporary place and use jq on that.

@bgilbert
Copy link
Contributor Author

bgilbert commented Aug 24, 2021

We don't strictly need it in /usr; that copy is only there for the convenience of create_disk.sh (and so we can use the overlay.d mechanism). We do need it in /boot, though, for coreos/coreos-installer#605.

You're right that we don't strictly need JSON in cosa, and coreos-installer could parse YAML if we want. But for a machine-readable API it seems like JSON would be a better choice?

@jlebon
Copy link
Member

jlebon commented Aug 25, 2021

Not a fan of having them in both /usr and /boot. Also would prefer YAML for comments, but agree JSON would be better on disk.

How about having them as YAML files under a new platforms/ dir or so in this repo (or even just a single platforms.yaml file I guess), and then cosa can just convert them to JSON and shove them into /boot?

@dustymabe
Copy link
Member

yay for YAML (at least in the most recent upload)

Any chance we can link to as much information as possible about why each platform/arch gets these values? platform documentation would be the most appropriate, but discovered knowledge (like investigation/discussion in GH issue comments) would also suffice. Basically it would be really nice in the future to know where to fish for similar information if we need to.

platforms.yaml Outdated Show resolved Hide resolved
@bgilbert
Copy link
Contributor Author

bgilbert commented Sep 2, 2021

Added comments, and a flag to disable the new behavior on x86_64 until a migration period has been announced. Ready for review.

@bgilbert bgilbert marked this pull request as ready for review September 2, 2021 03:32
platforms.yaml Outdated Show resolved Hide resolved
@dustymabe
Copy link
Member

Thanks so much for adding the context links. That will be really really helpful in the future.

dustymabe
dustymabe previously approved these changes Sep 2, 2021
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM - will try to test later today

@dustymabe
Copy link
Member

ok one final question. I went through and built without legacy: true. I do feel like we still need to set console=ttyS0 on our qemu image, no? coreos/fedora-coreos-tracker#567 is about removing the console entry for bare metal.

I understand that you said this file doesn't affect the qemu image, but I think it is worth discussing here. how we can or should behave towards qemu.

Note I'm specifically testing on x86_64 right now.

@bgilbert
Copy link
Contributor Author

bgilbert commented Sep 2, 2021

The #agreed in coreos/fedora-coreos-tracker#567 (comment) says "platform-specific default on other platforms" (and it looks as though QEMU was not specifically discussed in that meeting). At this point I strongly believe that on any platform that doesn't have specific requirements (e.g. AWS) we should provide no default console arguments and let users configure the system for their own needs. We have Ignition kernelArguments support to help with that. In particular with QEMU, new users will expect boot messages to go to the VGA console (e.g. in virt-manager or GNOME Boxes) and sending Ignition failures only to the serial console will continue to cause user confusion.

coreos/coreos-assembler#2400 includes code specifically to ensure that invocation of QEMU images through kola will not be affected by this change.

@dustymabe
Copy link
Member

The #agreed in coreos/fedora-coreos-tracker#567 (comment) says "platform-specific default on other platforms" (and it looks as though QEMU was not specifically discussed in that meeting). At this point I strongly believe that on any platform that doesn't have specific requirements (e.g. AWS) we should provide no default console arguments and let users configure the system for their own needs. We have Ignition kernelArguments support to help with that. In particular with QEMU, new users will expect boot messages to go to the VGA console (e.g. in virt-manager or GNOME Boxes) and sending Ignition failures only to the serial console will continue to cause user confusion.

yeah - considering we have legacy: true set for now we can haggle over other possible desired platform defaults in new issues/discussions.

dustymabe
dustymabe previously approved these changes Sep 2, 2021
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@bgilbert bgilbert marked this pull request as draft September 3, 2021 17:36
dustymabe added a commit to dustymabe/fedora-coreos-pipeline that referenced this pull request Sep 5, 2021
This is a workaround to get console=ttyS0,115200n8 into the
aarch64 AWS image. It does so by applying the following patch
to gf-platformid:

```diff
diff --git a/usr/lib/coreos-assembler/gf-platformid b/usr/lib/coreos-assembler/gf-platformid
index 2912b322c..36d089651 100755
--- a/usr/lib/coreos-assembler/gf-platformid
+++ b/usr/lib/coreos-assembler/gf-platformid
@@ -46,7 +46,11 @@ blscfg_path=$(coreos_gf glob-expand /boot/loader/entries/ostree-*.conf)
 coreos_gf download "${blscfg_path}" "${tmpd}"/bls.conf
 # Remove any platformid currently there
 sed -i -e 's, ignition.platform.id=[a-zA-Z0-9]*,,g' "${tmpd}"/bls.conf
-sed -i -e 's,^\(options .*\),\1 ignition.platform.id='"${platformid}"',' "${tmpd}"/bls.conf
+if [ "${platformid}" == 'aws' ]; then
+    sed -i -e 's|^\(options .*\)|\1 ignition.platform.id='"${platformid}"' console=ttyS0,115200n8|' "${tmpd}"/bls.conf
+else
+    sed -i -e 's,^\(options .*\),\1 ignition.platform.id='"${platformid}"',' "${tmpd}"/bls.conf
+fi
 coreos_gf upload "${tmpd}"/bls.conf "${blscfg_path}"

 if [ "$basearch" = "s390x" ] ; then
```

Once coreos/fedora-coreos-config#1181 and
coreos/coreos-assembler#2400 land then we
won't need this any longer.

This implements a fix for coreos/fedora-coreos-tracker#920
dustymabe added a commit to dustymabe/fedora-coreos-pipeline that referenced this pull request Sep 6, 2021
This is a workaround to get console=ttyS0,115200n8 into the
aarch64 AWS image. It does so by applying the following patch
to gf-platformid:

```diff
diff --git a/usr/lib/coreos-assembler/gf-platformid b/usr/lib/coreos-assembler/gf-platformid
index 2912b322c..36d089651 100755
--- a/usr/lib/coreos-assembler/gf-platformid
+++ b/usr/lib/coreos-assembler/gf-platformid
@@ -46,7 +46,11 @@ blscfg_path=$(coreos_gf glob-expand /boot/loader/entries/ostree-*.conf)
 coreos_gf download "${blscfg_path}" "${tmpd}"/bls.conf
 # Remove any platformid currently there
 sed -i -e 's, ignition.platform.id=[a-zA-Z0-9]*,,g' "${tmpd}"/bls.conf
-sed -i -e 's,^\(options .*\),\1 ignition.platform.id='"${platformid}"',' "${tmpd}"/bls.conf
+if [ "${platformid}" == 'aws' ]; then
+    sed -i -e 's|^\(options .*\)|\1 ignition.platform.id='"${platformid}"' console=ttyS0,115200n8|' "${tmpd}"/bls.conf
+else
+    sed -i -e 's,^\(options .*\),\1 ignition.platform.id='"${platformid}"',' "${tmpd}"/bls.conf
+fi
 coreos_gf upload "${tmpd}"/bls.conf "${blscfg_path}"

 if [ "$basearch" = "s390x" ] ; then
```

Once coreos/fedora-coreos-config#1181 and
coreos/coreos-assembler#2400 land then we
won't need this any longer.

This implements a fix for coreos/fedora-coreos-tracker#920
dustymabe added a commit to coreos/fedora-coreos-pipeline that referenced this pull request Sep 6, 2021
This is a workaround to get console=ttyS0,115200n8 into the
aarch64 AWS image. It does so by applying the following patch
to gf-platformid:

```diff
diff --git a/usr/lib/coreos-assembler/gf-platformid b/usr/lib/coreos-assembler/gf-platformid
index 2912b322c..36d089651 100755
--- a/usr/lib/coreos-assembler/gf-platformid
+++ b/usr/lib/coreos-assembler/gf-platformid
@@ -46,7 +46,11 @@ blscfg_path=$(coreos_gf glob-expand /boot/loader/entries/ostree-*.conf)
 coreos_gf download "${blscfg_path}" "${tmpd}"/bls.conf
 # Remove any platformid currently there
 sed -i -e 's, ignition.platform.id=[a-zA-Z0-9]*,,g' "${tmpd}"/bls.conf
-sed -i -e 's,^\(options .*\),\1 ignition.platform.id='"${platformid}"',' "${tmpd}"/bls.conf
+if [ "${platformid}" == 'aws' ]; then
+    sed -i -e 's|^\(options .*\)|\1 ignition.platform.id='"${platformid}"' console=ttyS0,115200n8|' "${tmpd}"/bls.conf
+else
+    sed -i -e 's,^\(options .*\),\1 ignition.platform.id='"${platformid}"',' "${tmpd}"/bls.conf
+fi
 coreos_gf upload "${tmpd}"/bls.conf "${blscfg_path}"

 if [ "$basearch" = "s390x" ] ; then
```

Once coreos/fedora-coreos-config#1181 and
coreos/coreos-assembler#2400 land then we
won't need this any longer.

This implements a fix for coreos/fedora-coreos-tracker#920
jschintag and others added 6 commits February 9, 2022 09:29
These tests have never run successfully on s390x. Skip them for now and
investigate wether or not they can be adapted for s390x.

Signed-off-by: Jan Schintag <jan.schintag@de.ibm.com>
SELinux denials are causing some issues and we need to have them looked
at. See coreos/fedora-coreos-tracker#1097
Since we're re-enabling `branched` we need these to apply
there too.
Can not reproduce `coreos.boot-mirror.luks` failed issue locally
 with rawhide after running 20 times, remove from denylist to see
if it happens.

See coreos/fedora-coreos-tracker#1092
We're going to rebase RHCOS 4.11, which picks up a new NM,
and the new case is actually the same.

BTW, minor rant here: We really need to stop defaulting to writing conditionals
that do `if is_fcos() {} else if is_rhcos() {}` because about 70%
of the time, this is actually trying to test "RHEL 8" versus "Fedora".

And in this case, what we want to dispatch on is the RHEL8 minor
version.  Or even the NetworkManager version.

Anyways for now, because we haven't updated the `redhat-release-coreos`
package because doing so is painful, dispatch on the OCP version.
coreosbot and others added 9 commits May 28, 2022 22:12
As mentioned in coreos/fedora-coreos-tracker#1209,
due to upstream changes, our current mechanism for testing
if RANDOM_TRUST_CPU is enabled is no longer valid. Lets instead
check the kernel config file to get the same information.

fixes: coreos/fedora-coreos-tracker#1209
…hide"

This reverts commit 13b9560.
coreos/fedora-coreos-tracker#1209 has been fixed by
#1764. We can once again
enable this test on rawhide.
We're seeing a weird failure there. Let's denylist it for now while
it is investigated.

See coreos/fedora-coreos-tracker#1215
Upstream NM started adding a readme-ifcfg-rh.txt file to explain
and encourage people to use keyfiles instead. This file doesn't
do anything, but breaks our logic that tries to determine if the
user provided networking config to the machine.

This file was added upstream in
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/commit/96d7362
Add a check to the no-default-initramfs-net-propagation test to make
sure there aren't any unknown files in /etc/sysconfig/network-scripts/.
This will allow us to drop the a corresponding test from mantle/kola [1].

[1] https://github.com/coreos/coreos-assembler/blob/5ee768b7ea069c08a7fa0b5b36dbb03bc9cd19ee/mantle/kola/tests/coretest/core.go#L251-L264
@bgilbert
Copy link
Contributor Author

bgilbert commented Jun 3, 2022

Added a kola test.

tests/kola/files/console-config Outdated Show resolved Hide resolved
tests/kola/files/console-config Outdated Show resolved Hide resolved
tests/kola/files/console-config Outdated Show resolved Hide resolved
cosa create_disk.sh and gf-set-platform will read this.  create_disk.sh
will also save a JSON version for the relevant architecture to
/boot/coreos/platforms.json for coreos-installer.
@bgilbert
Copy link
Contributor Author

bgilbert commented Jun 3, 2022

Updated!

dustymabe
dustymabe previously approved these changes Jun 4, 2022
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link

github-actions bot commented Jun 9, 2022

⛔ The next-devel branch is currently closed. PRs should target only testing-devel.

@bgilbert bgilbert closed this Jun 9, 2022
@bgilbert bgilbert deleted the platforms/testing-devel branch June 9, 2022 22:56
@bgilbert bgilbert restored the platforms/testing-devel branch June 9, 2022 22:57
@bgilbert bgilbert deleted the platforms/testing-devel branch June 9, 2022 22:57
@bgilbert
Copy link
Contributor Author

bgilbert commented Jun 9, 2022

Today's lesson: if a PR's head branch is renamed through the GitHub UI, GitHub closes the PR rather than following the rename. 😞

Continuation in #1781.

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.