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

Make the local network script support a dense3 network option #2988

Merged
merged 4 commits into from
Jan 6, 2023

Conversation

cbeck88
Copy link
Contributor

@cbeck88 cbeck88 commented Jan 6, 2023

The purpose of this was to do a local slam with prometheus and compare to the results we get in the cloud.

My results were like this:

image

image

So, I did not match the ~100 tx/s that Eran was seeing in the dev networks, but I did get about twice as high as I was getting for a 5 node network locally. (See other results here: #2934)

This is comparable to how dev networks deploy right now

This also changes the prometheus.yml.template so that we start
prometheus if we have a LOG_BRANCH variable, but we don't attempt
to push to grafana unless we have a GRAFANA_PASSWORD set
@cbeck88 cbeck88 requested review from eranrund and a team January 6, 2023 03:32
basic_auth:
username: 8687
password: ${GRAFANA_PASSWORD}
'''.replace('${GRAFANA_PASSWORD}', grafana_password) if grafana_password else ""
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's a bit janky to remove this stuff from the .template file but I didn't see a simpler way to accomplish this

i'm not sure the grafana push stuff is really important to us anymore, we can just use prometheus directly when doing local networks

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not fully understanding why this got removed from the template file. it seems to be doing more or less the same logic and looks like it could stay in the template file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to be able to start Prometheus even if i don't have a grafana password

I guess i could check what happens if you put a junk password in, maybe it fails authentication and then continues running normally

🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh I misinterpreted the code I was thinking this was setting the password to "" but it's setting the template_extra to ""

basic_auth:
username: 8687
password: ${GRAFANA_PASSWORD}
'''.replace('${GRAFANA_PASSWORD}', grafana_password) if grafana_password else ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not fully understanding why this got removed from the template file. it seems to be doing more or less the same logic and looks like it could stay in the template file.

tools/local-network/local_network.py Outdated Show resolved Hide resolved
tools/local-network/local_network.py Show resolved Hide resolved
for i in range(num_nodes):
other_nodes = [str(j) for j in range(num_nodes) if i != j]
peers = [Peer(p) for p in other_nodes]
self.add_node(str(i), peers, QuorumSet(1, other_nodes))
Copy link
Collaborator

Choose a reason for hiding this comment

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

❓ With a quorum set of 1 do we get much benefit use out of having 3 nodes running?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the quorum sets are "self + 1", so this effectively means that 2 nodes have to validate anything that happens. So that's what we want in a 2 out of 3 configuration

@nick-mobilecoin nick-mobilecoin requested a review from a team January 6, 2023 15:45
it looks that prometheus doesn't particularly care if the remote-write
part doesn't work due to lack of a grafana password, so we can leave it
in the template file unconditionally
@cbeck88 cbeck88 merged commit d4dd54e into release/v4.0 Jan 6, 2023
@cbeck88 cbeck88 deleted the add-dense-3-network-option branch January 6, 2023 18:26
aweis89 pushed a commit that referenced this pull request Jan 10, 2023
* Make the local network script support a dense3 network option

This is comparable to how dev networks deploy right now

This also changes the prometheus.yml.template so that we start
prometheus if we have a LOG_BRANCH variable, but we don't attempt
to push to grafana unless we have a GRAFANA_PASSWORD set

* simplify the part about disabling grafana

it looks that prometheus doesn't particularly care if the remote-write
part doesn't work due to lack of a grafana password, so we can leave it
in the template file unconditionally

* fixup whitespace

* update documentation for the script
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