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/snmp] Allow snmp container to use python smbus #2772

Merged
merged 1 commit into from
Apr 16, 2019

Conversation

Staphylo
Copy link
Collaborator

- What I did

The python3-smbus module is installed on the host and shared to the snmp
container via a docker mount.
This is a hack to prevent snmp from installing build-essential to build
a single cpython module.
Once the snmp docker image uses stretch, the python3-smbus should be available there directly.

- How I did it

Installed python3-smbus in the host.
Added a mountpoint for the cpython lib in snmp.

build_debian.sh Outdated
@@ -249,6 +249,10 @@ sudo LANG=C DEBIAN_FRONTEND=noninteractive chroot $FILESYSTEM_ROOT apt-get -y in
locales \
cgroup-tools

## Note: temporary workaround to allow snmp container to import smbus for arista
sudo LANG=C DEBIAN_FRONTEND=noninteractive chroot $FILESYSTEM_ROOT apt-get -y install \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we install python3-smbus to the snmp docker itself?
e,g, change this file:
https://github.com/Azure/sonic-buildimage/blob/master/dockers/docker-snmp-sv2/Dockerfile.j2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As you can see this Dockerfile extends docker-config-engine which extends docker-base which is jessie based. In jessie there is no such thing as python3-smbus. Stretch will have it once all the docker images will be upgraded to this version.

Also there will be another blocker since we build our own version of python3.6 for snmp. Stretch currently uses python3.5. It likely means that installing python3-smbus will only make it available for python3.5 and not python3.6.

I tried to confine the hack to the snmp container by manually installing the stretch python3-smbus in jessie but that generated a lot of errors so I abandoned this idea. We will be tracking the evolution of all these component and remove these mitigations once we can find a sound solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please use 'pip3.6 install' it from pypi? Then you don't worry about python3.5/3.6, nor about jessie/stretch.


In reply to: 274686749 [](ancestors = 274686749)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should have left a bigger comment for this change.
The smbus module is a cpython module and therefore require at least build-essentials and python-dev to compile as part of pip3.6 install.
I felt it was overkill to install the build toolchain install the module and remove the toolchain in the Dockerfile.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually we have similar logic in dockers/docker-snmp-sv2/Dockerfile.j2 already:

  1. install gcc
  2. pip install hiredis/pyyaml
  3. remove gcc
    Could you please try this `overkill' method? I prefer it because of binary compatibility issue between Jessie container and Stretch host.

In reply to: 274727333 [](ancestors = 274727333)

@@ -14,3 +14,4 @@ $(DOCKER_SNMP_SV2)_RUN_OPT += --net=host --privileged -t
$(DOCKER_SNMP_SV2)_RUN_OPT += -v /etc/sonic:/etc/sonic:ro
# mount Arista platform python libraries to support corresponding platforms SNMP power status query
$(DOCKER_SNMP_SV2)_RUN_OPT += -v /usr/lib/python3/dist-packages/arista:/usr/lib/python3/dist-packages/arista:ro
$(DOCKER_SNMP_SV2)_RUN_OPT += -v /usr/lib/python3/dist-packages/smbus.cpython-35m-x86_64-linux-gnu.so:/usr/lib/python3/dist-packages/smbus.cpython-36m-x86_64-linux-gnu.so
Copy link
Collaborator

@qiluo-msft qiluo-msft Apr 12, 2019

Choose a reason for hiding this comment

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

DOCKER_SNMP_SV2 [](start = 2, length = 15)

Sorry for missing the context.
Why snmp docker needs smbus? which component needs? for what purpose? #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On the 7060CX-32S we have to query the power supply status from an smbus device.
We therefore have to use the smbus module to do such query.

Snmp currently loads the platform plugins to provide power supply information, without this fix snmp-agent will throw a backtrace due to the missing module.
Hopefully this is just a temporary fix until STATE_DB contains the power supply information and snmp makes use of it rather than calling into a platform specific script.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add comment like:

Only for Arista 7060CX-32

temporary workaround to allow snmp container to import smbus for arista


In reply to: 274726238 [](ancestors = 274726238)

Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

As comments

@yxieca yxieca merged commit 539d4ff into sonic-net:master Apr 16, 2019
yxieca pushed a commit that referenced this pull request Apr 16, 2019
MichelMoriniaux pushed a commit to criteo-forks/sonic-buildimage that referenced this pull request May 28, 2019
@Staphylo Staphylo deleted the snmp-psu-smbus branch December 6, 2022 15:00
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Jun 5, 2023
Update sonic-utilities submodule pointer to include the following:
* 5c9b2177 Fix issue: out of range sflow polling interval is accepted and stored in config_db ([sonic-net#2847](sonic-net/sonic-utilities#2847))
* 72ca4848 Add CLI configuration options for teamd retry count feature ([sonic-net#2642](sonic-net/sonic-utilities#2642))
* 359dfc0c [Clock] Implement clock CLI ([sonic-net#2793](sonic-net/sonic-utilities#2793))
* b316fc27 Add transceiver status CLI to show output from TRANSCEIVER_STATUS table ([sonic-net#2772](sonic-net/sonic-utilities#2772))
* dc59dbd2 Replace pickle by json ([sonic-net#2849](sonic-net/sonic-utilities#2849))
* a66f41c4 [show] replace shell=True, replace xml by lxml, replace exit by sys.exit ([sonic-net#2666](sonic-net/sonic-utilities#2666))
* 57500572 [utilities_common] replace shell=True ([sonic-net#2718](sonic-net/sonic-utilities#2718))
* 6e0ee3e7 [CRM][DASH] Extend CRM utility to support DASH resources. ([sonic-net#2800](sonic-net/sonic-utilities#2800))
* b2c29b0b [config] Generate sysinfo in single asic ([sonic-net#2856](sonic-net/sonic-utilities#2856))

Signed-off-by: dprital <drorp@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants