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

WIP: blockfrost module and test #81

Merged
merged 10 commits into from
Jul 15, 2024
Merged

WIP: blockfrost module and test #81

merged 10 commits into from
Jul 15, 2024

Conversation

avnik
Copy link
Contributor

@avnik avnik commented May 27, 2024

No description provided.

@avnik avnik marked this pull request as draft May 27, 2024 17:59
@avnik avnik requested a review from aciceri May 28, 2024 15:23
Copy link
Member

@aciceri aciceri left a comment

Choose a reason for hiding this comment

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

I prepared a branch with minimal changes to make CI happy. Feel free to re-implement them here.

Also in the branch I'm also starting an ssh server so that you can connect with ssh root@localhost -p 2000 after having started the test interactively with nix run .#vmTests-blockfrost -- --interactive.
I don't if you already knew this "trick" but I believe it can help a lot.

Moreover if possible let's test something more significative besides /health, perhaps we can check /blocks/latest to see if the slot number is greater than a certain amount? Something similar to what we did for cardano-db-sync I mean.

Comment on lines +28 to +30
blockfrost = {
url = "github:blockfrost/blockfrost-backend-ryo?ref=v2.0.2";
};
Copy link
Member

Choose a reason for hiding this comment

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

We support multiple versions for the same packages now. Even if we'll have only one blockfrost for now I would already start preparing multiple blockfrost versions support for instance by renaming this input to something like blockrost-2.0.2. Feel free to address this in another PR so that we can merge this ASAP.

Copy link
Member

Choose a reason for hiding this comment

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

Take a look at what has been done for cardano-node, ogmios and kupo. Also you should export the blockfrost derivation between this flake's packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment this done only for node, -- kupo, dbsync, ogmios remain unversioned (and I feel this versioning follow to bloating code, and should be used only when neccesary). So should I rename it?

Copy link
Member

Choose a reason for hiding this comment

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

Ok let's ignore this until we support multiple versions.

machine.wait_until_succeeds("""[[ $(echo "$(cardano-cli query tip --testnet-magic ${magic} | jq '.syncProgress' --raw-output) > 0.001" | bc) == "1" ]]""")
machine.wait_for_unit("blockfrost-backend-ryo")
time.sleep(10)
machine.succeed("curl http://localhost:3000/health")
Copy link
Member

Choose a reason for hiding this comment

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

I believe it's /api/v0/health

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, /api/v0 is only on public instances, local ones have straight /health

machine.wait_for_unit("cardano-node-socket")
machine.wait_until_succeeds("""[[ $(echo "$(cardano-cli query tip --testnet-magic ${magic} | jq '.syncProgress' --raw-output) > 0.001" | bc) == "1" ]]""")
machine.wait_for_unit("blockfrost-backend-ryo")
time.sleep(10)
Copy link
Member

Choose a reason for hiding this comment

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

The reason why the test is failing is because this is not enough in CI. Anyway I would avoid using timeouts like this and suggest machine.wait_until_succeeds().

time.sleep(10)
machine.succeed("curl http://localhost:3000/health")
machine.succeed("curl --silent --fail http://localhost:3000/health")
print(machine.succeed("systemd-analyze security blockfrost"))
Copy link
Member

Choose a reason for hiding this comment

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

For some reason this is also returning non-zero, I haven't investigated it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Different module name, it should be "blockfrost-ryo". Already fixed this

Copy link
Member

@aciceri aciceri left a comment

Choose a reason for hiding this comment

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

@brainrake could you also please take a look? Especially for what concerns the internal coherence with other modules and if the options interface really makes sense in all cases (what if one runs postgres on a different machine? These kind of scenarios I mean...). I was wondering if it should also have an assertion but I'm not sure what it should assert.

PS: @avnik remember to check how docs look like

Comment on lines +28 to +30
blockfrost = {
url = "github:blockfrost/blockfrost-backend-ryo?ref=v2.0.2";
};
Copy link
Member

Choose a reason for hiding this comment

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

Ok let's ignore this until we support multiple versions.

Comment on lines 13 to 14
mkEnableOption ''
'';
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we put something here?

Copy link
Member

Choose a reason for hiding this comment

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

Check here. I t would be nice also having an example.

enable = true;
settings = {
inherit (config.cardano) network;
server.debug = true;
Copy link
Member

Choose a reason for hiding this comment

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

This is related to logs, right? Do we want it by default? Don't have a strong opinion honestly...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, debug probably could be removed. Was need to figure out that we had db connection issues

perSystem.vmTests.tests.blockfrost = {
impure = true;
module = {
nodes .machine = {pkgs, ...}: {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
nodes .machine = {pkgs, ...}: {
nodes.machine = {pkgs, ...}: {

ProtectKernelModules = true;
SystemCallArchitectures = "native";
# FIXME: Turn MemoryDenyWriteExecute prevent service from work
# MemoryDenyWriteExecute = true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# MemoryDenyWriteExecute = true;
# MemoryDenyWriteExecute = true;

ProtectKernelTunables = true;
RestrictRealtime = true;
# FIXME: Turn SystemCallFilter prevent service from work
# SystemCallFilter = ["@system-service" "~@privileged"];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# SystemCallFilter = ["@system-service" "~@privileged"];
# SystemCallFilter = ["@system-service" "~@privileged"];

modules/blockfrost.nix Outdated Show resolved Hide resolved
modules/blockfrost.nix Outdated Show resolved Hide resolved
modules/blockfrost.nix Outdated Show resolved Hide resolved
modules/blockfrost.nix Outdated Show resolved Hide resolved
modules/blockfrost.nix Outdated Show resolved Hide resolved
modules/blockfrost.nix Outdated Show resolved Hide resolved
modules/blockfrost.nix Outdated Show resolved Hide resolved
avnik and others added 5 commits July 5, 2024 14:06
Signed-off-by: Alexander V. Nikolaev <avn@avnik.info>
Co-authored-by: Andrea Ciceri <andrea.ciceri@autistici.org>
Co-authored-by: Alexander V. Nikolaev <avn@avnik.info>
Signed-off-by: Alexander V. Nikolaev <avn@avnik.info>
Signed-off-by: Alexander V. Nikolaev <avn@avnik.info>
Improved behaviour in case when db-sync and/or postgress not enabled locally

Signed-off-by: Alexander V. Nikolaev <avn@avnik.info>
It was causing issues with package options having
as default values packages not in `pkgs`
@nrutledge nrutledge mentioned this pull request Jul 8, 2024
@aciceri aciceri marked this pull request as ready for review July 15, 2024 10:32
@aciceri aciceri merged commit 8a4df85 into main Jul 15, 2024
2 checks passed
@aciceri aciceri deleted the avnik/blockfrost branch July 15, 2024 10:32
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.

None yet

3 participants