This repository has been archived by the owner on Jul 27, 2022. It is now read-only.
Problem:(CRO-541) no automated test for jailing, unjailing using liveness faults #604
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
because simultaneous integration test is the cause, it's tested on i7 cpu with 16 GB ram |
tomtau
requested review from
tomtau,
calvinaco,
devashishdxt,
lezzokafka,
linfeng-crypto and
yihuang
tomtau
suggested changes
Nov 20, 2019
There was a problem hiding this comment.
seems the test doesn't run on drone -- if you need something in the environment, you can use nix:
https://github.com/crypto-com/chain/blob/master/ci-scripts/drone.nix
https://github.com/crypto-com/chain/blob/master/.drone.yml#L43
tomtau
reviewed
Nov 20, 2019
client-rpc/src/rpc/staking_rpc.rs
Outdated
| @@ -44,6 +44,9 @@ pub trait StakingRpc: Send + Sync { | |||
| to_address: String, | |||
| view_keys: Vec<String>, | |||
| ) -> Result<String>; | |||
|
|
|||
| #[rpc(name = "staking_unjail")] | |||
| fn unjail(&self, request: WalletRequest, unjail_address: String) -> Result<String>; | |||
There was a problem hiding this comment.
can you base it off the latest master? this was already merged: ce3c976 so this PR will only show things that changed or are new
tomtau
reviewed
Nov 20, 2019
| @@ -0,0 +1,3 @@ | |||
| #!/bin/bash | |||
| tendermint node --p2p.persistent_peers "e04c6e7fcda7f3947179871053cec8de7b7a20cb@chain0:26656" | |||
There was a problem hiding this comment.
can this be taken from environment variable instead of hardcoded?
There was a problem hiding this comment.
i'll move to env variable
tomtau
reviewed
Nov 20, 2019
| @@ -0,0 +1,319 @@ | |||
| # This is a TOML config file. | |||
There was a problem hiding this comment.
could this setup be generated instead of hardcoded?
There was a problem hiding this comment.
may be useful to incorporate the chain bot @yihuang wrote
There was a problem hiding this comment.
it's possible, but scripts will be complex for now.
i'm thinking to make genesis generation directly by dev-conf.json directly.
i'll find a simpler way
There was a problem hiding this comment.
i'll check yihuang's script
tomtau
reviewed
Nov 20, 2019
| wait_for_ready(1) | ||
|
|
||
| count=2 | ||
| while True: |
There was a problem hiding this comment.
so this will hang forever (there's no timeout / max attempts) if the test fails?
There was a problem hiding this comment.
i'll add time limit, maybe two hours.
because it's constantly polling, it's not necessary to re-attempt.
There was a problem hiding this comment.
Right now the jailing period is 600s, do we want to reduce it to facilitate the test? Moreover, two hours timeout seems to be too long?
p.s. I think Drone.io has a 1 hour timeout right now.
There was a problem hiding this comment.
i'll change to 1 hour timeout
There was a problem hiding this comment.
it's configured max 1 hour
There was a problem hiding this comment.
why not around 10-20 minutes, if the jailing period is 10 minutes in the test?
|
@leejw51 @leejw51crypto not sure what packages you need, but if it's not available in nix directly, you can do something like: |
tomtau
reviewed
Nov 20, 2019
Comment on lines
11
to
13
| - "1022:22" | ||
| - "9981:9981" | ||
| - "26656:26656" | ||
| - "26657:26657" |
There was a problem hiding this comment.
as these ports are hardcoded, it won't be possible to run two tests in parallel -- @calvinaco @calvinlauco had something like that sorted in the existing integration tests
There was a problem hiding this comment.
@leejw51 @leejw51crypto Yes, there's a script to find a free port to use: https://github.com/crypto-com/chain/blob/master/ci-scripts/find-free-port.sh
There was a problem hiding this comment.
only vm0 needs external port-mapping for client-rpc, i'll remove un-necessary ports.
tomtau
reviewed
Nov 20, 2019
integration-tests/jail/run.sh
Outdated
Comment on lines
7
to
8
| pip3 install docker | ||
| python3 ./disk/jail_test.py |
There was a problem hiding this comment.
execute in nix -- no need for pip
tomtau
reviewed
Nov 20, 2019
| @@ -0,0 +1,3 @@ | |||
| ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQC9VL9tO86mtzyI+Js3EzxGXGrH7uiYJyI8M4laBpmuRuxeS3aistg+LoQ9bOnDx4B6eVrXmr7BQ1zXZhtOSyJfiCvTV+3RZta89Rp9K1MuF6dcZA9nAtlsxW8vEnh0BEFtRlsrvJkR6LKrB3airMmTxjRb6SnLi0vD1DO0W4Y3n7hDhqdVruRBWt/oPDLPyFdu4Z9Ffk4uWPJqOgEk63qU+x2iiwgnIlHi7/NX9YQMXBKWCsJu5PVgyAAYfk86Wlq1l5xDQnBYK+V+tf6zMmlkLgBqJFXQca5UTqsSYPnW5AlxI6JIrJAOsxX+4e7Qf1sSil6hz1RHcKa+dtThEzqp user@hongkong | |||
There was a problem hiding this comment.
why is ssh needed for the test?
There was a problem hiding this comment.
this is is for debugging, i'll remove it
calvinlauyh
reviewed
Nov 20, 2019
.drone.yml
Outdated
| commands: | ||
| - mkdir bin | ||
| - export PATH="$PATH:$PWD/bin" | ||
| - echo "OK" |
There was a problem hiding this comment.
This line can be removed I think
integration-tests/jail/Dockerfile
Outdated
| RUN apt-get install cmake libgflags-dev libzmq3-dev pkg-config -y | ||
| RUN apt-get install --no-install-recommends libssl-dev libzmq3-dev -y | ||
| RUN apt install unzip -y | ||
| RUN apt install tmux -y |
There was a problem hiding this comment.
I suggest to aggregate installation into one line to reduce layers. Also standardize the installation step to apt install
Codecov Report
@@ Coverage Diff @@
## master #604 +/- ##
==========================================
- Coverage 70.33% 70.33% -0.01%
==========================================
Files 133 133
Lines 16404 16404
==========================================
- Hits 11538 11537 -1
- Misses 4866 4867 +1
|
leejw51crypto
force-pushed
the
cro-541
branch
2 times, most recently
from
a6cf08e
to
7abd411
Compare
Comment on lines
82
to
83
| create_staking_address("a", "1") | ||
| create_staking_address("a", "1") |
There was a problem hiding this comment.
why create staking address twice?
integration-tests/jail/disk/test.py
Outdated
| return response.json()["result"] | ||
|
|
||
|
|
||
| def create_staking_address(name, passphrase): |
There was a problem hiding this comment.
I think you can create a python package for rpc client.
There was a problem hiding this comment.
leejw51crypto
force-pushed
the
cro-541
branch
4 times, most recently
from
35cd047
to
ce79778
Compare
integration-tests/jail/run.sh
Outdated
| #!/bin/bash | ||
| docker-compose up -d --build | ||
| echo "docker compose ok" | ||
| nix-shell -p python37Packages.docker --run "python3 ./disk/jail_test.py" |
leejw51crypto
force-pushed
the
cro-541
branch
from
ce79778
to
c9c8121
Compare
|
yes,
|
|
checked |
tomtau
reviewed
Dec 11, 2019
Comment on lines
+143
to
+144
| - export JAIL_CLIENT_RPC=9981 | ||
| - export JAIL_CHAIN_RPC=26657 |
There was a problem hiding this comment.
is this fixed? wouldn't the ports change every time? isn't only docker-compose down enough?
also it means that only one test can run at a time? check with @calvinaco on the integration test tear down
There was a problem hiding this comment.
I believe this is only to make docker-compose down happy, when starting, the ports are chosen from free ports.
tomtau
suggested changes
Dec 11, 2019
There was a problem hiding this comment.
As two instances of this test can't be executed in parallel, it'll be better that it's not triggered on every PR, so adding these conditions to the pipeline:
trigger:
branch:
- master
- staging
- trying
event:
- push
plus run all the docker-compose invocations with -p jail-<SOME UNIQUE/DETERMINISTIC ID IN ENV...>, so that docker-compose -p jail-<SOME UNIQUE/DETERMINISTIC ID IN ENV...> down will bring down the setup in that build rather than elsewhere
leejw51crypto
force-pushed
the
cro-541
branch
from
8104874
to
ef58736
Compare
yihuang
approved these changes
Dec 11, 2019
i'll work on this |
leejw51crypto
force-pushed
the
cro-541
branch
from
0cc3d85
to
ac8b74c
Compare
|
fixed using git commit hash |
tomtau
reviewed
Dec 11, 2019
| #!/bin/bash | ||
| echo "compile binaries" | ||
| cd ./compile | ||
| docker-compose up |
There was a problem hiding this comment.
there's still this "network" for compiler -- how does this get shut down? running a compiler should be doable without creating a docker compose network
There was a problem hiding this comment.
this docker-compose is not daemon,
it's synchronous.
no need to tear-down.
multiple running is fine.
|
the test fails: |
tomtau
reviewed
Dec 11, 2019
|
|
||
| --- | ||
| kind: pipeline | ||
| type: exec |
There was a problem hiding this comment.
can this execute in docker instead of exec? could use this base image https://hub.docker.com/r/nixos/nix/ ?
There was a problem hiding this comment.
this is the same with our integration test.
|
prior error is fixed, |
this is fixed |
|
could you run again? |
|
bors try |
|
bors try |
|
🔒 Permission denied Existing reviewers: click here to make leejw51crypto a reviewer |
|
checked |
tryTimed out |
leejw51
force-pushed
the
cro-541
branch
2 times, most recently
from
4375837
to
8c978f4
Compare
|
@leejw51crypto can you squash the commits and rebase them on top of the latest master? |
…ness faults add bin scripts add jailing intergration test add validators jail test show status check staked state after unjail wait for test activate jailing, unjailing intergration test tidy up tidy up add python error exit tidy up make apt install as one line use TENDERMINT_FLAG env variable remove ssh remove un-necessary script remove redundant code tidy up fix nix path add nix tidy up tidy up add nix path tidy up folders add path fix nix path tidy up port bind random port for simulataneous testing tidy up yml tidy up files activate test add zmq fix nix compile fix display compile zeromq fix jail.nix activate compiling fix toml remove config remove config recompile add clang change docker source fix FROM try with localhost change to url update chainbot tidy up tidy up ports tidy up assert tidy up python remove go0,go1 fix python exception remove ssh add tear down fix python test change name fix python exception warning tidy validators remove un-necessary assert remove un-necessary import update chainbot markdown apply dummy env tidy up do not use config directly remove tendermint argment handled in config.toml add project to docker-compose fix current_hash activate compile use project name in compiling support simultaneous running give enough time to boot up because if it's run simultaneously, 1/n times slower for tendermint, it takes long time~ restore to simple code make faster
leejw51crypto
force-pushed
the
cro-541
branch
from
2589439
to
e713bac
Compare
|
bors r+ |
bors bot
added a commit
that referenced
this pull request
Dec 21, 2020
2401: Bump cbindgen from 0.15.0 to 0.16.0 r=tomtau a=dependabot-preview[bot] Bumps [cbindgen](https://github.com/eqrion/cbindgen) from 0.15.0 to 0.16.0. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/eqrion/cbindgen/blob/master/CHANGES">cbindgen's changelog</a>.</em></p> <blockquote> <h2>0.16.0</h2> <pre><code> * Remove artificial restriction on lifetime parameters on enums ([#604](mozilla/cbindgen#604)) * Add an option for converting usize/isize into size_t/ptrdiff_t. ([#606](mozilla/cbindgen#606)) * Allow controlling the cargo profile used for expansion. ([#607](mozilla/cbindgen#607)) * Support wider range of expressions in enum discriminants ([#614](mozilla/cbindgen#614)) * Support generation of Cython bindings ([#590](mozilla/cbindgen#590)) * Fixed some issues with style=tag and recursive structs ([#615](mozilla/cbindgen#615)) * Default C style to Both (as specified in docs) ([#615](mozilla/cbindgen#615)) * Fix resolution of path dependencies from certain modules. ([#629](mozilla/cbindgen#629)) * Support inlined definitions for tuple variants with a single field in C ([#631](mozilla/cbindgen#631)) </code></pre> <p>Thanks to all the awesome contributors that contributed to this release.</p> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/eqrion/cbindgen/commit/a00b4215a907601680f6e9acaf93df1cbafa8ded"><code>a00b421</code></a> v0.16.0</li> <li><a href="https://github.com/eqrion/cbindgen/commit/b82e37525478d7d0bddbea2baf27e2eb9434531f"><code>b82e375</code></a> enum: Support inlined definitions for tuple variants with a single field</li> <li><a href="https://github.com/eqrion/cbindgen/commit/eaf3e57e743dba7621bfa59d0c5bf0656a79cb71"><code>eaf3e57</code></a> enum: Remove some redundant function parameters</li> <li><a href="https://github.com/eqrion/cbindgen/commit/0083c43e13c384787023c1fc8c491744d9f41070"><code>0083c43</code></a> Remove <code>Struct::tuple_struct</code></li> <li><a href="https://github.com/eqrion/cbindgen/commit/8a5db0baebfd45f4c43681151d111cdcb31bc6c5"><code>8a5db0b</code></a> Minor cleanup to <code>fn close_brace</code></li> <li><a href="https://github.com/eqrion/cbindgen/commit/1963f0c92e93456359713db353aa6276f6200f7b"><code>1963f0c</code></a> Partially support <code>#[cfg]</code>s on fields</li> <li><a href="https://github.com/eqrion/cbindgen/commit/dfcee869ba536e661a0c972b5407ea3a5579d960"><code>dfcee86</code></a> enum: Do not forget to rename entities in enum discriminants</li> <li><a href="https://github.com/eqrion/cbindgen/commit/fbc2237b7d7b8ab250ec184c709bc5d4129fceab"><code>fbc2237</code></a> parser: Fix resolution of #[path] dependencies from certain modules.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/9f558e30f3313f2fe6fcdc172b6f86c619564414"><code>9f558e3</code></a> enum: <code>enum_name</code> -> <code>tag_name</code></li> <li><a href="https://github.com/eqrion/cbindgen/commit/8997277cb71922b028a003349c59bd87ae0afe4d"><code>8997277</code></a> enum: Break up <code>Enum::write</code> into multiple functions</li> <li>Additional commits viewable in <a href="https://github.com/eqrion/cbindgen/compare/v0.15.0...v0.16.0">compare view</a></li> </ul> </details> <br /> [](https://dependabot.com/compatibility-score/?dependency-name=cbindgen&package-manager=cargo&previous-version=0.15.0&new-version=0.16.0) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language - `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com): - Update frequency (including time of day and day of week) - Pull request limits (per update run and/or open at any time) - Out-of-range updates (receive only lockfile updates, if desired) - Security updates (receive only security updates, if desired) </details> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Solution:add integration test using docker-compose