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

[Feature][Fix] Upgrade dependencies #284

Merged
merged 21 commits into from
Sep 20, 2022
Merged

Conversation

viniarck
Copy link
Member

@viniarck viniarck commented Sep 8, 2022

Fixes #155, #158, #159, #282

  • The follow libraries got major recent upgrades:
ipython==8.1.1
flask-socketio==5.2.0
flask_cors==3.0.10
flask[async]==2.1.3
janus==1.0.0
jinja2==3.1.2
watchdog==2.1.9
pyjwt==2.4.0
pylint==2.15.0

Changes Summary

  • Flask/Werkzeug 2.0.0+ now provide async support, so NApps can leverage asyncio and its ecosystem when applicable using the same rest decorator. I'll give an example below.
  • Replaced get_event_loop with get_running_loop when applicable to be compatible with python 3.9+ in the future.
  • ipython console now provides command completion with ctrl + f, no changes in storing command history though.
  • I had to upgrade pylint since a very old one was being used causing conflicts on development dependencies, as a result the linter is picking up more errors, so I had to fix several of tiny linter issues. I didn't want to touch this many files in this same PR, but it had to be done.
  • Thanks to the upgrades security/CVEs upstream patches are now all set.
  • Log traceback error if NApps execute method doesn't handle an exception
  • Stop APIServer instance after unloading NApps

Flask async route example

  • This is an v1/request endpoint, notice the only difference is the async keyword before the method definition. This method will run in the event loop, still backed by a thread. In this case, it's performing two concurrent requests and returning an empty response. You can leverage asyncio when it's suitable to do so:
    @rest("v1/request")
    async def request(self):
        async with httpx.AsyncClient() as client:
            requests = [client.get("https://www.amlight.net"),
                        client.get("https://www.google.com")]
            await asyncio.gather(*requests)
        return jsonify()

flask_async_route

Request stress test

New endpoint

I've added a temporarily v1/compute endpoint in the sample_ui NApp, it handled 100+ request/sec over 30 secs successfully:

    @rest("v1/compute")
    async def compute(self):
        await asyncio.sleep(1)
        return jsonify()
❯ jq -ncM '{method: "GET", url: "http://localhost:8181/api/kytos/sample_ui/v1/compute"}' | vegeta attack -format=json -rate 120/1s -duration=30s | tee results.bin | vegeta report
Requests      [total, rate, throughput]         3600, 120.03, 115.97
Duration      [total, attack, wait]             31.043s, 29.992s, 1.052s
Latencies     [min, mean, 50, 90, 95, 99, max]  1.002s, 1.061s, 1.026s, 1.161s, 1.271s, 1.496s, 1.653s
Bytes In      [total, mean]                     10800, 3.00
Bytes Out     [total, mean]                     0, 0.00
Success       [ratio]                           100.00%
Status Codes  [code:count]                      200:3600
Error Set:

Existing sync endpoints

I've also repeated the same tests that were used on #225, and the existing instability still exist when a significant rate 500+ req /sec over 30 sec happens which is expected since there were no changes in this endpoints, so confirming that no regression happened:

❯ jq -ncM '{method: "GET", url: "http://localhost:8181/api/kytos/topology/v3/"}' | vegeta attack -format=json -rate 500/1s -duration=30s | tee results.bin | vegeta report
Requests      [total, rate, throughput]         15000, 500.03, 330.79
Duration      [total, attack, wait]             44.318s, 29.998s, 14.32s
Latencies     [min, mean, 50, 90, 95, 99, max]  1.329ms, 2.714s, 321.884ms, 7.434s, 19.543s, 30.001s, 30.001s
Bytes In      [total, mean]                     90554820, 6036.99
Bytes Out     [total, mean]                     0, 0.00
Success       [ratio]                           97.73%
Status Codes  [code:count]                      0:340  200:14660
Error Set:
Get "http://localhost:8181/api/kytos/topology/v3/": read tcp 127.0.0.1:56811->127.0.0.1:8181: read: connection reset by peer
Get "http://localhost:8181/api/kytos/topology/v3/": read tcp 127.0.0.1:44915->127.0.0.1:8181: read: connection reset by peer
Get "http://localhost:8181/api/kytos/topology/v3/": read tcp 127.0.0.1:49101->127.0.0.1:8181: read: connection reset by peer
Get "http://localhost:8181/api/kytos/topology/v3/": read tcp 127.0.0.1:50877->127.0.0.1:8181: read: connection reset by peer
Get "http://localhost:8181/api/kytos/topology/v3/": read tcp 127.0.0.1:45463->127.0.0.1:8181: read: connection reset by peer
❯ jq -ncM '{method: "POST", url: "http://localhost:8181/api/kytos/flow_manager/v2/flows/00:00:00:00:00:00:00:01", body: { "force": true, "flows": [ { "priority": 10, "match": { "in_port": 1, "dl_vlan": 100 }, "actions": [ { "action_type": "output", "port": 1 } ] } ] } | @base64, header: {"Content-Type": ["application/json"]}}' | vegeta attack -format=json -rate 120/1s - duration=10s | tee results.bin | vegeta report
Requests      [total, rate, throughput]         1200, 120.10, 118.50
Duration      [total, attack, wait]             10.127s, 9.992s, 134.909ms
Latencies     [min, mean, 50, 90, 95, 99, max]  6.961ms, 92.855ms, 42.635ms, 271.165ms, 401.01ms, 493.645ms, 523.631ms
Bytes In      [total, mean]                     44400, 37.00
Bytes Out     [total, mean]                     146400, 122.00
Success       [ratio]                           100.00%
Status Codes  [code:count]                      202:1200
Error Set:

@viniarck viniarck requested a review from a team September 8, 2022 21:06
@viniarck
Copy link
Member Author

viniarck commented Sep 8, 2022

I'll leave this PR in draft until Kytos-ng NApps also regenerate their requirement/run.txt dependencies, otherwise it'll break pip dendencies resolution in the docker image, so this PR needs to be strategically merged only when the NApps are ready, I'll keep track of them with this list, I'll also run e2e tests with these branches when they're ready:

  • of_core
  • flow_manager
  • topology
  • pathfinder
  • mef_eline
  • maintenance
  • coloring
  • sdntrace
  • sdntrace_cp
  • flow_stats
  • noviflow
  • of_lldp

@viniarck viniarck marked this pull request as draft September 8, 2022 21:09
@viniarck
Copy link
Member Author

The docker image has been built successfully using the branches that upgraded the dependencies, e2e tests are passing as well, I'll go ahead and mark this PR as ready for review:

+ python3 -m pytest tests/ -k topology
============================= test session starts ==============================
platform linux -- Python 3.9.2, pytest-6.2.5, py-1.11.0, pluggy-1.0.0
rootdir: /tests
plugins: timeout-2.0.2
collected 199 items / 181 deselected / 18 selected

tests/test_e2e_05_topology.py ..................                         [100%]

=============================== warnings summary ===============================
test_e2e_05_topology.py: 17 warnings
  /usr/lib/python3/dist-packages/mininet/node.py:1121: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
    return ( StrictVersion( cls.OVSVersion ) <

test_e2e_05_topology.py: 17 warnings
  /usr/lib/python3/dist-packages/mininet/node.py:1122: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
    StrictVersion( '1.10' ) )

-- Docs: https://docs.pytest.org/en/stable/warnings.html
------------------------------- start/stop times -------------------------------
========= 18 passed, 181 deselected, 34 warnings in 1268.63s (0:21:08) =========

@viniarck viniarck marked this pull request as ready for review September 14, 2022 14:42
Shifted loop.create_task of consumers after loading NApps
Shifted loop.create_task of consumers after loading NApps
@viniarck
Copy link
Member Author

I'll go ahead and merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade requirements/run.txt dependencies for the upcoming release
1 participant