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

chore: Upgraded python and dependencies #460

Merged
merged 24 commits into from
Mar 28, 2024
Merged

chore: Upgraded python and dependencies #460

merged 24 commits into from
Mar 28, 2024

Conversation

Alopalao
Copy link

@Alopalao Alopalao commented Feb 26, 2024

Closes #456
Closes #448
Closes #420
Closes #350
Closes #313

Summary

Updated python3.11
Created this PR to see remote tests results.
This PR only works with 3.11 because ipython==8.22.1 requirements
Updated python dependencies. Major ones would be openapi-core, tox, pydantic, pytest, pymongo, ipython, tenacity and more
Listing here the related PRs:

Local Tests

Created venv with python 3.11.2
Run tox, kytosd

End-to-End Tests

PR

============================= test session starts ==============================
platform linux -- Python 3.11.2, pytest-7.2.1, pluggy-1.4.0
rootdir: /tests
plugins: rerunfailures-13.0, timeout-2.2.0, anyio-4.3.0
collected 261 items

tests/test_e2e_01_kytos_startup.py ..                                    [  0%]
tests/test_e2e_05_topology.py ....................                       [  8%]
tests/test_e2e_10_mef_eline.py ..........ss.....x.....x................  [ 23%]
tests/test_e2e_11_mef_eline.py ......                                    [ 26%]
tests/test_e2e_12_mef_eline.py .....Xx.                                  [ 29%]
tests/test_e2e_13_mef_eline.py ....Xs.s.....Xs.s.XXxX.xxxx..X........... [ 44%]
.                                                                        [ 45%]
tests/test_e2e_14_mef_eline.py x                                         [ 45%]
tests/test_e2e_15_mef_eline.py .....                                     [ 47%]
tests/test_e2e_16_mef_eline.py .                                         [ 47%]
tests/test_e2e_20_flow_manager.py .....................                  [ 55%]
tests/test_e2e_21_flow_manager.py ...                                    [ 57%]
tests/test_e2e_22_flow_manager.py ...............                        [ 62%]
tests/test_e2e_23_flow_manager.py ..............                         [ 68%]
tests/test_e2e_30_of_lldp.py ....                                        [ 69%]
tests/test_e2e_31_of_lldp.py ...                                         [ 70%]
tests/test_e2e_32_of_lldp.py ...                                         [ 72%]
tests/test_e2e_40_sdntrace.py ..............                             [ 77%]
tests/test_e2e_41_kytos_auth.py ........                                 [ 80%]
tests/test_e2e_42_sdntrace.py ..                                         [ 81%]
tests/test_e2e_50_maintenance.py ............................            [ 91%]
tests/test_e2e_60_of_multi_table.py .....                                [ 93%]
tests/test_e2e_70_kytos_stats.py ........                                [ 96%]
tests/test_e2e_80_pathfinder.py ss......                                 [100%]

@Alopalao Alopalao requested a review from a team as a code owner February 26, 2024 21:58
@Alopalao
Copy link
Author

Scrutinizer tests were passing with these changes. Changing this PR to draft.

@Alopalao Alopalao marked this pull request as draft February 27, 2024 01:02
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

@Alopalao nicely done how this is shaping up. Looking great.

Let's make sure this gets shipped with amlight/kytos-docker#34, just so e2e tests are passing. Regarding which debian version should be used on Dockerfile I'll recommend syncing with @italovalcy, his last suggestion on Slack was Debian stable aka "bookworm" which already ships with 3.11.X

requirements/run.in Show resolved Hide resolved
requirements/run.in Show resolved Hide resolved
requirements/run.in Show resolved Hide resolved
requirements/run.txt Show resolved Hide resolved
@Alopalao
Copy link
Author

Alopalao commented Mar 3, 2024

Detected one error from elastic-apm. Everythin works, this issue only appears when closing kytos with the following message:

2024-03-03 03:51:55,807 - ERROR [elasticapm.transport] (MainThread) Closing the transport connection timed out.

And it is inconsistent.

Detected an error with starlette, every request is disconnected causing an error when getting request.body(). Looking into it.
UPDATE: I was getting this error by saving the request in memory which was my old method to test it. API requests are working normally now.

@Alopalao
Copy link
Author

Alopalao commented Mar 4, 2024

Updated pydantic in commit dbc8b29cca3a999420ab72ef262449bd64d565c2

@Alopalao
Copy link
Author

Alopalao commented Mar 5, 2024

Updated testing libraries, commit 5eda4340f81187b92751e52889780d21c133db73.

Copy link
Author

@Alopalao Alopalao left a comment

Choose a reason for hiding this comment

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

Finished updating dependencies. I think every NApp needs to be updated because of some breaking changes. The culprit dependencies are pydantic, openapi-core, elastic-apm and asyncio. I am going to be submitting more PRs for the rest of the NApps. All of them are in the same branch upgrade/python

kytos/core/controller.py Show resolved Hide resolved
kytos/core/connection.py Outdated Show resolved Hide resolved
kytos/core/kytosd.py Show resolved Hide resolved
return (
self.request.headers.get("Content-Type")
or "application/octet-stream"
)
Copy link
Author

@Alopalao Alopalao Mar 14, 2024

Choose a reason for hiding this comment

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

openapi-core==0.19.0 has some issues including:

  • Not differentiating between numeric string and integers
  • Broken spec validation can cause to accept any request

From my tests openapi-core==0.18.2 is more stable but this property which causes some errors. Brought this from current request implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Good to know. Let's keep track of this too. If you could both:

  • Create an internal issue for us, just so in a next upgrade this is a pre-requisite to be solved.
  • Create also an issue upstream since they're breaking a basic property of OpenAPI validation.

Copy link
Author

Choose a reason for hiding this comment

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

Created this issue. What does issue upstream mean?

Copy link
Member

Choose a reason for hiding this comment

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

