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

Some Updates to worker management #2494

Merged
merged 8 commits into from Jan 12, 2021
Merged

Some Updates to worker management #2494

merged 8 commits into from Jan 12, 2021

Conversation

vepkenez
Copy link
Contributor

@vepkenez vepkenez commented Jan 5, 2021 •
edited

Type of PR:

  • Bugfix
  • Feature
  • Documentation
  • Other

Required reviews:

  • 1
  • 2
  • 3

What this does:

  • Fixes a couple of bugs discovered by @arjunhassard
  • adds nucypher cloudworkers stop feature
  • handle change from envvar to cli args for --max-gas-price and generally make cli arg handling more flexible
  • improve performance when no seed node present in node inventory
  • small UX improvements

Why it's needed:

  • Some basic functionality (ability to set gas price and update nodes in certain contexts) is currently broken... this fixes it.

@KPrasch
Copy link
Member

KPrasch commented Jan 5, 2021 •
edited

Looks like you've got a malformed RST table causing CI failure - looking good otherwise /home/circleci/nucypher/docs/source/guides/network_node/nucypher_host_management_cli.rst:19:Malformed table.

One question: Is this the best way to handle a restart and not just a stop?

derekpierre
derekpierre reviewed Jan 6, 2021
View reviewed changes
deploy/ansible/worker/include/run_ursula.yml
@@ -85,14 +85,14 @@
env: "{{runtime_envvars}}"

- name: "wait a few seconds for the seed node to become available"
when: SEED_NODE_URI is not undefined
when: SEED_NODE_URI is not undefined and SEED_NODE_URI
Copy link
Member

Choose a reason for hiding this comment

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

is the extra check for ensuring the SEED_NODE_URI value is non-empty?

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... it errors if SEED_NODE_URI is undefined but it can still be None.
So the first check is to avoid an error if it's undefined, the 2nd one is the actual logic of detecting if it has a non None value.

nucypher/cli/commands/cloudworkers.py Outdated Show resolved Hide resolved
nucypher/cli/commands/cloudworkers.py Outdated Show resolved Hide resolved
KPrasch
KPrasch reviewed Jan 8, 2021
View reviewed changes
Copy link
Member

@KPrasch KPrasch left a comment

Choose a reason for hiding this comment

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

How can we recommended to restart nodes via clouddeploy given these new changes (think updating configuration)?

MANIFEST.in
@@ -12,4 +12,6 @@ recursive-include nucypher/blockchain/eth/aragon_artifacts *.json
recursive-include nucypher/blockchain/eth/contract_registry *.json *.md
prune nucypher/blockchain/eth/contract_registry/historical
recursive-include nucypher/network/templates *.html *.mako
recursive-include nucypher/utilities/templates *.html *.mako
recursive-include deploy/ansible/worker *.yml
Copy link
Member

Choose a reason for hiding this comment

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

Alright! Getting this thing published.

deploy/ansible/worker/include/run_ursula.yml
@@ -76,7 +76,7 @@
max-file: "5"
image: "{{ nucypher_image | default('nucypher/nucypher:latest') }}"
restart_policy: "unless-stopped"
command: "nucypher ursula run {{nucypher_ursula_run_options | default('')}} --lonely {{prometheus | default('')}} {{gas_strategy | default('')}} --network {{network_name}}"
command: "nucypher ursula run {{nucypher_ursula_run_options}} --lonely"
Copy link
Member

Choose a reason for hiding this comment

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

👍 much needed cleanup

@KPrasch
Copy link
Member

KPrasch commented Jan 11, 2021

Is this now the recommended way to restart a node via clouddeploy?

@vepkenez
Copy link
Contributor Author

@KPrasch I don't really have a dedicated restart just calling update with no args would do that. Should I add a restart?

@KPrasch
Copy link
Member

KPrasch commented Jan 11, 2021

I'll defer to you on that one. Just wondering if this is a good way to handle changing --max-gas-price, although we can solve that another way too. IMO, it's not too bad to just call stop and then start with the new value.

@vepkenez
Copy link
Contributor Author

The only reason to call stop would be to shut down the node and have it not start again. Like for example @arjunhassard had a situation where his node was out of gas money, and starting up, checking for funding and crashing over and over was using up his Infura quota. So he wanted to stop it for awhile.

A docker pull and restart happen for update and deploy.
nucypher cloudworkers update -c max-gas-price=50 is how you'd do what you're talking about.

@KPrasch
Copy link
Member

KPrasch commented Jan 11, 2021

A docker pull and restart happen for update and deploy.
nucypher cloudworkers update -c max-gas-price=50 is how you'd do what you're talking about.

Ah yes - Got it. Thanks for clarifying!

@vepkenez
Copy link
Contributor Author

vepkenez commented Jan 11, 2021 •
edited

I am actually struggling with how to remove a previously configured CLI arg.
Like if I want to unset max-gas-price or --prometheus

How do we unset config file values?

@KPrasch
Copy link
Member

KPrasch commented Jan 11, 2021

I am actually struggling with how to remove a previously configured CLI arg.
Like if I want to unset max-gas-price or --prometheus

Yeah, since --max-gas-price is enforced as a decimal it's not possible to set it to null|None via the CLI. We might need a new option to unset it.

@KPrasch
Copy link
Member

KPrasch commented Jan 12, 2021

I'm prepared to approve, but there are outstanding comments and questions from @derekpierre

KPrasch
KPrasch approved these changes Jan 12, 2021
View reviewed changes
Copy link
Member

@KPrasch KPrasch left a comment

Choose a reason for hiding this comment

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

Approved but awaiting responses

@vepkenez
Copy link
Contributor Author

Pushing the RFC fixes in a few minutes

derekpierre
derekpierre approved these changes Jan 12, 2021
View reviewed changes
Copy link
Member

@derekpierre derekpierre left a comment

Choose a reason for hiding this comment

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

🎸

@vepkenez
Copy link
Contributor Author

@KPrasch I just noticed that I had actually avoided the unsetting issues by only applying cli args on the ursula run and not the ursula config so it's not an issue (thanks me from two weeks ago!)

@KPrasch
Copy link
Member

KPrasch commented Jan 12, 2021

Ah yes, that's a good point. Technically the lack of ability to unset max gas price applies only to config, not one-shot 👍

@KPrasch KPrasch merged commit 3733644 into nucypher:main Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KPrasch KPrasch KPrasch approved these changes

@derekpierre derekpierre derekpierre approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vepkenez @KPrasch @derekpierre