one process per baker #517
Merged
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
|
I find the DAEMON env var useful because I set |
When baking for several addresses, today we instantiate only one baker process. This is unsupported and bad, because it does things sequentially. The supported way to bake for several addresses is to run one baking process per address. This PR changes tezos-k8s to do that. This is a second, less intrusive attempt after feedback from Aryeh. * helm computes the maximum number of baking processes that any pod in the statefulset would need * instantiates the max number of bakers for each pod (no choice here..) * the baker entrypoint figures which baker it must bake for thanks to a new BAKER_INDEX env var. If it must idle, it enters an infinite sleeping loop. This works when using `bake_using_accounts`. I verified that I did not break `bake_using_account`. I verified by: * creating a private chain with 3 bakers using mkchain * assigning two bakers to the first node and one baker to the second I also ran mkchain with no parameters to ensure it still works. Various cleanups: * remove a octez v13 specific catch (must pass toggle even when vote file absent) in the baker entrypoint * remove the DAEMON env var in baker (was used to disambiguate between baker and endorser) Random comment: I don't think it's great that the logic to pick the baking accounts is written in config-generator.sh instead of py, but I'm not changing this here.
nicolasochem
force-pushed
the
multiple_baker_processes_2
branch
from
f9e2afc
to
5b65c01
Compare
I reinstated it. Thanks for looking, a review would me much appreciated. |
harryttd
reviewed
Dec 29, 2022
There was a problem hiding this comment.
Looks great to me. I have not thoroughly tested but the code looks good.
Do you mind:
- removing the comment
remove the DAEMON env var...from the PR description - leaving a comment that this PR's changes should be temporary. Based on changes that should be taking place in the future to how the node and baker will be functioning differently.
- Merge latest master into this PR
| @@ -255,17 +259,31 @@ | |||
| {{- define "tezos.container.bakers" }} | |||
| {{- if has "baker" $.node_vals.runs }} | |||
| {{- $node_vals_images := $.node_vals.images | default dict }} | |||
There was a problem hiding this comment.
I know this line was not introduced in this PR but i don't see why it is needed.
harryttd
approved these changes
Dec 29, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When baking for several addresses, today we instantiate only one baker process. This is unsupported and bad, because it does things sequentially. The supported way to bake for several addresses is to run one baking process per address. This PR changes tezos-k8s to do that.
This is a second, less intrusive attempt after feedback from Aryeh.
This works when using
bake_using_accounts. I verified that I did not breakbake_using_account.I verified by:
I also ran mkchain with no parameters to ensure it still works.
Various cleanups:
Note that in the future, the baker will be its own pod to this will become irrelevant.