Improves logging for the chroot2 utility and introduces EVE_CONTAINER_NO_POWEROFF mode #3628

rouming · 2023-11-25T13:06:57Z

PR improves logging for the chroot2 utility by printing possible parameters. There is a customer container, which provides invalid WORKDIR in its environment, so that chdir() fails. EVE considers this as fatal (which is ok), but does not print what exact directory does not exist, which is nasty. This PR improves that by err() libc call.

PR propagates an error from to a chroot2 caller and init-initrd scripts prints the exit value. Valid expectations are that init (process which is executed by the following execvp() call) never exits, if it does - return an exit status. It is possible to get 0 (success) of a chroot2 execution status iif a spawned child process returned 0. If a child process was terminated by a signal or coredumped (same as terminated by a SIGSEGV), the chroot2 returns -1 (255).

The last few commits introduce EVE_CONTAINER_NO_POWEROFF flag. If container's init script misbehaves and exits, it becomes very difficult to debug such a container, because VM shuts down immediately by explicit poweroff call. The EVE_CONTAINER_NO_POWEROFF can be set in the application environment variables on the controller side and instead of calling /sbin/poweroff, init-initrd sleeps infinitely, waiting for attention from a developer.

CC: @jsfakian

codecov · 2023-11-25T13:28:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9589a8e) 19.37% compared to head (4c9ca80) 19.36%.
Report is 2 commits behind head on master.

❗ Current head 4c9ca80 differs from pull request most recent head 9a7c625. Consider uploading reports for the commit 9a7c625 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3628      +/-   ##
==========================================
- Coverage   19.37%   19.36%   -0.01%     
==========================================
  Files         232      232              
  Lines       50551    50551              
==========================================
- Hits         9796     9791       -5     
- Misses      40045    40050       +5     
  Partials      710      710

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pkg/xen-tools/initrd/chroot2.c

Patch improves logging for fatal error path printing all arguments out. Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>

rouming · 2023-11-27T11:49:04Z

Difference to previous version:

Add EVE_CONTAINER_NO_POWEROFF for easy debugging

rene

LGTM

shjala · 2023-11-27T14:16:44Z

LGTM.

OhmSpectator · 2023-11-27T14:54:44Z

pkg/xen-tools/initrd/chroot2.c

-    return 0;
+    child_pid = wait(&wstatus);
+    if (child_pid < 0)
+        err(-1, "waitpid() failed:");


The error message is outdated.

Indeed, thanks.

And do you intentionally leave the semicolon at the end? It's not critical at all, but just in case...

OhmSpectator · 2023-11-27T15:07:40Z

Except for one typo, it looks fine to me.
I would only suggest adding info on how and where to set the new variable: EVE_CONTAINER_NO_POWEROFF. It is not obvious at all how the init-initrd script is started and how to parameterise it.

rouming · 2023-11-27T15:09:14Z

Except for one typo, it looks fine to me. I would only suggest adding info on how and where to set the new variable: EVE_CONTAINER_NO_POWEROFF. It is not obvious at all how the init-initrd script is started and how to parameterise it.

The last commit covers that in the DEBUGGING doc

Wait for a child exit status and returns it to a caller of a chroor2 caller. Valid expectations are that init (process which is executed by the following execvp() call) never exits, if it does - return an exit status. It is possible to get 0 (success) of a chroot2 execution status iif a spawned child process returned 0. If a child process was terminated by a signal or coredumped (same as terminated by a SIGSEGV), the chroot2 returns -1 (255). Also patch replaces waitpid() on wait() for simplicity, since chroot2 starts only one child there is no reason to specify what child do we wait, wait() call waits for any child and is similar in this context. Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>

…ation Output all warnings and treat them as errors. This make chroot2.c and hacf.c compilation more strict. Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>

Add an explicit echo which warns about power off of the whole VM. Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>

…F is set If container's init script misbehaves and exits, it becomes very difficult to debug such a container, because VM shuts down immediately by explicit poweroff call. This patch introduces an EVE_CONTAINER_NO_POWEROFF flag, which can be set in the application environment variables on the controller side and instead of calling /sbin/poweroff init-initrd sleeps infinitely, waiting for attention from a developer. Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>

OhmSpectator · 2023-11-27T15:15:34Z

The last commit covers that in the DEBUGGING doc

I saw this change, but it only says

If EVE_CONTAINER_NO_POWEROFF=1 environment variable is set on the controller side.

I'm confused. What exactly does it mean, "is set on the controller side"?

rouming · 2023-11-27T15:20:28Z

The last commit covers that in the DEBUGGING doc

I saw this change, but it only says

If EVE_CONTAINER_NO_POWEROFF=1 environment variable is set on the controller side.

I'm confused. What exactly does it mean, "is set on the controller side"?

This is the "Add Custom Config Template" section of the edge application. Environment comes with the app config.

OhmSpectator · 2023-11-27T15:36:17Z

This is the "Add Custom Config Template" section of the edge application. Environment comes with the app config.

Thanks, now I get it!

docs/DEBUGGING.md

