Reduce usage of module level globals #424

jhesketh · 2021-04-14T13:59:29Z

This refactor instantiates Tickers (ie long running sub processes that
persist across requests+sessions) as part of the app. It stores them
inside FastAPI's app.state, which in turn is available on FastAPI's
Request object.

I could not find the most "FastAPI way" of doing this. From my reading, the way we were handling state with module level globals is generally the approach that FastAPI takes. This has some difficult draw backs with mocking and testing, not to mention potential complications on instantiating multiple redundant (or even conflicting) instances, and the general developer confusion about what they are accessing. There is some good discussion on it here: tiangolo/fastapi#81

The solution, as describe in the commit above, is to create all the Tickers ourselves and register them in app.state. This will make it trivial to swap them out for fake Tickers in our testing as we can build different app's.

Follow up:

Write example functional tests (will be much easier with this refactor).

Closes: #276
Closes: #282

jecluis

LGTM but mypy is failing badly. I suspect is partially because of comment below, and because of a lot of calls are made using app.state.gstate, which will likely confuse the static checker because, I suspect, the type of app.state is not known.

src/gravel/controllers/gstate.py

jecluis

Also, commit is missing the DCO.

jhesketh · 2021-04-21T07:33:59Z

FYI I haven't done a deployment to verify everything still works since rebasing this.

kshtsk · 2021-04-21T11:59:22Z

jenkins run tumbleweed

kshtsk · 2021-04-21T11:59:32Z

jenkins run leap

kshtsk · 2021-04-21T14:47:29Z

I guess you saw these multiple traces:

>>> -- Logs begin at Wed 2021-04-21 12:20:10 UTC, end at Wed 2021-04-21 12:22:32 UTC. --
>>> Apr 21 12:20:46 localhost systemd[1]: Started Project Aquarium.
>>> Apr 21 12:20:49 localhost uvicorn[1317]: INFO:     Started server process [1317]
>>> Apr 21 12:20:49 localhost uvicorn[1317]: INFO:     Waiting for application startup.
>>> Apr 21 12:20:49 localhost uvicorn[1317]: INFO:     2021-04-21 12:20:49 -- aquarium -- Aquarium startup!
>>> Apr 21 12:20:49 localhost uvicorn[1317]: INFO:     2021-04-21 12:20:49 -- aquarium -- starting node manager
>>> Apr 21 12:20:49 localhost uvicorn[1317]: ERROR:    2021-04-21 12:20:49 -- on -- Traceback (most recent call last):
>>> Apr 21 12:20:49 localhost uvicorn[1317]:   File "/usr/local/lib/python3.8/site-packages/starlette/datastructures.py", line 669, in __getattr__
>>> Apr 21 12:20:49 localhost uvicorn[1317]:     return self._state[key]
>>> Apr 21 12:20:49 localhost uvicorn[1317]: KeyError: 'gstate'
>>> Apr 21 12:20:49 localhost uvicorn[1317]: During handling of the above exception, another exception occurred:
>>> Apr 21 12:20:49 localhost uvicorn[1317]: Traceback (most recent call last):
>>> Apr 21 12:20:49 localhost uvicorn[1317]:   File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 526, in lifespan
>>> Apr 21 12:20:49 localhost uvicorn[1317]:     async for item in self.lifespan_context(app):
>>> Apr 21 12:20:49 localhost uvicorn[1317]:   File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 467, in default_lifespan
>>> Apr 21 12:20:49 localhost uvicorn[1317]:     await self.startup()
>>> Apr 21 12:20:49 localhost uvicorn[1317]:   File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 502, in startup
>>> Apr 21 12:20:49 localhost uvicorn[1317]:     await handler()
>>> Apr 21 12:20:49 localhost uvicorn[1317]:   File "./aquarium.py", line 99, in on_startup
>>> Apr 21 12:20:49 localhost uvicorn[1317]:     nodemgr: NodeMgr = NodeMgr(app.state.gstate)
>>> Apr 21 12:20:49 localhost uvicorn[1317]:   File "/usr/local/lib/python3.8/site-packages/starlette/datastructures.py", line 672, in __getattr__
>>> Apr 21 12:20:49 localhost uvicorn[1317]:     raise AttributeError(message.format(self.__class__.__name__, key))
>>> Apr 21 12:20:49 localhost uvicorn[1317]: AttributeError: 'State' object has no attribute 'gstate'
>>> Apr 21 12:20:49 localhost uvicorn[1317]: ERROR:    2021-04-21 12:20:49 -- on -- Traceback (most recent call last):
>>> Apr 21 12:20:49 localhost uvicorn[1317]:   File "/usr/local/lib/python3.8/site-packages/starlette/datastructures.py", line 669, in __getattr__
>>> Apr 21 12:20:49 localhost uvicorn[1317]:     return self._state[key]
>>> Apr 21 12:20:49 localhost uvicorn[1317]: KeyError: 'gstate'
>>> Apr 21 12:20:49 localhost uvicorn[1317]: During handling of the above exception, another exception occurred:
>>> Apr 21 12:20:49 localhost uvicorn[1317]: Traceback (most recent call last):
>>> Apr 21 12:20:49 localhost uvicorn[1317]:   File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 526, in lifespan
>>> Apr 21 12:20:49 localhost uvicorn[1317]:     async for item in self.lifespan_context(app):
>>> Apr 21 12:20:49 localhost uvicorn[1317]:   File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 467, in default_lifespan
>>> Apr 21 12:20:49 localhost uvicorn[1317]:     await self.startup()
>>> Apr 21 12:20:49 localhost uvicorn[1317]:   File "/usr/local/lib/python3.8/site-packages/starlette/routing.py", line 502, in startup
>>> Apr 21 12:20:49 localhost uvicorn[1317]:     await handler()
>>> Apr 21 12:20:49 localhost uvicorn[1317]:   File "./aquarium.py", line 99, in on_startup
>>> Apr 21 12:20:49 localhost uvicorn[1317]:     nodemgr: NodeMgr = NodeMgr(app.state.gstate)
>>> Apr 21 12:20:49 localhost uvicorn[1317]:   File "/usr/local/lib/python3.8/site-packages/starlette/datastructures.py", line 672, in __getattr__
>>> Apr 21 12:20:49 localhost uvicorn[1317]:     raise AttributeError(message.format(self.__class__.__name__, key))
>>> Apr 21 12:20:49 localhost uvicorn[1317]: AttributeError: 'State' object has no attribute 'gstate'
>>> Apr 21 12:20:49 localhost uvicorn[1317]: ERROR:    2021-04-21 12:20:49 -- on -- Application startup failed. Exiting.
>>> Apr 21 12:20:49 localhost uvicorn[1317]: ERROR:    2021-04-21 12:20:49 -- on -- Application startup failed. Exiting.

jecluis

It looks really great, but there's a few things that are mostly nits, a few other things we can change in a follow-up PR, and one thing I'm pretty sure it's a hard bug.

src/aquarium.py

src/gravel/controllers/gstate.py

src/gravel/controllers/resources/storage.py

src/gravel/tests/conftest.py

src/gravel/tests/unit/controllers/test_services.py

This refactor instantiates Tickers (ie long running sub processes that persist across requests+sessions) as part of the app. It stores them inside FastAPI's `app.state`, which in turn is available on FastAPI's `Request` object. Signed-off-by: Joshua Hesketh <josh@nitrotech.org>

This change also reduces usage of mocks which can cause issues between isolated tests. Signed-off-by: Joshua Hesketh <josh@nitrotech.org>

Signed-off-by: Joshua Hesketh <josh@nitrotech.org>

Signed-off-by: Joao Eduardo Luis <joao@suse.com>

jecluis

pushed a patch that fixes unresolved issues.

jecluis · 2021-04-25T11:54:50Z

jenkins run tumbleweed

jhesketh added the WIP Work in progress - Do not merge label Apr 14, 2021

jhesketh requested a review from jecluis April 14, 2021 13:59

github-actions bot added the gravel Related to the Aquarium Backend label Apr 14, 2021

jecluis suggested changes Apr 16, 2021

View reviewed changes

src/gravel/controllers/gstate.py Outdated Show resolved Hide resolved

jecluis suggested changes Apr 16, 2021

View reviewed changes

jhesketh force-pushed the refactor branch from 7c92ac8 to bdc474b Compare April 21, 2021 07:26

jhesketh removed the WIP Work in progress - Do not merge label Apr 21, 2021

jhesketh force-pushed the refactor branch from bdc474b to 4a9b979 Compare April 22, 2021 05:04

jecluis suggested changes Apr 22, 2021

View reviewed changes

jhesketh added 3 commits April 23, 2021 15:09

Re-enable test_services

e38743c

This change also reduces usage of mocks which can cause issues between isolated tests. Signed-off-by: Joshua Hesketh <josh@nitrotech.org>

Fix copyright headers in test files

fe8a7e2

Signed-off-by: Joshua Hesketh <josh@nitrotech.org>

jhesketh force-pushed the refactor branch from e2a2380 to fe8a7e2 Compare April 23, 2021 05:14

Merge fe8a7e2 into 304e8c4

e285a6b

jecluis previously approved these changes Apr 23, 2021

View reviewed changes

jhesketh added the WIP Work in progress - Do not merge label Apr 23, 2021

gravel: store state in api fastapi app instance

e68e299

Signed-off-by: Joao Eduardo Luis <joao@suse.com>

jecluis dismissed their stale review via e68e299 April 25, 2021 07:27

jecluis self-requested a review April 25, 2021 07:27

jecluis approved these changes Apr 25, 2021

View reviewed changes

jecluis merged commit b40d0bf into aquarist-labs:main Apr 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce usage of module level globals #424

Reduce usage of module level globals #424

jhesketh commented Apr 14, 2021 •

edited

jecluis left a comment

jecluis left a comment

jhesketh commented Apr 21, 2021

kshtsk commented Apr 21, 2021

kshtsk commented Apr 21, 2021

kshtsk commented Apr 21, 2021

jecluis left a comment

jecluis left a comment

jecluis commented Apr 25, 2021

Reduce usage of module level globals #424

Reduce usage of module level globals #424

Conversation

jhesketh commented Apr 14, 2021 • edited

jecluis left a comment

Choose a reason for hiding this comment

jecluis left a comment

Choose a reason for hiding this comment

jhesketh commented Apr 21, 2021

kshtsk commented Apr 21, 2021

kshtsk commented Apr 21, 2021

kshtsk commented Apr 21, 2021

jecluis left a comment

Choose a reason for hiding this comment

jecluis left a comment

Choose a reason for hiding this comment

jecluis commented Apr 25, 2021

jhesketh commented Apr 14, 2021 •

edited