Hi @LenkaSeg. Thanks for your PR.

I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/ok-to-test

@LenkaSeg: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Full PR test history. Your PR dashboard.

Still working on this.

To confirm that the changes in this PR are merging the CLI flags into config:

bin/crio --pinns-path=$(pwd)/bin/pinns

From a different terminal:

bin/crio status config print | grep log_level

Output:

log_level = "debug"

Let's do the hot reload:

kill -HUP $(pgrep crio)

Check the log level again:

bin/crio status config print | grep log_level

Output (is still debug):

log_level = "debug"

To confirm that the changes in this PR are merging the CLI flags into config:

bin/crio --pinns-path=$(pwd)/bin/pinns

From a different terminal:

bin/crio status config print | grep log_level

Output:

log_level = "debug"

Let's do the hot reload:

kill -HUP $(pgrep crio)

Check the log level again:

bin/crio status config print | grep log_level

Output (is still debug):

log_level = "debug"

@LenkaSeg very nice!

How did you set the log level here? Was CLI using a different level, and crio.conf had a different one? Looking at the above, I am not sure which way it was. 😄

How did you set the log level here? Was CLI using a different level, and crio.conf had a different one? Looking at the above, I am not sure which way it was. 😄

Ah, sorry, the log level was passed as a cli argument. The first command was supposed to be:

sudo bin/crio --pinns-path=$(pwd)/bin/pinns --log-level=debug

I copied the wrong line :)

The conf had log_level set to "warn":

lenkaseg@fedora:$ cat /etc/crio/crio.conf.d/99-log-level.conf
[crio.runtime]
log_level = "warn"

@LenkaSeg, looks good so far! Thank you for persevering with this.

We might have to update some tests. Also, if you could rebase your branch to get some of the latest updates, then it would have with some other test failures we have here.

Considering the "debug" option here:

Would it be ok to disable those failing tests?
Or create a different bats file, add the test covering this change and customize start_crio?

Considering the "debug" option here:

cri-o/test/helpers.bash

Line 222 in d622521

-l debug \

Would it be ok to disable those failing tests? Or create a different bats file, add the test covering this change and customize start_crio?

@LenkaSeg, we seem to have some behaviour change now, which probably makes sense since the previous (old) behaviour was a bit buggy, so to speak. So now we need to figure out why we have these failing tests and how to update them so that these would pass.

If we delete a test, we can lose some signal unless we are sure that the test is no longer valid or feasible.

What do you think?

That said, I didn't have a look, yet, what the issues are. 🙇

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: LenkaSeg
Once this PR has been reviewed and has the lgtm label, please assign saschagrunert for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.