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.

Sign up for GitHub

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

Jump to bottom

chore: add supervisor for lpt infra #3176

Merged
merged 12 commits into from
Nov 26, 2024
Merged

chore: add supervisor for lpt infra #3176

merged 12 commits into from
Nov 26, 2024

Conversation

NagyZoltanPeter
Copy link
Contributor

Description

This PR is based upon the previous one, and serves the same sole purpose: enable infra deployment.
Prev PR: #3158

Changes

  • After agreement with Marko: https://github.com/status-im/infra-misc/pull/380 we decided that infra deployment image must run continuously. As this is against how this test is designed it is now extended:
  • Added lpt_supervisor.py script that takes default deployment arguments and a period time arg, and restarts tester apps in predefined times. (This has to be set carefully to allow both parties (SENDER/RECEIVER) to finish before next period comes.)
  • infra.env : predefined deployment arguments to testing, built into the image
  • if the image tag is set to deploy for docker build, it will create a specific image for infra deployment (having different entrypoint with lpt_supervisor.py)
  • As a plus, as Franck requested, I added message latency metrics (latency was already reported but only to stdout).

@NagyZoltanPeter
Adding lpt-runner script and assemble into liteprotocoltester image -…
f3be1b7 
… to ease infra deployment
@NagyZoltanPeter
Add supervisor that can run lpt continously in infra environment, inf…
ffe67ac 
…ra.env defines defaults for run, in case image tag of lpt docker image is deploy it will build a specific image for infra deployment.
@NagyZoltanPeter
Added message latency metrics
24daf46
@NagyZoltanPeter
Fix typo
b8f6522
@NagyZoltanPeter NagyZoltanPeter self-assigned this Nov 10, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 10, 2024 •
edited
Loading

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3176

Built from c939756

gabrielmer
gabrielmer approved these changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Thank you!

Ivansete-status
Ivansete-status approved these changes Nov 20, 2024
View reviewed changes
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for it! 💯
I just added some minor comments

Makefile
--build-arg="MAKE_TARGET=liteprotocoltester" \
--build-arg="NIMFLAGS=$(DOCKER_LPT_NIMFLAGS)" \
--build-arg="NIM_COMMIT=$(DOCKER_NIM_COMMIT)" \
--build-arg="LOG_LEVEL=TRACE" \
--label="commit=$(shell git rev-parse HEAD)" \
--label="version=$(GIT_VERSION)" \
--target $(TARGET) \
--target $(if $(filter deploy,$(DOCKER_LPT_TAG)),deployment_lpt,standalone_lpt) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the diff between deployment_lpt and standalone_lpt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the distinction between the different use. deployment_lpt stands for running in our infra environment with its own constraints. - main diff is that in deployment we need to run a supervisor ahead of tests that can control restarts.
standalone_lpt is the normal usage, so in lpt_runner you can easily run your tests on demand.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for clarifying! I was wondering if we could add a comment somewhere, if not done already:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah must add it to lpt readme at least!

apps/liteprotocoltester/infra.env Outdated
TEST_INTERVAL_MINUTES=10
START_PUBLISHING_AFTER=120
NUM_MESSAGES=300
DELAY_MESSAGES=1000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could have a more explicit var name;P

Suggested change
DELAY_MESSAGES=1000
MSG_INTERVAL_MILLIS=1000

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I need to apply such change more deep. I will check!

apps/liteprotocoltester/infra.env Outdated
@@ -0,0 +1,11 @@
TEST_INTERVAL_MINUTES=10
START_PUBLISHING_AFTER=120
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
START_PUBLISHING_AFTER=120
START_PUBLISHING_AFTER_SECS=120

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just as above, thanks for the suggestion.

@NagyZoltanPeter NagyZoltanPeter merged commit a7264d6 into master Nov 26, 2024
8 of 10 checks passed
@NagyZoltanPeter NagyZoltanPeter deleted the chore-add-supervisor-for-lpt-infra branch November 26, 2024 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ivansete-status Ivansete-status Ivansete-status approved these changes

@gabrielmer gabrielmer gabrielmer approved these changes

@markoburcul markoburcul markoburcul approved these changes

@darshankabariya darshankabariya Awaiting requested review from darshankabariya

Assignees

@NagyZoltanPeter NagyZoltanPeter

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NagyZoltanPeter @markoburcul @gabrielmer @Ivansete-status