Network / Contracts models + CRUD endpoints + Celery Orchestration #44

frisitano · 2021-03-01T22:38:12Z

This PR introduces database tables for both chains and contracts and associated CRUD endpoints to interact with these tables. This will allow the user to activate / deactivate chains and contracts, and also allows them to add / delete contracts.

When a chain / contract is activated / deactivated / create / deleted, all celery collector tasks are terminated and new collector tasks are executed to reflect the updated chain statuses.

closes #21
supports paralink-network/paralink-ui#10

… orchestration

jbargu

A couple of questions:

Would it be possible to terminate only the workers that have changed in the request?
I would at least test the client <-> DB interaction in this PR.
Have you thought about how could we perform integration tests with celery? It does not need to be in this PR but it should be somehow included as it is the critical functionality of the node.

jbargu · 2021-03-02T11:40:54Z

src/__init__.py

@@ -36,7 +40,9 @@ def create_app(args: dict = {}) -> Sanic:  # noqa: C901
    if app.config["ENABLE_BACKGROUND_WORKER"]:
        from src.process.collector import start_collecting

-        asyncio.get_event_loop().run_until_complete(start_collecting(chains))
+        @app.listener("after_server_start")


Could we move this below to have all the hooks on the same place?

jbargu · 2021-03-02T11:54:12Z

src/api/chains.py

+    except ActiveChainFailed as e:
+        raise NotFound(e)
+
+    await restart_collectors(processor, chains, request.app.db)


If I tried curl -X PUT -H "Content-Type: application/json" -d '{"active":true}' localhost:7424/api/chains/eth.mainnet without the celery worker running this part would fail but the chain status would be updated. Also any other problems would result in the same behaviour. We should either wrap the DB update into transaction or return appropriate message if the collector restart failed.

My proposal would be to check if the celery worker is enabled app.config["ENABLE_DATABASE"] == True. If the celery worker is enabled then we would restart the appropriate celery chain collector. If not then we would do nothing. This will allow the user to configure the desired chain / contract statuses whilst the celery worker is disabled.

jbargu · 2021-03-02T12:01:19Z

src/process/__init__.py

@@ -15,8 +15,7 @@
        "src.process.collector.*": {"queue": "collect"},
        "src.process.executor.*": {"queue": "execute"},
    },
-    task_serializer="pickle",
-    accept_content=["json", "pickle"],
+    accept_content=["json"],


jbargu · 2021-03-02T12:13:04Z

src/api/contracts.py

