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

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

Merged
merged 20 commits into from
Jul 24, 2017
Merged

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

merged 20 commits into from
Jul 24, 2017

Conversation

pavel-shirshov
Copy link
Contributor

We install supervisord from pip, which uses /usr/local/bin, instead of /usr/bin as default.

@@ -100,4 +100,4 @@ COPY ["supervisord.conf", "sshd.conf", "ptf_nn_agent.conf", "/etc/supervisor/con

Copy link
Collaborator

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

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

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

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

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

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

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

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

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

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

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.

@pavel-shirshov pavel-shirshov merged commit d898011 into sonic-net:master Jul 24, 2017
@pavel-shirshov pavel-shirshov deleted the pavelsh/fix_broken_ptf branch July 24, 2017 20:36
lguohan pushed a commit that referenced this pull request Mar 26, 2020
798ce2f [multi-asic]: Update reload of systemd services to support multi-asic platforms (#856)
6f51428 [Mellanox] Fix thermal control issue: use natural sort for fan status and thermal status (#836)
51d26ce [ntp]: support "show ntp" with mgmt vrf based on linux os version (#858)

Signed-off-by: SuvarnaMeenakshi <sumeenak@microsoft.com>
abdosi added a commit that referenced this pull request Jun 3, 2020
Make sure db_migrator is run after all config are loaded during (#926)
Vnet alias mapping (#924)
Changes to make lldp show command for multi-npu platforms. (#914)
[Mellanox] Fix thermal control issue: use natural sort for fan
status and thermal status (#836)
[Mellanox] add document for thermal control related cli (#832)
stepanblyschak pushed a commit to stepanblyschak/sonic-buildimage that referenced this pull request May 10, 2021
… and thermal status (sonic-net#836)

* [thermal fix] use natural sort for fan status and thermal status
* [thermal fix] set fan status to N/A when fan is removed
* Adjust header name for show platform temperature output
stepanblyschak added a commit to stepanblyschak/sonic-buildimage that referenced this pull request Jun 30, 2021
Changes:

```
3c485e5 [recorder] Fix incorrect attribute enum value capability query (sonic-net#843)
677ebca [sairedis] Client/Server support zmq configuration file (sonic-net#845)
7c70e34 [sairedis] Add support for bulk api in client/server (sonic-net#844)
76d28a6 [pyext] Use SAI autogenerated saiswig.i (sonic-net#837)
9949c48 [vslib] implement query for SAI_DEBUG_COUNTER_TYPE enum values (sonic-net#842)
e385212 [MPLS] Minor tweaks to VS for MPLS support for CRM polling of MPLS In-segments and NHs.
d819f97 [meta] Add support for ignored attributes names (sonic-net#836)
c163238 Add cisco-8000 checks to syncd_init_common (sonic-net#839)
9aed2ff [sairedis] Add support for client server architecture (sonic-net#838)
```

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
liat-grozovik pushed a commit that referenced this pull request Jul 7, 2021
Changes:

3c485e5 [recorder] Fix incorrect attribute enum value capability query (#843)
677ebca [sairedis] Client/Server support zmq configuration file (#845)
7c70e34 [sairedis] Add support for bulk api in client/server (#844)
76d28a6 [pyext] Use SAI autogenerated saiswig.i (#837)
9949c48 [vslib] implement query for SAI_DEBUG_COUNTER_TYPE enum values (#842)
e385212 [MPLS] Minor tweaks to VS for MPLS support for CRM polling of MPLS In-segments and NHs.
d819f97 [meta] Add support for ignored attributes names (#836)
c163238 Add cisco-8000 checks to syncd_init_common (#839)
9aed2ff [sairedis] Add support for client server architecture (#838)

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
xumia pushed a commit that referenced this pull request Jul 8, 2021
Changes:

3c485e5 [recorder] Fix incorrect attribute enum value capability query (#843)
677ebca [sairedis] Client/Server support zmq configuration file (#845)
7c70e34 [sairedis] Add support for bulk api in client/server (#844)
76d28a6 [pyext] Use SAI autogenerated saiswig.i (#837)
9949c48 [vslib] implement query for SAI_DEBUG_COUNTER_TYPE enum values (#842)
e385212 [MPLS] Minor tweaks to VS for MPLS support for CRM polling of MPLS In-segments and NHs.
d819f97 [meta] Add support for ignored attributes names (#836)
c163238 Add cisco-8000 checks to syncd_init_common (#839)
9aed2ff [sairedis] Add support for client server architecture (#838)

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
Changes:

3c485e5 [recorder] Fix incorrect attribute enum value capability query (sonic-net#843)
677ebca [sairedis] Client/Server support zmq configuration file (sonic-net#845)
7c70e34 [sairedis] Add support for bulk api in client/server (sonic-net#844)
76d28a6 [pyext] Use SAI autogenerated saiswig.i (sonic-net#837)
9949c48 [vslib] implement query for SAI_DEBUG_COUNTER_TYPE enum values (sonic-net#842)
e385212 [MPLS] Minor tweaks to VS for MPLS support for CRM polling of MPLS In-segments and NHs.
d819f97 [meta] Add support for ignored attributes names (sonic-net#836)
c163238 Add cisco-8000 checks to syncd_init_common (sonic-net#839)
9aed2ff [sairedis] Add support for client server architecture (sonic-net#838)

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
AidanCopeland pushed a commit to Metaswitch/sonic-buildimage that referenced this pull request Apr 14, 2022
During SAI development, some existing attributes could be deprecated and marked ignored in headers, but old value name still can be used in redis database in current running systems. When doing sonic upgrade, old value needs to be correctly interpreted and translated to new name. This PR addresses this issue.
mssonicbld added a commit that referenced this pull request Jan 10, 2024
…omatically (#17410)

#### Why I did it
src/sonic-swss-common
```
* 16bc247 - (HEAD -> master, origin/master, origin/HEAD) [tests] fix binary_data_get unit test (#841) (72 minutes ago) [Yakiv Huryk]
* b2480ad - Add SonicDBConfig::reset method (#843) (4 weeks ago) [ganglv]
* ab3ce86 - [Azp]: Fix azp dash dependency (#842) (5 weeks ago) [Ze Gan]
* 5d1fe2d - add support for binary data read for Table::get() (#836) (5 weeks ago) [Yakiv Huryk]
```
#### How I did it
#### How to verify it
#### Description for the changelog
mssonicbld added a commit that referenced this pull request May 31, 2024
…omatically (#19149)

#### Why I did it
src/sonic-swss-common
```
* 96ad341 - (HEAD -> 202311, origin/202311) [action] [PR:836] add support for binary data read for Table::get() (#836) (8 hours ago) [mssonicbld]
* 9d70e50 - [ci] Use requests==2.31.0 instead of latest version to avoid test failure. (#877) (#881) (17 hours ago) [mssonicbld]
```
#### How I did it
#### How to verify it
#### Description for the changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants