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

feat: make a txsim binary build from Makefile and dockerfile #2052

Merged
merged 6 commits into from
Jul 24, 2023

Conversation

Bidon15
Copy link
Member

@Bidon15 Bidon15 commented Jul 10, 2023

docker/txsim.sh Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2023

Codecov Report

Merging #2052 (b1e0a68) into main (83aff91) will increase coverage by 0.02%.
The diff coverage is 25.00%.

❗ Current head b1e0a68 differs from pull request most recent head 6f5204b. Consider uploading reports for the commit 6f5204b to get more accurate results

@@            Coverage Diff             @@
##             main    #2052      +/-   ##
==========================================
+ Coverage   25.02%   25.05%   +0.02%     
==========================================
  Files         121      121              
  Lines       13806    13811       +5     
==========================================
+ Hits         3455     3460       +5     
  Misses       9995     9995              
  Partials      356      356              
Impacted Files Coverage Δ
x/blob/types/payforblob.go 73.55% <25.00%> (+0.65%) ⬆️

docker/Dockerfile_txsim Outdated Show resolved Hide resolved
docker/Dockerfile_txsim Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@Bidon15 Bidon15 requested a review from tty47 July 17, 2023 15:53
@Bidon15 Bidon15 marked this pull request as ready for review July 17, 2023 15:53
@Bidon15 Bidon15 requested a review from MSevey July 17, 2023 15:56
docker/README.md Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
Co-authored-by: Jose Ramon Mañes <[email protected]>
Copy link
Contributor

@tty47 tty47 left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
docker/Dockerfile_txsim Outdated Show resolved Hide resolved
docker/Dockerfile_txsim Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,122 @@
# Celestia `txsim` Docker Image Usage Guide

The `txsim` binary is a tool that can be used to simulate transactions on the Celestia network. It can be used to test the performance of the Celestia network.
Copy link
Collaborator

Choose a reason for hiding this comment

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

[no change needed][follow up issue] there are other Dockerfiles in this directory. They weren't touched in this PR but maintainers should consider adding to this README what the other files do (i.e. Dockerfile_ephemeral)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Ideally, we need to make a dedicated issue to document other 2 DockerFiles we have in the repo.
I started this ReadMe, so we can spec out not only the protocol, but what options do we have to run the software apart from vanilla make install && celestia-appd xyz in the host machine

Comment on lines +102 to +120
## Flag Breakdown

Here's a breakdown of what each flag means:

- `-k`: Whether a new key should be created (1 for yes, 0 for no)
- `-p`: The path to the keyring for the prefunded account
- `-r`: The RPC endpoints for the `txsim` binary
- `-g`: The gRPC endpoints for the `txsim` binary
- `-t`: The poll time for the `txsim` binary
- `-b`: The number of blob sequences to run
- `-a`: The range of blobs to send per PFB in a sequence
- `-s`: The range of blob sizes to send
- `-m`: The mnemonic for the keyring
- `-d`: The seed for the random number generator
- `-e`: The number of send sequences to run
- `-i`: The amount to send from one account to another
- `-v`: The number of send iterations to run per sequence
- `-u`: The number of stake sequences to run
- `-w`: The amount of initial stake per sequence
Copy link
Collaborator

Choose a reason for hiding this comment

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

ahh this legend really helps. It may be even clearer to use longer format flags. I.e. --seed, --key, etc. but not blocking

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, getopts doesn't support long-option flagging and writing a custom for-loop with edge cases of optional arguments is not what I really want to do.

As a v2, I'd like to remove completely this script and make it as server with REST APIs that we can trigger via a dashboard.

docker/txsim.sh Outdated Show resolved Hide resolved
You can run the `txsim` Docker image using the `docker run` command. Here's an example:

```bash
docker run -it -v ${HOME}/.celestia-app:/home/celestia ghcr.io/celestiaorg/txsim -k 0 -r http://consensus-validator-robusta-rc6.celestia-robusta.com:26657,http://consensus-full-robusta-rc6.celestia-robusta.com:26657 -g consensus-validator-robusta-rc6.celestia-robusta.com:9090 -t 10s -b 10 -d 100 -e 10
Copy link
Collaborator

Choose a reason for hiding this comment

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

[optional] I'm a Docker noob so I don't know what these shorthand flags do. The long format flags would help a lot for readability

Motivation: https://changelog.com/posts/use-long-flags-when-scripting

For example --volume instead of -v

Copy link
Member Author

@Bidon15 Bidon15 Jul 18, 2023

Choose a reason for hiding this comment

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

If I get rid of getopts, then yes, we will have long-format flags.
But I want to pursue a simple server approach that can be triggered on/off rather than a cli command 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

[still not blocking][optional] I don't see why getopts is related to this proposal. The docker run --help docs claim that --volume is the long form for -v

  -v, --volume list                    Bind mount a volume

and I guess -it can be replaced with

  -i, --interactive                    Keep STDIN open even if not attached
  -t, --tty                            Allocate a pseudo-TTY

but I think maybe the remaining flags are unique to the script running inside the docker container so they can't be replaced with longer format flags.

Suggested change
docker run -it -v ${HOME}/.celestia-app:/home/celestia ghcr.io/celestiaorg/txsim -k 0 -r http://consensus-validator-robusta-rc6.celestia-robusta.com:26657,http://consensus-full-robusta-rc6.celestia-robusta.com:26657 -g consensus-validator-robusta-rc6.celestia-robusta.com:9090 -t 10s -b 10 -d 100 -e 10
docker run --interactive --tty --volume ${HOME}/.celestia-app:/home/celestia ghcr.io/celestiaorg/txsim -k 0 -r http://consensus-validator-robusta-rc6.celestia-robusta.com:26657,http://consensus-full-robusta-rc6.celestia-robusta.com:26657 -g consensus-validator-robusta-rc6.celestia-robusta.com:9090 -t 10s -b 10 -d 100 -e 10

@Bidon15 Bidon15 requested a review from rootulp July 18, 2023 11:29
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @Bidon15 ! All my remaining feedback in this PR is optional so thanks for creating GH issues! FWIW I haven't tested this manually.


## docker-txsim-build: Build the Docker image tx simulator.
txsim-build-docker:
docker build -t ghcr.io/celestiaorg/txsim -f docker/Dockerfile_txsim .
Copy link
Collaborator

Choose a reason for hiding this comment

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

[optional]

Suggested change
docker build -t ghcr.io/celestiaorg/txsim -f docker/Dockerfile_txsim .
docker build --tag ghcr.io/celestiaorg/txsim --file docker/Dockerfile_txsim .

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +23 to +30

docker-txsim-build:
permissions:
contents: write
packages: write
uses: celestiaorg/.github/.github/workflows/[email protected]
with:
dockerfile: docker/Dockerfile_txsim
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we would just combine and put as a matrix?

  docker-security-build:
    strategy:
      matrix:
        dockerfile: [Dockerfile, docker/Dockerfile_txsim]
    permissions:
      contents: write
      packages: write
    uses: celestiaorg/.github/.github/workflows/[email protected]
    with:
      dockerfile: ${{ matrix.dockerfile }}

REF: https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#defining-the-maximum-number-of-concurrent-jobs

Github will try and run them in parallel, so unless there is a bug this is trying to work around, making them separate doesn't speed things up.

Copy link
Member

Choose a reason for hiding this comment

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

also can be a follow up, doesn't need to block in PR. 👍

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

lgtm utack

@evan-forbes
Copy link
Member

need one of us to merge @Bidon15?

@rootulp rootulp merged commit 7494cc7 into celestiaorg:main Jul 24, 2023
20 checks passed
@Bidon15 Bidon15 deleted the txsim-cmd branch August 4, 2023 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants