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

cloud.ks: Add tty0 console output for ignition logging #158

Merged
merged 1 commit into from
Jul 20, 2018

Conversation

Rahuls0720
Copy link
Contributor

Many platforms, like QEMU, use tty0 for console output. This PR adds tty0 console output to allow ignition logging/debugging.

Jira card: https://projects.engineering.redhat.com/browse/COREOS-202

@openshift-ci-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 6, 2018
@jlebon
Copy link
Member

jlebon commented Jul 9, 2018

Have you confirmed this fixes the issue? My memory is a bit fuzzy on this, but I remember looking into this some time ago and found systemd/systemd#3403. Basically, the order in which the console= arguments are given matters. I wonder if we also need systemd.show_status=1 for this?

@Rahuls0720 Rahuls0720 changed the title cloud.ks: Add tty0 console output for ignition logging [WIP] cloud.ks: Add tty0 console output for ignition logging Jul 9, 2018
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2018
@ashcrow
Copy link
Member

ashcrow commented Jul 10, 2018

@jlebon that's a good find. I believe @Rahuls0720 is attempting to build a local version of RHCOS with these changes. We chatted a bit about it yesterday but I wasn't able to help as much as I would have liked due to meetings.

@Rahuls0720 were you able to successfully create a local version of RHCOS with your change to test?

@Rahuls0720
Copy link
Contributor Author

Rahuls0720 commented Jul 10, 2018

I was able to run it locally using vagrant, but Ignition logs output just fine to both tty0 and tty1. So the card doesn't apply to vagrant. I will test it on qemu today hopefully

@crawford
Copy link
Contributor

@Rahuls0720 were you looking at the VGA output or serial output when you tested vagrant? Which virtualization backend were you using with vagrant?

@Rahuls0720
Copy link
Contributor Author

Rahuls0720 commented Jul 10, 2018

I used virsh to view the output. Oh, does it look at serial? :/

Copy link
Contributor

@crawford crawford left a comment

Choose a reason for hiding this comment

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

From the kernel docs: "Note that you can only define one console per device type (serial, video)."

Just replace tty1 with tty0.

@crawford
Copy link
Contributor

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2018
@ashcrow
Copy link
Member

ashcrow commented Jul 18, 2018

I spoke with @crawford today and it sounds like we should go with setting to tty0 and ttyS0.

@crawford Agree?

@crawford
Copy link
Contributor

@ashcrow As mentioned in the last comment, I think we should go with tty0 and ttyS0. I don't know what order they should be in (there are conflicting docs regarding the ordering).

@Rahuls0720
Copy link
Contributor Author

Rahuls0720 commented Jul 18, 2018

Does anyone know how to log the tty0 console output for either qemu or vagrant? I'm looking for something like -nographic or -serial file:FILENAME qemu flags that allows me to view and log the tty0 output (to ensure ignition ran).

@crawford mentioned one method of ensure ignition ran/logged is using an invalid url in the ignition config, but boot up halts at an emergency shell
screenshot from 2018-07-18 14-04-26

@ashcrow
Copy link
Member

ashcrow commented Jul 18, 2018

/cc @arithx

@crawford
Copy link
Contributor

@Rahuls0720 Use an Ignition config that references something from an address that is unreachable. Something like this should be interesting:

{
  "ignition": {
    "version": "2.2.0",
    "config": {
      "append": [{
        "source": "http://localhost"
      }]
    }
  }
}

I haven't validated that or tested it.

@arithx
Copy link
Contributor

arithx commented Jul 19, 2018

In kola we use -chardev file,id=log,path=<path_to_log> to capture the serial console on QEMU (https://github.com/coreos/mantle/blob/master/platform/machine/qemu/cluster.go#L189) but I'm not sure about specifically capturing the tty0

@Rahuls0720
Copy link
Contributor Author

Rahuls0720 commented Jul 19, 2018

@jlebon Do you know the reason tty1 is being used?

Also, the last console set in the kernel command line arg is the one systemd logs everything to (this console has the C flag in /proc/consoles). The other consoles only get the kernel logs. If tty1 is needed, console=tty1 console=ttyS0,115200n8 console=tty0 should do the trick. Otherwise console=ttyS0,115200n8 console=tty0.

Let me know your thoughts on this!

@jlebon
Copy link
Member

jlebon commented Jul 19, 2018

@Rahuls0720 It's just inherited from the Fedora kickstart ours is based on.

So, reading systemd/systemd#3403 again, it sounds like there's no way right now to get systemd messages to go to all consoles without Plymouth? If that's the case, then I'd vote for ttyS0 rather than tty0 being the last argument. E.g. cloud providers make it easy to access those logs and locally we can use virsh console (which I've found super useful exactly wrt. debugging ignition).

@Rahuls0720
Copy link
Contributor Author

Rahuls0720 commented Jul 19, 2018

I'm a bit confused on Plymouth. Plymouth is part of the initramfs and RHCOS doesn't include Plymouth. So why do boot up messages still appear on both ttyS0 and tty0?

Can you use virsh console for tty0? I thought it only displays serial output

@jlebon
Copy link
Member

jlebon commented Jul 19, 2018

Right, virsh console connects to the serial console. Since it's already set as the last console= argument, systemd messages show up there. I thought @crawford wanted to make them appear on both of them. Though reading the original Jira ticket now, it looks like the goal was to see journal output such as ignition, which definitely works today with virsh console.

I also tried running QEMU directly and got ignition log messages using e.g. -chardev file,id=log,path=/tmp/log.txt -serial chardev:log like kola does. @crawford Does that not work for you?

@ashcrow
Copy link
Member

ashcrow commented Jul 19, 2018

Related coreos/bugs#2136

@Rahuls0720
Copy link
Contributor Author

Rahuls0720 commented Jul 19, 2018

@crawford @ashcrow @jlebon I believe this should solve the problem.

Just to ensure everyone is on the same page, I replaced tty1 with tty0 and moved tty0 as the last console in the kernel args. My understanding is tty0 should look identical to ttyS0. Currently tty0 looks identical to ttyS0, except that tty0 has ignition logs . The journald.conf option 'forwardtoconsole' is included in RHCOS, but doesn't work with ignition which is why tty0 and ttyS0 differ. Please see coreos/bugs#2136 for details about this ignition bug.

@crawford
Copy link
Contributor

@jlebon I was watching the VGA console (tty0).

@Rahuls0720 We need to use ForwardToKMsg so it shows up on both.

@Rahuls0720
Copy link
Contributor Author

Rahuls0720 commented Jul 20, 2018

@jlebon @crawford Can you give this a lgtm label if no other changes are needed?

@crawford
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2018
@crawford
Copy link
Contributor

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 20, 2018
@crawford
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 20, 2018
@Rahuls0720 Rahuls0720 changed the title [WIP] cloud.ks: Add tty0 console output for ignition logging cloud.ks: Add tty0 console output for ignition logging Jul 20, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 20, 2018
@openshift-merge-robot openshift-merge-robot merged commit 632b59c into openshift:master Jul 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants