Skip to content

[docker-ptf]: Update entrypoint entry for docker-ptf #836

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.

Sign up for GitHub

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

Merged
pavel-shirshov merged 20 commits into from
Jul 24, 2017
Merged

[docker-ptf]: Update entrypoint entry for docker-ptf #836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
4ea6318
Fix SAI submodule id
pavel-shirshov May 30, 2017
991124c
Merge remote-tracking branch 'upstream/master'
pavel-shirshov May 30, 2017
219c4d0
Merge remote-tracking branch 'upstream/master'
pavel-shirshov May 31, 2017
1db3b3d
Merge remote-tracking branch 'upstream/master'
pavel-shirshov Jun 1, 2017
ebb58b6
Merge remote-tracking branch 'upstream/master'
pavel-shirshov Jun 10, 2017
f1a9ca2
Merge remote-tracking branch 'upstream/master'
pavel-shirshov Jun 12, 2017
cd5f07f
Merge remote-tracking branch 'upstream/master'
pavel-shirshov Jun 27, 2017
045ae9d
Merge remote-tracking branch 'upstream/master'
pavel-shirshov Jun 29, 2017
19adf1e
Merge remote-tracking branch 'upstream/master'
pavel-shirshov Jul 1, 2017
832e2d4
Merge branch 'master' of https://github.com/Azure/sonic-buildimage
pavel-shirshov Jul 5, 2017
4cf1e74
Install python modules from pypi not from debian repo
pavel-shirshov Jul 5, 2017
07ee0f7
Remove get-pip and don't install parallel-ssh
pavel-shirshov Jul 5, 2017
4f5795d
Use latest cffi
pavel-shirshov Jul 6, 2017
cf8e803
Update comments
pavel-shirshov Jul 6, 2017
87fbfb3
Merge branch 'master' of https://github.com/Azure/sonic-buildimage
pavel-shirshov Jul 14, 2017
6487c00
Merge branch 'master' of https://github.com/Azure/sonic-buildimage
pavel-shirshov Jul 20, 2017
7d7a7b7
Merge branch 'master' of https://github.com/Azure/sonic-buildimage
pavel-shirshov Jul 24, 2017
f26cb50
Updated entrypoint for docker-ptf container
pavel-shirshov Jul 24, 2017
3ead64f
Add main supervisord.conf file under /etc/supervisor. Run supervisord…
pavel-shirshov Jul 24, 2017
eb1bd42
Use correct syntax for entrypoint
pavel-shirshov Jul 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions dockers/docker-ptf/Dockerfile.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,11 @@ RUN mkdir /var/run/sshd \
&& sed -ri 's/UsePAM yes/#UsePAM yes/g' /etc/ssh/sshd_config \
&& sed -i '$aUseDNS no' /etc/ssh/sshd_config

COPY ["supervisord.conf", "sshd.conf", "ptf_nn_agent.conf", "/etc/supervisor/conf.d/"]
COPY ["supervisord.conf", "/etc/supervisor/"]
COPY ["conf.d/supervisord.conf", "conf.d/sshd.conf", "conf.d/ptf_nn_agent.conf", "/etc/supervisor/conf.d/"]

RUN mkdir -p /var/log/supervisor

Copy link
Collaborator

@lguohan lguohan Jul 24, 2017

Choose a reason for hiding this comment

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

it looks for supervisord.conf in /etc/supervisor/

Copy link
Contributor

@jleveque jleveque Jul 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

There is a "main" config file in /etc/supervisor/, and that file includes the /etc/supervisor/conf.d/ path, which is where the docker-specific config file lives.

I will say that naming the docker-specific config file "supervisor.conf" does make things a bit confusing. We could rename the docker-specific config files at some point to make it a bit more obvious.

Copy link
Contributor Author

@pavel-shirshov pavel-shirshov Jul 24, 2017

Choose a reason for hiding this comment

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

I can rename conf.d/supervisord.conf file to something else. But I think it's good enough name to show that we're changing supervisord configuration in the file.

Copy link
Contributor

@jleveque jleveque Jul 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Currently, the conf.d/ file is only partial configuration, with the remaining (shared) configuration in /etc/supervisord.conf. Thus, I feel like the partial config file in conf.d should have a name specific to what it's configuring (along the lines of "container.conf"). This is a common convention for all /etc/*/conf.d/ config files. The fact that it resides under /etc/supervisor/conf.d/ should be enough to signify that it is modifying the supervisord configuration. See my comment below for more.

EXPOSE 22

ENTRYPOINT ["/usr/bin/supervisord"]
ENTRYPOINT ["/usr/local/bin/supervisord", "-c", "/etc/supervisor/supervisord.conf"]
Copy link
Contributor

@jleveque jleveque Jul 24, 2017

Choose a reason for hiding this comment

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

Why do you need to specify the config file? As I mentioned above, the main config file, /etc/supervisor/supervisord.conf should include all conf files under /etc/supervisor/conf.d/. Is this not working?

Copy link
Contributor Author

@pavel-shirshov pavel-shirshov Jul 24, 2017

Choose a reason for hiding this comment

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

