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

Enable logging to the systemd journal if a user config is provided #958

Merged
merged 2 commits into from
Apr 14, 2020

Conversation

sohankunkerkar
Copy link
Member

Fixes #903

@coreosbot
Copy link
Contributor

Can one of the admins verify this patch?

internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
@sohankunkerkar sohankunkerkar force-pushed the ign-journal branch 2 times, most recently from 64d27d4 to 6b6d860 Compare April 7, 2020 20:50
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Would be cool to add a test for this! And actually, with the new "external" host tests, this could be done pretty easily by just adding a script to tests/kola/ that greps the journal for messages with the given ID.

More generic testing where e.g. you provided referenced user configs or you don't provide a user config at all would require an internal kola test in cosa though.

internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
@sohankunkerkar sohankunkerkar force-pushed the ign-journal branch 2 times, most recently from 7e4ee81 to 29f08f6 Compare April 9, 2020 15:37
internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
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 - a few nit/optional comments

@sohankunkerkar sohankunkerkar changed the title WIP: Enable logging to the systemd journal if a user config is provided Enable logging to the systemd journal if a user config is provided Apr 14, 2020
internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
@sohankunkerkar sohankunkerkar force-pushed the ign-journal branch 2 times, most recently from c74ecbd to 250bcf4 Compare April 14, 2020 18:01
internal/exec/engine.go Outdated Show resolved Hide resolved
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Just a final message tweak suggestion, but LGTM overall!

internal/exec/engine.go Outdated Show resolved Hide resolved
internal/exec/engine.go Outdated Show resolved Hide resolved
@sohankunkerkar sohankunkerkar force-pushed the ign-journal branch 2 times, most recently from f7c6f55 to 8f4bfd5 Compare April 14, 2020 19:03
@dustymabe
Copy link
Member

LGTM

@sohankunkerkar sohankunkerkar merged commit 110130e into coreos:master Apr 14, 2020
jlebon added a commit to jlebon/ignition that referenced this pull request Jul 16, 2020
Regression from coreos#958. We switched the list of providers from an array to
a map. But iteration order through a map is undefined, so we lost the
precedence of providers.

I think this is the cause behind a lot of the FCOS installer test
timeouts, such as:

coreos/coreos-assembler#1597

There, we pass the Ignition config for the PXE boot via
`ignition.config.url`, but if the metal (no-op) fetcher appears earlier
than the `cmdline` fetcher, we get no config. And similarly for the
installed system when the no-op fetcher appears before the `system`
fetcher (which coreos-installer's `--ignition-file` leverages).

The likelihood of this happening increased in the v2.4.0 release due to
coreos#1002, which only gave us one try
to iterate over the correct provider first (at the `fetch` stage),
rather than every stage having a go at it.

Closes: coreos/coreos-assembler#1597
jlebon added a commit to jlebon/ignition that referenced this pull request Jul 16, 2020
Regression from coreos#958. We switched the list of providers from an array to
a map. But iteration order through a map is undefined, so we lost the
precedence of providers.

I think this is the cause behind a lot of the FCOS installer test
timeouts, such as:

coreos/coreos-assembler#1597

There, we pass the Ignition config for the PXE boot via
`ignition.config.url`, but if the metal (no-op) fetcher appears earlier
than the `cmdline` fetcher, we get no config. And similarly for the
installed system when the no-op fetcher appears before the `system`
fetcher (which coreos-installer's `--ignition-file` leverages).

The likelihood of this happening increased in the v2.4.0 release due to
coreos#1002, which only gave us one try
to iterate over the correct provider first (at the `fetch` stage),
rather than every stage having a go at it.

Closes: coreos/coreos-assembler#1597
@sohankunkerkar sohankunkerkar deleted the ign-journal branch August 6, 2020 23:27
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.

RFE: Provide a way for apps to determine whether a user config was provided
4 participants