-
Notifications
You must be signed in to change notification settings - Fork 247
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
Passes --public-ip-address
to hailctl dataproc start
#14653
Passes --public-ip-address
to hailctl dataproc start
#14653
Conversation
* Add support for public-ip-address in dataproc * Use the same code style as previous code --------- Co-authored-by: Michael Franklin <illusional@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking this change. Looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no changes since last review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small question about whether this needs to be an option, since it seems like a strict necessity. But happy to move over to 👍 if the answer is "yes"
Opt( | ||
help='Allow nodes to have a public IP address, and hence make requests on the public internet (default is internal-only from dataproc 2.2).' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From your description:
since we need internet access to install some of our dependencies
Is there any use case where someone would want to disable public IP addresses and not have the dependencies set up? If not, should this just be forced to be true (instead of being an option)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, looks like we get the list of dependencies from hail/python/hailtop/hailctl/deploy.yaml
, which is generated when we build the wheel, so there is not a way for the user to avoid using the packages we specify. Revised to just make this part of the command string instead of an option!
FWIW, this change worked fixed our dataproc initialisation :) |
--public-ip-address
arg to hailctl dataproc start
and enables it by default--public-ip-address
to hailctl dataproc start
Closes #14652.
See populationgenomics#346. Thanks for the contribution @illusional!
Gives Dataproc clusters started via
hailctl dataproc start
internet access by default, since we need it to install some of our dependencies, per the error message in the linked issue.