Otherwise it gives following in the log
/usr/local/lib/python2.7/dist-packages/supervisor/options.py:298: UserWarning: Supervisord is running as root and it is searching for its configuration file in default locations (including its current working directory); you probably want to specify a "-c" argument specifying an absolute path to a configuration file for improved security.
'Supervisord is running as root and it is searching '

So I provide -c to suppress this message.

Copy link
Contributor

@jleveque jleveque Jul 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I see. Just note that in specifying the config file, supervisor is no longer loading /etc/supervisor.conf, which you modified below. None of the configuration in that file will be loaded. If you need config from both, you'll either have to let the main file include the conf.d file, or copy all pertinent config into the conf.d file, in which case, you should delete the main file to prevent confusion. Personally, I'm OK with the warning message, at least for now.

Copy link
Collaborator

@lguohan lguohan Jul 24, 2017

Choose a reason for hiding this comment

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

is this addressed now?

Copy link
Contributor Author

@pavel-shirshov pavel-shirshov Jul 24, 2017

Choose a reason for hiding this comment

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

There is no /etc/supervisor.conf file.
There're:
/etc/supervisor/supervisord.conf file with full configuration
/etc/supervisor/conf.d/supervisord.conf with one-line adjustment.
I use "-c /etc/supervisor/supervisord.conf" which loads three other configurations with include.

Output from log
2017-07-24 20:53:00,652 CRIT Supervisor running as root (no user in config file)
2017-07-24 20:53:00,652 INFO Included extra file "/etc/supervisor/conf.d/ptf_nn_agent.conf" during parsing
2017-07-24 20:53:00,652 INFO Included extra file "/etc/supervisor/conf.d/sshd.conf" during parsing
2017-07-24 20:53:00,652 INFO Included extra file "/etc/supervisor/conf.d/supervisord.conf" during parsing
2017-07-24 20:53:00,665 INFO RPC interface 'supervisor' initialized
2017-07-24 20:53:00,665 CRIT Server 'unix_http_server' running without any HTTP authentication checking
2017-07-24 20:53:00,665 INFO supervisord started with pid 7
2017-07-24 20:53:01,667 INFO spawned: 'sshd' with pid 10
2017-07-24 20:53:01,669 INFO spawned: 'ptf_nn_agent' with pid 11
2017-07-24 20:53:02,670 INFO success: sshd entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2017-07-24 20:53:02,671 INFO success: ptf_nn_agent entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
root@24850f76d0a1:/# supervisorctl status
ptf_nn_agent RUNNING pid 11, uptime 0:01:00
sshd RUNNING pid 10, uptime 0:01:00

Copy link
Contributor Author

@pavel-shirshov pavel-shirshov Jul 24, 2017

Choose a reason for hiding this comment

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

So I don't know what to address here.

Copy link
Contributor

@jleveque jleveque Jul 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for any confusion. I thought you were passing "/etc/supervisor/conf.d/supervisord.conf" as the main config file. I got confused by the filenames, even after commenting about it earlier!

Supervisor looks for its default configuration in a few locations, among them, its current working directory, /etc/supervisord.conf, and /etc/supervisor/supervisord.conf. Thus, in our case, /etc/supervisor/supervisord.conf is the default config file, so using "-c /etc/supervisor/supervisord.conf" will load the default config plus the includes, as you described.

0 dockers/docker-ptf/ptf_nn_agent.conf → dockers/docker-ptf/conf.d/ptf_nn_agent.conf
View file
Open in desktop
File renamed without changes.
0 dockers/docker-ptf/sshd.conf → dockers/docker-ptf/conf.d/sshd.conf
View file
Open in desktop
File renamed without changes.
2 changes: 2 additions & 0 deletions dockers/docker-ptf/conf.d/supervisord.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[supervisord]
nodaemon=true
28 changes: 27 additions & 1 deletion dockers/docker-ptf/supervisord.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,28 @@
; supervisor config file

[unix_http_server]
file=/var/run/supervisor.sock ; (the path to the socket file)
chmod=0700 ; sockef file mode (default 0700)

[supervisord]
nodaemon=true
logfile=/var/log/supervisor/supervisord.log ; (main log file;default $CWD/supervisord.log)
pidfile=/var/run/supervisord.pid ; (supervisord pidfile;default supervisord.pid)
childlogdir=/var/log/supervisor ; ('AUTO' child log dir, default $TEMP)

; the below section must remain in the config file for RPC
; (supervisorctl/web interface) to work, additional interfaces may be
; added by defining them in separate rpcinterface: sections
[rpcinterface:supervisor]
supervisor.rpcinterface_factory = supervisor.rpcinterface:make_main_rpcinterface

[supervisorctl]
serverurl=unix:///var/run/supervisor.sock ; use a unix:// URL for a unix socket

; The [include] section can just contain the "files" setting. This
; setting can list multiple files (separated by whitespace or
; newlines). It can also contain wildcards. The filenames are
; interpreted as relative to this file. Included files *cannot*
; include files themselves.

[include]
files = /etc/supervisor/conf.d/*.conf