+    """
+    data = request.json
+    await Contract.create_contract(
+        request.app.db, data["id"], data["active"], data["chain"]


Data should be validated:

is the given address of a correct format depending on the chain?

jbargu · 2021-03-02T12:15:42Z

src/models/contract.py

+
+    __tablename__ = "contracts"
+
+    id = sa.Column(sa.String, primary_key=True)


Contract model requires an address field. With this setup now we cannot have 2 contracts with the same address on 2 different chains (e.g. ETH, BSC). A contract can therefore be uniquely identifiable by a (address, chain) tuple.

I've added the address field. I have not had time to change the interfaces to use a (address, chain) as a primary key for the contracts. They are currently using the id field which is an auto-incrementing integer. I can change this tomorrow.

frisitano · 2021-03-04T21:01:33Z

A couple of questions:

Would it be possible to terminate only the workers that have changed in the request?

I would at least test the client <-> DB interaction in this PR.

Have you thought about how could we perform integration tests with celery? It does not need to be in this PR but it should be somehow included as it is the critical functionality of the node.

Yes it will be possible to terminate only the worker that has changed, I'll update the PR to do this.
Good point, I will get some tests written.
Yes we could run our tests using the docker-compose stack. We could probably reuse the paralink_node docker image and just execute a test command (you could also execute examples in a similar. Viable solution for Missing integration tests #41? ):
command: /wait-for-it.sh -t 0 rabbitmq:5672 -- pipenv run pytest
We would need to take care and run tests synchronously when testing the dynamics of the tasks on the workers (restarting chains)

jbargu

This looks great, thank you. I made a couple of tests locally, it seems to work alright. I had hoped you will include the integration tests: client -> API endpoints -> DB as this is what we care from the FE side but I guess this is fine.

jbargu · 2021-03-16T07:14:40Z

Dockerfile

-RUN mkdir /home/worker/paralink-node
-RUN mkdir /home/worker/paralink-node/data
+RUN mkdir -p /home/worker/paralink-node
+RUN mkdir -p /home/worker/paralink-node/data


I hope this does not exposes the system for any vulnerabilities.

I will resolve this shortly. I was thinking we can provide a build flag to indicate if the root user should be used. We can then toggle root on for github builds and turn it off for operation. What do you think?

jbargu · 2021-03-16T07:38:33Z

src/network/__init__.py

@@ -5,4 +5,6 @@
 from src.network.chains import Chains

 chain_config = Path(config.DATA_FOLDER).joinpath("chain_config.json")
-chains = Chains.read_from_json(str(chain_config.absolute()))
+chains = Chains.from_list(
+    json.load(open(Path(config.DATA_FOLDER).joinpath("chain_config.json")))


Any reason why this cannot use the above chain_config variable?

No reason... just me being blind 😄

jbargu · 2021-03-16T08:08:05Z

tests/conftest.py

+@pytest.fixture()
+async def tables(engine):
+    async with engine.begin() as conn:
+        await conn.run_sync(Base.metadata.create_all)


I prefer to migrate or use schema + migrate to spot any issues regarding the migrations. I guess this will do for now. The tables are persisted though, since you do not call drop_all anywhere. Not a major issue, if we are using a new DB every time.

Performing the migration sounds like a sensible thing to do. I will see about changing this.

frisitano added 2 commits March 1, 2021 21:53

add Chain and Contract models + CRUD endpoints + celery chains reload…

68a72a8

… orchestration

refactor imports

0073938

frisitano mentioned this pull request Mar 1, 2021

[WIP] Network table #31

Closed

jbargu requested changes Mar 2, 2021

View reviewed changes

jbargu assigned frisitano Mar 2, 2021

frisitano added 24 commits March 7, 2021 18:32

add tests + github integration workflow

8f324cd

remove uses arg from git workflow

fbd78bb

update docker-compose + git workflow

260246a

reorder github workflow actions

f85aab9

correct docker attach command in github workflow

1c5a685

disable tty

cdfbffe

test refactor

c08d085

add debug step

a9b1d21

add quotation to force tilde recongition in .env

d79c206

change permissions for ~/.paralink dir in github workflow

0285f02

change permissions for ~/.paralink dir in github workflow

a3d220f

workflow

e272b1c

workflow

f777c87

workflow

24b32e8

workflow

cb5311b

workflow

e3fcd49

workflow

1e35c1d

workflow

412a6b7

workflow

f46b5b3

worker

ed8136b

workflow

fca3592

add contract address validation + integration test database

5a7de58

revert job name

b85cbec

add docstrings

9934299

frisitano requested a review from jbargu March 16, 2021 00:08

jbargu approved these changes Mar 16, 2021

View reviewed changes

jbargu merged commit 3a56b92 into paralink-network:master Mar 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network / Contracts models + CRUD endpoints + Celery Orchestration #44

Network / Contracts models + CRUD endpoints + Celery Orchestration #44

frisitano commented Mar 1, 2021 •

edited

Loading

jbargu left a comment

jbargu Mar 2, 2021

jbargu Mar 2, 2021

frisitano Mar 6, 2021

jbargu Mar 2, 2021

jbargu Mar 2, 2021

jbargu Mar 2, 2021

frisitano Mar 16, 2021

frisitano commented Mar 4, 2021

jbargu left a comment •

edited

Loading

jbargu Mar 16, 2021

frisitano Mar 20, 2021

jbargu Mar 16, 2021

frisitano Mar 20, 2021

jbargu Mar 16, 2021

frisitano Mar 20, 2021


		__tablename__ = "contracts"

		id = sa.Column(sa.String, primary_key=True)

Network / Contracts models + CRUD endpoints + Celery Orchestration #44

Network / Contracts models + CRUD endpoints + Celery Orchestration #44

Conversation

frisitano commented Mar 1, 2021 • edited Loading

jbargu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frisitano commented Mar 4, 2021

jbargu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frisitano commented Mar 1, 2021 •

edited

Loading

jbargu left a comment •

edited

Loading