search

sonic-net / sonic-dbsyncd

Python library for sonic/redis database syncing
Other
6 stars 47 forks source link

[sonic-dbsyncd] Changes to convert sonic-dbsyncd from python 2 to 3 #32

Closed abdosi closed 3 years ago

abdosi commented 3 years ago

What I did:

How I verify:

Python3 wheel package getting build and pytest working

- abdosi@ec855a8c084b:/sonic/src/sonic-dbsyncd$ python3 -m pytest -v

=============================================================================================== test session starts ===============================================================================================
platform linux -- Python 3.7.3, pytest-3.10.1, py-1.7.0, pluggy-0.8.0 -- /usr/bin/python3
cachedir: .pytest_cache
rootdir: /sonic/src/sonic-dbsyncd, inifile:
plugins: cov-2.6.0
collected 9 items

tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_loc_chassis PASSED                                                                                                                                   [ 11%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_multiple_mgmt_ip PASSED                                                                                                                              [ 22%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_parse_json PASSED                                                                                                                                    [ 33%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_parse_short PASSED                                                                                                                                   [ 44%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_parse_short_short PASSED                                                                                                                             [ 55%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_remote_sys_capability_list PASSED                                                                                                                    [ 66%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_single_mgmt_ip PASSED                                                                                                                                [ 77%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_sync_roundtrip PASSED                                                                                                                                [ 88%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_timeparse PASSED                                                                                                                                     [100%]

============================================================================================ 9 passed in 0.12 seconds =============================================================================================
qiluo-msft commented 3 years ago

class SwssSyncClient(mockredis.MockRedis):

Please check https://github.com/Azure/sonic-utilities/pull/1238/files I think you need similar change to make it support decode_responses option #Closed

Refers to: tests/mock_tables/dbconnector.py:37 in 4d0963a. [](commit_id = 4d0963a670cd4e5eba44e9b08d2c490bb70eb888, deletion_comment = False)

abdosi commented 3 years ago

class SwssSyncClient(mockredis.MockRedis):

Please check https://github.com/Azure/sonic-utilities/pull/1238/files I think you need similar change to make it support decode_responses option

Refers to: tests/mock_tables/dbconnector.py:37 in 4d0963a. [](commit_id = 4d0963a, deletion_comment = False)

@qiluo-msft Updated. Modified function to do first _encode (by calling base class function) and if dcode_response == True then decode it back. Works for both python2 and python3.

abdosi commented 3 years ago

Need to modify setup.py to acknowledge Python 3 support: https://github.com/Azure/sonic-dbsyncd/blob/master/setup.py#L38. LGTM tool also uses this to determine what version of Python to check code with.

Also, do we need to continue supporting Python 2 with this package? If not we should remove Python 2 from that list.

Also, please ensure the dependencies in setup.py are correct and complete. One thing that stands out to me is 'enum34>=1.1.6' here. This package should only be installed for Python 2, if installed for Python 3, it will break the enum module in the standard library. If we need to continue supporting Python 2, you can do something like I did here.

@jleveque Updated setup.py. Also I have updated jekin pr build of this repo to to just compile/build/install/test python3 wheel. As part of submodule-update PR for sonic-buildimage will update the Makefile and make lldp docker to start using this python3 package and other corresponding changes.

Jenkin Config Change for PR Job: image

abdosi commented 3 years ago

retest this please

qiluo-msft commented 3 years ago

Remove extra line? no need to change this file.

@qiluo-msft Done #Closed

Refers to: tests/mock_tables/LLDP_ENTRY_TABLE.json:474 in 67d4206. [](commit_id = 67d42068d00da93cd56a515115a35e23b2c19bbf, deletion_comment = False)

abdosi commented 3 years ago

@jleveque /@qiluo-msft Any idea on this failure ? Do we need to update something on jenkins. It is working fine in local enviroment.

https://sonic-jenkins.westus2.cloudapp.azure.com/job/common/job/sonic-dbsyncd-build-pr/50/console

abdosi commented 3 years ago

retest this please

abdosi commented 3 years ago

retest this please

abdosi commented 3 years ago

@jleveque and @qiluo-msft. Can you please review the PR. All the comments are addressed.

Jenkins job was failing when I reverted Jenkin to not to do pip-install for dependency package and rely on setup.py. That looks to be Jenkin machine issue. Eg:(https://sonic-jenkins.westus2.cloudapp.azure.com/job/common/job/sonic-dbsyncd-build-pr/50/console)

As of now Jenkin job is same as previous and move all commands from python to python3. python2 package is removed and we are building python3 package.

Rest of changes to start using python3 package will come as part of submodule update PR.

abdosi commented 3 years ago

retest this please

qiluo-msft commented 3 years ago

@abdosi will you remove https://github.com/Azure/sonic-buildimage/blob/master/rules/dbsyncd-py2.mk ?

yes @qiluo-msft