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

Updated base image and some more functionality #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ifireball
Copy link

  1. Rebased the image on the centos:7.7.1908 image
  2. Moved exported env var configuration from a file to an env var in the
    Dockefile
  3. Added a script for storing the command-line arguments passed to the
    container to a file before starting up systemd
  4. Made systemd and journald output go to /dev/console which should be
    collected by the container engine automatically

Note that certain versions of Docker have an issue with collecting
/dev/console properly. See the following for explanation:

Signed-off-by: Barak Korren [email protected]

1. Rebased the image on the centos:7.7.1908 image
2. Moved exported env var configuration from a file to an env var in the
   Dockefile
3. Added a script for storing the command-line arguments passed to the
   container to a file before starting up systemd
4. Made systemd and journald output go to `/dev/console` which should be
   collected by the container engine automatically

Note that certain versions of Docker have an issue with collecting
`/dev/console` properly. See the following for explanation:

- systemd/systemd#4262
- moby/moby#27202
- https://bugzilla.redhat.com/show_bug.cgi?id=1373780

Signed-off-by: Barak Korren <[email protected]>
main() {
local export_path="${1:?}"
local include
Copy link
Member

Choose a reason for hiding this comment

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

Not needed


include=$(join "${INCLUDE_LIST[@]}")
get_env_vars() {
include=$(join "$@")
Copy link
Member

Choose a reason for hiding this comment

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

Missing local

/usr/bin/tr \\000 \\n < /proc/1/environ |
grep -E "^($include)=" >> "$export_path"
chmod 0644 "$export_path"
sed -nre "/^($include)=/{s/ /\\\\ /;p}"
Copy link
Member

Choose a reason for hiding this comment

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

Can you please a comment which explains what this sed command does?

ENV ENV_EXPORT_PATH=/etc/ci-container.environment
ENV ARGS_EXPORT_PATH=/etc/ci-container.args
# A list of variables to be exported to $export_path
ENV ENV_INCLUDE_LIST="ENV_EXPORT_PATH ARGS_EXPORT_PATH"
Copy link
Member

Choose a reason for hiding this comment

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

This change implies that each image that is going to be based on the systemd image, and want to include it's own env vars, will have to redeclare the ENV_INCLUDE_LIST variable. This is risky since the ENV_EXPORT_PATH ARGS_EXPORT_PATH can be omitted when this happens.

@ifireball
Copy link
Author

@gbenhaim please look at PR #3, it takes things in a different direction, and if we merge it I'll probably abandon this one

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.

2 participants