Cool. @Alopalao, by upstream I mean the openapi-core` upstream repo. Since we depend on it, let's also file an issue there reporting the issue, or if it's already been reported just link with the issue you opened here and see if they'll fix.

requirements/run.in Show resolved Hide resolved
kytos/core/user.py Show resolved Hide resolved
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

Nicely done @Alopalao, awesome to have py 3.11 in our core projects. Also, to have most of the dependencies upgraded too. This PR looks good to me, almost ready, just left some minor details to be sorted out though.

Also, as we spoke, I'll recommend that you rerun a stress tests with flow mods to ensure the transport core part is all set (it should be no surprises since e2e tests are passing, but let's do also some stress testing to be safe). It'd be great to also simulate some connections getting closes in a high freq you can use OvS sudo ovs-vsctl set-controller s1 tcp:127.0.0.1:6654 (to change the port to also simulate the peer forcing a connection getting closed), and then set sudo ovs-vsctl set-controller s1 tcp:127.0.0.1:6653 again.

kytos/core/connection.py Show resolved Hide resolved
kytos/core/controller.py Show resolved Hide resolved
kytos/core/connection.py Show resolved Hide resolved
requirements/run.in Show resolved Hide resolved
return (
self.request.headers.get("Content-Type")
or "application/octet-stream"
)
Copy link
Member

Choose a reason for hiding this comment

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

Good to know. Let's keep track of this too. If you could both:

  • Create an internal issue for us, just so in a next upgrade this is a pre-requisite to be solved.
  • Create also an issue upstream since they're breaking a basic property of OpenAPI validation.

tests/conftest.py Outdated Show resolved Hide resolved
"""Assign parameters to instance variables.

Args:
address (|hw_address|): Source address.
port (int): Port number.
socket (socket): socket.
socket (TransportSocket): socket.
Copy link
Member

Choose a reason for hiding this comment

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

socket docstring become obsolete, feel free to nuke it

Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

It's almost there @Alopalao

  • Requested changes on openapi validation (check out the thread)
  • Also, when trying to create an EVC on mef_eline it resulted in 405
❯ curl -H 'Content-type: application/json' -X POST http://127.0.0.1:8181/api/kytos/mef_eline/v2/evc/ -d '{"name": "inter_evpl", "dynamic_backup_path": true, "uni_a": {"interface_id": "00
:00:00:00:00:00:00:01:1", "tag": {"tag_type": "vlan", "value": 2222}}, "uni_z": { "interface_id": "00:00:00:00:00:00:00:03:1", "tag": {"tag_type": "vlan", "value": 2222}}}'
{"description":"Method Not Allowed","code":405}

So either validations aren't working correctly or there has been a regression, but you mentioned that it worked on your e2e PR, can you double check this?

Other than that we also need to sort out the following:

  • Let me know if stress tests with flow mods have been exercised

  • Once PRs are merged I'll update Scrutinizer CI global config to start using python 3.11 from this point forward

  • update documetation on how to install py 3.11 and which Ubuntu or Debian version

  • of_core PR trivial detail

  • kytos docker try to upgrade pytest to 8+ too

kytos/core/helpers.py Show resolved Hide resolved
@viniarck viniarck self-requested a review March 26, 2024 12:56
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

@Alopalao mef_eline's POST /v2/evc/ is still returning 405 method not allowed.

❯ echo '{ "name": "epl", "service_level": 7, "dynamic_backup_path": true, "uni_a": { "interface_id": "00:00:00:00:00:00:00:01:1" }, "uni_z": { "interface_id": "00:00:00:00:00:00:00:03:1"
 } }' | http http://127.0.0.1:8181/api/kytos/mef_eline/v2/evc/
HTTP/1.1 405 Method Not Allowed
content-length: 47
content-type: application/json
date: Tue, 26 Mar 2024 12:55:14 GMT
server: uvicorn

{
    "code": 405,
    "description": "Method Not Allowed"
}

That's unexpected considering you said e2e were passing, can you double check this? Once threads have been addressed, e2e tests passing, flows stress tested with the latest commits, let me known and I'll do a final review. So, I'll request changes again here to make it explicit.

@viniarck
Copy link
Member

@Alopalao mef_eline's POST /v2/evc/ is still returning 405 method not allowed.

❯ echo '{ "name": "epl", "service_level": 7, "dynamic_backup_path": true, "uni_a": { "interface_id": "00:00:00:00:00:00:00:01:1" }, "uni_z": { "interface_id": "00:00:00:00:00:00:00:03:1"
 } }' | http http://127.0.0.1:8181/api/kytos/mef_eline/v2/evc/
HTTP/1.1 405 Method Not Allowed
content-length: 47
content-type: application/json
date: Tue, 26 Mar 2024 12:55:14 GMT
server: uvicorn

{
    "code": 405,
    "description": "Method Not Allowed"
}

That's unexpected considering you said e2e were passing, can you double check this? Once threads have been addressed, e2e tests passing, flows stress tested with the latest commits, let me known and I'll do a final review. So, I'll request changes again here to make it explicit.

It turns out this was a result of wrong installation of conflict dependencies with openapi-core. pythons setup.py develop is not longer recommended due to upstream changes, for more information check out Aldo's PR updating the docs

@viniarck viniarck self-requested a review March 26, 2024 18:16
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

It's pretty close to land, as we spoke, once these are addressed let me know and we'll ship it:

  • Try to fix the egg=kytos[dev] with a new syntax on NApps, since this will be fully deprecated in a next pip version:
DEPRECATION: git+https://github.com/kytos-ng/kytos.git@upgrade/python#egg=kytos[dev] contains an egg fragment with a non-PEP 508 name pip 25.0 will enforce this behaviour change. A possi
ble replacement is to use the req @ url syntax, and remove the egg fragment. Discussion can be found at https://github.com/pypa/pip/issues/11617```
  • Kytos-ng/kytos readme installing also needs to be updated to py3.11, in this readme it make sense to keep it.

  • I'm also seeing this DeprecationWarning when starting kytosd, probably not a big deal but let's see if you're also seeing it, and let's see if it might be easy to fix:

/home/viniarck/repos/kytos/.direnv/python-3.11/bin/kytosd:4: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
  __import__('pkg_resources').require('kytos==2023.2.0')```

Thanks, Aldo. Nicely done. Lotta of work to have this upgrade, glad to see the final result almost here. Feel free to mark the check boxes when it's done or use whatever you think it's easier and post a comment here and this will get shipped, and let's also close the open threads on this PR.

requirements/dev.txt Outdated Show resolved Hide resolved
@Alopalao
Copy link
Author

Created issue for egg fragments warnings.

@viniarck viniarck self-requested a review March 28, 2024 14:46
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

Nicely done @Alopalao. Excellent PR upgrading to Python 3.11 and the core dependencies, glad to see that we successfully manage to update most of them. Let's ship it? I'll start by merging this one and then updating scrutinizer ci global config just so NApps PRs are expected to pass too.

kytos/core/connection.py Outdated Show resolved Hide resolved
Co-authored-by: Vinicius Arcanjo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants