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

Install python-redis package to docker containers #14632

Merged

Conversation

liuh-80
Copy link
Contributor

@liuh-80 liuh-80 commented Apr 13, 2023

Install python-redis package to docker containers

Why I did it

This this bug: #14531
The 'flush_unused_database' is part of docker-database, and docker-database does not install python-redis package by itself. it's using redis installed by sonic-py-swsssdk.
So after remove sonic-py-swsssdk from container, this script break.

To this this bug and avoid similer bug happen again, install python-redis to docker containers which removed sonic-py-swsssdk .

How I did it

Install python-redis to containers.

How to verify it

Pass all UT.
Create new UT to cover this scenario: sonic-net/sonic-mgmt#8032

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211

Description for the changelog

Improve sudo cat command for RO user.

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@liuh-80 liuh-80 changed the title [POC] Add redis package to docker containers [POC] Install python-redis package to docker containers Apr 13, 2023
@liuh-80 liuh-80 changed the title [POC] Install python-redis package to docker containers Install python-redis package to docker containers Apr 13, 2023
@liuh-80 liuh-80 marked this pull request as ready for review April 13, 2023 06:11
@qiluo-msft qiluo-msft requested a review from xumia April 13, 2023 06:56
@qiluo-msft
Copy link
Collaborator

Is it true that only flush_unused_database inside docker-database uses redis python package? If yes, we can change only docker-database Dockerfile.

Is it possible to replace this dependency by libswsscommon? If yes, create a new issue to track.

@liuh-80
Copy link
Contributor Author

liuh-80 commented Apr 13, 2023

Is it true that only flush_unused_database inside docker-database uses redis python package? If yes, we can change only docker-database Dockerfile.

Is it possible to replace this dependency by libswsscommon? If yes, create a new issue to track.

The swsssdk been removed from all these containser by this PR: https://github.com/sonic-net/sonic-buildimage/pull/11469/files

To avoid potensial risk that other script may break because python-redis missing, maybe we install python-redis to all containers?

Also it's possible replace the dependency by libswsscommon, new issue: #14634

@@ -45,6 +45,9 @@ RUN pip3 install --no-cache-dir \
pyyaml \
smbus

# Install python-redis
RUN pip3 install redis==4.5.4
Copy link
Collaborator

Choose a reason for hiding this comment

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

@liuh-80 why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The swssdk depends on python-redis.
So before we remove swssdk by this PR, python-redis also install to docker-snmp when install swsssdk: https://github.com/sonic-net/sonic-buildimage/pull/11469/files

So to avoid potensial risk that some other script also using python-redis break because swsssdk removed from this docker, maybe we install python-redis to this container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, code removed.

@@ -47,6 +47,9 @@ RUN pip3 install requests
# We install the libpci module in order to be able to do PCI transactions
RUN pip3 install libpci

# Install python-redis
RUN pip3 install redis==4.5.4
Copy link
Collaborator

Choose a reason for hiding this comment

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

@liuh-80 why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same reason as docker-snmp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, code removed.

@xumia
Copy link
Collaborator

xumia commented Apr 13, 2023

Hua, do we need to install redis to all docker config engine images, or only swss layer enough? Such as docker-swss-layer-bullseye, docker-swss-layer-buster.

@liuh-80
Copy link
Contributor Author

liuh-80 commented Apr 13, 2023

Hua, do we need to install redis to all docker config engine images, or only swss layer enough? Such as docker-swss-layer-bullseye, docker-swss-layer-buster.

Seems those docker layer is based on docker config engine images?

https://github.com/sonic-net/sonic-buildimage/blob/d57de0987a26553e94b8c31caed385e8b156c70c/dockers/docker-swss-layer-buster/Dockerfile.j2
{% from "dockers/dockerfile-macros.j2" import install_debian_packages, install_python_wheels, copy_files %}
FROM docker-config-engine-buster-{{DOCKER_USERNAME}}:{{DOCKER_USERTAG}}

The original swsssdk and python-redis been installed to docker config engine images, so there are potensial gap if we only install python-redis to these layer.

@nazariig
Copy link
Collaborator

Seems those docker layer is based on docker config engine images?

@liuh-80 that is exactly my question: why do we need to have the same package in database docker when it is already installed in config engine? What is the motivation to have it in both places?

@liuh-80
Copy link
Contributor Author

liuh-80 commented Apr 14, 2023

Seems those docker layer is based on docker config engine images?

@liuh-80 that is exactly my question: why do we need to have the same package in database docker when it is already installed in config engine? What is the motivation to have it in both places?

@nazariig, I check these docker layer file, currently the python-redis does not installed in it.

Also the docker-database is based on config engine, not using swss layer.

@@ -47,6 +47,9 @@ RUN pip3 install requests
# We install the libpci module in order to be able to do PCI transactions
RUN pip3 install libpci

# Install python-redis
RUN pip3 install redis==4.5.4
Copy link
Collaborator

@qiluo-msft qiluo-msft Apr 15, 2023

Choose a reason for hiding this comment

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

This is already in base image docker-config-engine-bullseye* #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, code removed.

@@ -45,6 +45,9 @@ RUN pip3 install --no-cache-dir \
pyyaml \
smbus

# Install python-redis
RUN pip3 install redis==4.5.4
Copy link
Collaborator

@qiluo-msft qiluo-msft Apr 15, 2023

Choose a reason for hiding this comment

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

This is already in base image docker-config-engine-bullseye* #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, code removed.

@@ -23,6 +23,9 @@ RUN apt-get install -y \
RUN pip3 install pyangbind==0.8.1
RUN pip3 uninstall -y enum34

# Install python-redis
RUN pip3 install redis==4.5.4
Copy link
Collaborator

Choose a reason for hiding this comment

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

==4.5.4

@xumia, do we need to specify the version?

Copy link
Collaborator

@xumia xumia Apr 15, 2023

Choose a reason for hiding this comment

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

==4.5.4

@xumia, do we need to specify the version?

If python3-redis installed (redis=3.5.3 currently), then need to specify the version, or use pip3 install --upgrade redis. If the version not specified, then the installation will be skipped, use 3.5.3.

If we need the version 4.5.4 exactly, then "pip3 install redis==4.5.4" is good.
If we want to use the latest version, and at least 4.5.4, use "pip3 install redis>=4.5.4", prefer to auto version upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, we need exactly 4.5.4, because swsssdk install 4.5.4:

https://github.com/sonic-net/sonic-py-swsssdk/blob/master/setup.py
dependencies = [
'redis==4.5.4;python_version >= "3.0"',
'redis>=3.5.3;python_version < "3.0"',
'redis-dump-load',
]

@qiluo-msft qiluo-msft merged commit a14cc76 into sonic-net:master Apr 20, 2023
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request Apr 20, 2023
Install python-redis package to docker containers

#### Why I did it
This this bug: sonic-net#14531
The 'flush_unused_database' is part of docker-database, and docker-database does not install python-redis package by itself. it's using redis installed by sonic-py-swsssdk.
So after remove sonic-py-swsssdk from container, this script break.

To this this bug and avoid similer bug happen again, install python-redis to docker containers which removed sonic-py-swsssdk .

#### How I did it
Install python-redis to containers.

#### How to verify it
Pass all UT.
Create new UT to cover this scenario: sonic-net/sonic-mgmt#8032

#### Description for the changelog
Improve sudo cat command for RO user.
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202211: #14775

mssonicbld pushed a commit that referenced this pull request Apr 23, 2023
Install python-redis package to docker containers

#### Why I did it
This this bug: #14531
The 'flush_unused_database' is part of docker-database, and docker-database does not install python-redis package by itself. it's using redis installed by sonic-py-swsssdk.
So after remove sonic-py-swsssdk from container, this script break.

To this this bug and avoid similer bug happen again, install python-redis to docker containers which removed sonic-py-swsssdk .

#### How I did it
Install python-redis to containers.

#### How to verify it
Pass all UT.
Create new UT to cover this scenario: sonic-net/sonic-mgmt#8032

#### Description for the changelog
Improve sudo cat command for RO user.
liuh-80 added a commit to sonic-net/sonic-mgmt that referenced this pull request Apr 26, 2023
**What I did**
Add new UT for flush_unused_database scripts.

**Why I did it**
The flush_unused_database script not cover by UT.

**How I verified it**
Pass all UT.

**Details if related**
This UT depends on PR: sonic-net/sonic-buildimage#14632
mrkcmo pushed a commit to Azarack/sonic-mgmt that referenced this pull request Oct 3, 2023
**What I did**
Add new UT for flush_unused_database scripts.

**Why I did it**
The flush_unused_database script not cover by UT.

**How I verified it**
Pass all UT.

**Details if related**
This UT depends on PR: sonic-net/sonic-buildimage#14632
AharonMalkin pushed a commit to AharonMalkin/sonic-mgmt that referenced this pull request Jan 25, 2024
**What I did**
Add new UT for flush_unused_database scripts.

**Why I did it**
The flush_unused_database script not cover by UT.

**How I verified it**
Pass all UT.

**Details if related**
This UT depends on PR: sonic-net/sonic-buildimage#14632
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.

6 participants