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

[LocalNet] Run Relayer and AppGateServer #179

Merged
merged 6 commits into from
Nov 16, 2023
Merged

Conversation

okdas
Copy link
Member

@okdas okdas commented Nov 11, 2023

Summary

Human Summary

As we've got relayer moved from the alpha repo, and appgateserver merged into main, we need to be able to run them on LocalNet. Support for this on helm-charts side have been merged.

AI Summary

Summary generated by Reviewpad on 11 Nov 23 17:40 UTC

This pull request introduces changes related to the relayer and appgateserver.

It adds new values files for the appgateserver and relayer in the localnet/kubernetes directory and sets their enable values to true.

Additionally, it modifies the Tiltfile to include the appgateserver helm resource, which uses the new values file and sets the replicaCount based on the localnet configuration.

Overall, this patch adds support for the appgateserver and enables the relayer in the localnet setup.

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

  • Run all unit tests: make go_develop_and_test
  • Verify Localnet manually: See the instructions [here](TODO: add link to instructions)

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have performed a self-review of my own code
  • I have commented my code, updated documentation and left TODOs throughout the codebase

@okdas okdas added this to the Shannon TestNet milestone Nov 11, 2023
@okdas okdas self-assigned this Nov 11, 2023
@okdas okdas marked this pull request as ready for review November 11, 2023 18:53
@Olshansk Olshansk added the infra Infra or tooling related improvements, additions or fixes label Nov 14, 2023
@Olshansk Olshansk self-requested a review November 14, 2023 04:45
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

This LGTM. @okdas What do you see as the blocking next step to get this in?

@bryanchriswhite
Copy link
Contributor

bryanchriswhite commented Nov 14, 2023

Based on the state of charts/poktroll/templates/scripts.yaml on main, it looks like we still need to:

  1. Update the subcommand name relayer -> relayminer
  2. Decide when/how to workaround or integrate with the solution to the supplier1/app1 vs. supplier2/app2 for the relayminer's and appgate server's signing-key flags, respectively. See #poktroll in discord and #177 comment.
  3. (Optional, based on 2) ensure that the suppler and app both have public keys on chain.

Tiltfile Outdated
@@ -127,11 +128,23 @@ helm_resource(
poktroll_chart,
flags=[
"--values=./localnet/kubernetes/values-common.yaml",
"--values=./localnet/kubernetes/values-relayer.yaml",
Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2023-11-14 at 19 44 45

Still getting this error with the relayers in tilt. Is it still looking for the old command and not the new one?

Copy link
Member Author

@okdas okdas Nov 14, 2023

Choose a reason for hiding this comment

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

I am surprised you still see that, as we no longer have the pocketd binary. Let's circle back to this if you still have that issue once we merge everything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Confirmed: this would not happen on a PR with relayminer implemented. We just need to merge everything.

h5law
h5law previously requested changes Nov 14, 2023
Copy link
Contributor

@h5law h5law left a comment

Choose a reason for hiding this comment

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

AppGateServer works perfectly in localnet k8s! 🔥 Still not working for me with the relayerminer but once thats done this is gunna be so good!

@@ -0,0 +1,2 @@
relayer:
Copy link
Member

Choose a reason for hiding this comment

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

Per @bryanchriswhite's comment, we need to rename this to relayminer

@Olshansk
Copy link
Member

@bryanchriswhite Agreed regarding s/relayer/relayminer in this PR.

Re (2) & (3) - I believe it's outside the scope of this PR and we'll capture it in #180.

@okdas
Copy link
Member Author

okdas commented Nov 14, 2023

Per discussion above, I'm going to tweak the helm charts to use relayminer instead of relayer.

@bryanchriswhite bryanchriswhite mentioned this pull request Nov 14, 2023
10 tasks
@okdas okdas requested a review from Olshansk November 15, 2023 00:22
@okdas
Copy link
Member Author

okdas commented Nov 15, 2023

I ended up splitting helm charts for appgate-server and relayminer - https://github.com/pokt-network/helm-charts/pull/6/files. The services are going to have different configs, ports exposed so it made sense to separate them. Also, this should potentially simplify key management for node runners. We will get to that point in a few weeks. :)

Just need a review on that PR and we should be good.

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Left one comment but LGTM otherwise

@@ -19,5 +19,7 @@ RUN mv /poktroll/bin/poktrolld /usr/bin/poktrolld

EXPOSE 8545
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Adding a comment on what each exposed port is used for will go a long way!

Copy link
Member Author

Choose a reason for hiding this comment

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

@Olshansk these will be changed soon, I'll prepare a doc for us to discuss the configs/ports and dependencies for different actors.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. In that case, please see the suggested comment I left below.

@okdas okdas dismissed h5law’s stale review November 15, 2023 19:38

Relayminer doesn't work b/c it is not merged yet.

Co-authored-by: Daniel Olshansky <[email protected]>
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

@okdas Anything blocking from getting this in?

@okdas
Copy link
Member Author

okdas commented Nov 16, 2023

@okdas Anything blocking from getting this in?

nope, let's get it in!

@okdas okdas merged commit c08a103 into main Nov 16, 2023
8 checks passed
bryanchriswhite added a commit that referenced this pull request Nov 16, 2023
…relayer_cli

* pokt/main:
  [LocalNet] Run Relayer and AppGateServer (#179)
  [Relay] E2E Relay Gaps (#177)
bryanchriswhite added a commit that referenced this pull request Nov 16, 2023
…-update

* merge/e2e_test/relay_x_relayer_cli:
  [LocalNet] Run Relayer and AppGateServer (#179)
  [Relay] E2E Relay Gaps (#177)
  More tiny comment updates
  Added a couple more comments
  Update some comments and TODOs
  Update the names and references to queryNode/sequencerNode/fullNode etc
  Update pkg/relayer/cmd/cmd.go
  [Test] Updating `relay.feature` to run curl command to enable E2E Relay Test (#178)
  Updated comments for post 177+179 work for okdas
  Update OpenAPI spec
  Update .gitignore
  chore: update comment
  chore: move shared dependency setup logic to shared pkg
  chore: cleanup flags and dependencies for appgateserver cmd
  [Supplier] chore: improve supplier not found error message (#183)
  [CI] Integrate E2E tests with GitHub CI (#152)
okdas added a commit that referenced this pull request Nov 14, 2024
* relayer+appgateserver

* switch to the other chart

* cleanup

* Update Dockerfile.dev

Co-authored-by: Daniel Olshansky <[email protected]>

---------

Co-authored-by: Daniel Olshansky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra Infra or tooling related improvements, additions or fixes
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants