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

[GCP] Add gcp.force_enable_external_ips configuration #3699

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Ultramann
Copy link

This PR adds a new configuration to the ~/.sky/config.yaml for gcp, force_enable_external_ips. Setting this configuration to true (default is false for backwards compatibility) will cause the gcp subnet provisioner to create VMs with a public ip address even if use_internal_ips is set to true; this is useful when communication within a vcp is desired and the VM needs to make calls to the public internet.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: conda deactivate; bash -i tests/backward_compatibility_tests.sh

@Ultramann Ultramann changed the title Add gcp.force_enable_external_ips configuration [GCP] Add gcp.force_enable_external_ips configuration Jun 27, 2024
@Michaelvll Michaelvll self-requested a review June 28, 2024 19:16
Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

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

Thanks for adding the support of this new knob @Ultramann! LGTM.

nit: to make it expose to users, let's add it to our docs as well: https://github.com/skypilot-org/skypilot/blob/master/docs/source/reference/config.rst

Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

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

Thanks for the update @Ultramann! This looks good to me. Once we have tests added, it should be good to go.



Force Enable Exteral IPs
~~~~~~~~~~~~~~~~~~~~~~~
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~~~~~~~

This cause the failure of CI ; )

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

2 participants