…ariable A few lines in the doc never hurts. Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>

eriknordmark

LGTM

rouming requested review from eriknordmark and shjala November 25, 2023 13:06

rouming requested a review from rene as a code owner November 25, 2023 13:06

github-actions bot requested review from OhmSpectator and rucoder November 25, 2023 13:07

rene reviewed Nov 27, 2023

View reviewed changes

pkg/xen-tools/initrd/chroot2.c Outdated Show resolved Hide resolved

pkg/xen-tools/initrd/chroot2.c Show resolved Hide resolved

rouming force-pushed the chroot2-more-logging branch from dd60173 to 3b128fa Compare November 27, 2023 10:56

github-actions bot requested a review from rene November 27, 2023 10:57

pkg/xen-tools/chroot2: add more explicit logging on error path

966e4ca

Patch improves logging for fatal error path printing all arguments out. Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>

rouming force-pushed the chroot2-more-logging branch from 3b128fa to 4897866 Compare November 27, 2023 11:03

rouming changed the title ~~Improves logging for the chroot2 utility~~ Improves logging for the chroot2 utility and introduces EVE_CONTAINER_NO_POWEROFF mode Nov 27, 2023

rouming force-pushed the chroot2-more-logging branch from 4897866 to 45761c9 Compare November 27, 2023 11:47

rouming force-pushed the chroot2-more-logging branch from 45761c9 to 4ff689a Compare November 27, 2023 12:27

rene approved these changes Nov 27, 2023

View reviewed changes

shjala approved these changes Nov 27, 2023

View reviewed changes

OhmSpectator reviewed Nov 27, 2023

View reviewed changes

rouming added 4 commits November 27, 2023 16:13

pkg/xen-tools/Dockerfile: provide -Wall and -Werror for C code compil…

8ed1816

…ation Output all warnings and treat them as errors. This make chroot2.c and hacf.c compilation more strict. Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>

pkg/xen-tools/init-initrd: explicitly warns about VM power off

57b5e93

Add an explicit echo which warns about power off of the whole VM. Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>

rouming force-pushed the chroot2-more-logging branch from 4ff689a to 4c9ca80 Compare November 27, 2023 15:16

github-actions bot requested review from OhmSpectator, rene and shjala November 27, 2023 15:16

eriknordmark reviewed Nov 27, 2023

View reviewed changes

docs/DEBUGGING.md Outdated Show resolved Hide resolved

OhmSpectator approved these changes Nov 27, 2023

View reviewed changes

docs/DEBUGGING: add a few lines about EVE_CONTAINER_NO_POWEROFF env v…

9a7c625

…ariable A few lines in the doc never hurts. Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>

rouming force-pushed the chroot2-more-logging branch from 4c9ca80 to 9a7c625 Compare November 28, 2023 10:50

github-actions bot requested review from eriknordmark and OhmSpectator November 28, 2023 10:50

OhmSpectator approved these changes Nov 28, 2023

View reviewed changes

eriknordmark approved these changes Nov 28, 2023

View reviewed changes

eriknordmark merged commit eccdcb6 into lf-edge:master Nov 28, 2023
32 of 41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improves logging for the chroot2 utility and introduces EVE_CONTAINER_NO_POWEROFF mode #3628

Improves logging for the chroot2 utility and introduces EVE_CONTAINER_NO_POWEROFF mode #3628

rouming commented Nov 25, 2023 •

edited

Loading

codecov bot commented Nov 25, 2023 •

edited

Loading

rouming commented Nov 27, 2023

rene left a comment

shjala commented Nov 27, 2023

OhmSpectator Nov 27, 2023

rouming Nov 27, 2023

OhmSpectator Nov 27, 2023

OhmSpectator commented Nov 27, 2023

rouming commented Nov 27, 2023

OhmSpectator commented Nov 27, 2023

rouming commented Nov 27, 2023

OhmSpectator commented Nov 27, 2023

eriknordmark left a comment

Improves logging for the chroot2 utility and introduces EVE_CONTAINER_NO_POWEROFF mode #3628

Improves logging for the chroot2 utility and introduces EVE_CONTAINER_NO_POWEROFF mode #3628

Conversation

rouming commented Nov 25, 2023 • edited Loading

codecov bot commented Nov 25, 2023 • edited Loading

Codecov Report

rouming commented Nov 27, 2023

rene left a comment

Choose a reason for hiding this comment

shjala commented Nov 27, 2023

OhmSpectator Nov 27, 2023

Choose a reason for hiding this comment

rouming Nov 27, 2023

Choose a reason for hiding this comment

OhmSpectator Nov 27, 2023

Choose a reason for hiding this comment

OhmSpectator commented Nov 27, 2023

rouming commented Nov 27, 2023

OhmSpectator commented Nov 27, 2023

rouming commented Nov 27, 2023

OhmSpectator commented Nov 27, 2023

eriknordmark left a comment

Choose a reason for hiding this comment

rouming commented Nov 25, 2023 •

edited

Loading

codecov bot commented Nov 25, 2023 •

edited

Loading