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

Handle kube config management for sky local commands #2253

Merged
merged 3 commits into from
Jul 19, 2023
Merged

Handle kube config management for sky local commands #2253

merged 3 commits into from
Jul 19, 2023

Conversation

hemildesai
Copy link
Contributor

@hemildesai hemildesai commented Jul 17, 2023 •
edited
Loading

Changes:

  1. Remove prompt asking for confirmation to override ~/.kube/config in sky local up since kind does not override the kube config (it just adds new entries to the clusters, users and contexts list)
  2. Set the first available context as current-context in kube config after deletion of kind cluster in sky local down. Upon deletion, kind sets the current-context field to empty in the kube config, which causes kubectl commands to fail. So we set a previous context (if available).

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
    • sky local up with missing ~/.kube/config
    • sky local up with empty ~/.kube/config
    • sky local down with no other contexts in ~/.kube/config
    • sky local down with available contexts in ~/.kube/config
  • 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: bash tests/backward_comaptibility_tests.sh

@hemildesai hemildesai force-pushed the hd/sky-local-kubeconfig-mgmt branch from ce105b9 to 9aae657 Compare July 17, 2023 01:50
romilbhardwaj
romilbhardwaj reviewed Jul 17, 2023
View reviewed changes
Copy link
Collaborator

@romilbhardwaj romilbhardwaj left a comment

Choose a reason for hiding this comment

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

Great work @hemildesai! Thanks, and welcome to SkyPilot! 🚀

sky/cli.py Outdated
Comment on lines 4422 to 4430
# Check if ~/.kube/config exists:
if os.path.exists(os.path.expanduser('~/.kube/config')):
# Check if kubeconfig is valid, `kind delete` leaves an empty kubeconfig
valid, reason = kubernetes_utils.check_credentials()
if valid or (not valid and 'Invalid configuration' not in reason):
# Could be a valid kubeconfig or a non-empty but non-functioning
# kubeconfig - check if user wants to overwrite it
prompt = 'Cluster config found at ~/.kube/config. Overwrite it?'
click.confirm(prompt, default=True, abort=True, show_default=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If a context != kind-skypilot is already set, can we print a message saying that we will automatically switch kubectl context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Valid point, I will change the script to prevent switching current context if it's not the SkyPilot context. Wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I think it's okay to automatically switch context, just that we should let the user know that we are going to switch contexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, will update 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 0b10ef9

@hemildesai hemildesai force-pushed the hd/sky-local-kubeconfig-mgmt branch from d2f1fb7 to 0b10ef9 Compare July 18, 2023 01:52
romilbhardwaj
romilbhardwaj approved these changes Jul 18, 2023
View reviewed changes
Copy link
Collaborator

@romilbhardwaj romilbhardwaj left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @hemildesai!

sky/skylet/providers/kubernetes/utils.py Outdated
@@ -66,3 +66,18 @@ def get_cluster_status(cluster_name: str,
cluster_status.append(status_lib.ClusterStatus.INIT)
# If pods are not found, we don't add them to the return list
return cluster_status


def get_current_kube_config_context() -> Union[str, None]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: we use Optional[str] instead of Union[str, None] in our codebase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Updated in ef01c15

@romilbhardwaj
Copy link
Collaborator

Awesome, thanks @hemildesai! Merging now.

@romilbhardwaj romilbhardwaj merged commit 4b54555 into skypilot-org:k8s_cloud Jul 19, 2023
romilbhardwaj added a commit that referenced this pull request Aug 2, 2023
@romilbhardwaj @aviweit @hemildesai
[k8s] Kubernetes support (#2096)
4045cf3 
* Working Ray K8s node provider based on SSH

* wip

* working provisioning with SkyPilot and ssh config

* working provisioning with SkyPilot and ssh config

* Updates to master

* ray2.3

* Clean up docs

* multiarch build

* hacking around ray start

* more port fixes

* fix up default instance selection

* fix resource selection

* Add provisioning timeout by checking if pods are ready

* Working mounting

* Remove catalog

* fixes

* fixes

* Fix ssh-key auth to create unique secrets

* Fix for ContainerCreating timeout

* Fix head node ssh port caching

* mypy

* lint

* fix ports

* typo

* cleanup

* cleanup

* wip

* Update setup

* readme updates

* lint

* Fix failover

* Fix failover

* optimize setup

* Fix sync down logs for k8s

* test wip

* instance name parsing wip

* Fix instance name parsing

* Merge fixes for query_status

* [k8s_cloud] Delete k8s service resources. (#2105)

Delete k8s service resources.

- 'sky down' for Kubernetes cloud to remove cluster service resources.

* Status refresh WIP

* refactor to kubernetes adaptor

* tests wip

* clean up auth

* wip tests

* cli

* cli

* sky local up/down cli

* cli

* lint

* lint

* lint

* Speed up kind cluster creation

* tests

* lint

* tests

* handling for non-reachable clusters

* Invalid kubeconfig handling

* Timeout for sky check

* code cleanup

* lint

* Do not raise error if GPUs requested, return empty list

* Address comments

* comments

* lint

* Remove public key upload

* add shebang

* comments

* change permissions

* remove chmod

* merge 2241

* add todo

* Handle kube config management for sky local commands (#2253)

* Set current-context (if availablee) after sky local down and remove incorrect prompt in sky local up

* Warn user of kubeconfig context switch during sky local up

* Use Optional instead of Union

* Switch context in create_cluster if cluster already exists.

* fix typo

* update sky check error msg after sky local down

* lint

* update timeout check

* fix import error

* Fix kube API access from within cluster (load_incluster_auth)

* lint

* lint

* working autodown and sky status -r

* lint

* add test_kubernetes_autodown

* lint

* address comments

* address comments

* lint

* deletion timeouts wip

* [k8s_cloud] Ray pod not created under current context namespace. (#2302)

'namespace' exists under 'context' key.

* head ssh port namespace fix

* [k8s-cloud] Typo in sky local --help. (#2308)

Typo.

* [k8s-cloud] Set build_image.sh to be executable. (#2307)

* Set build_image.sh to be executable.

* Use TAG to easily switch between registries.

* remove ingress

* remove debug statements

* UX and readme updates

* lint

* fix logging for 409 retry

* lint

* lint

* comments

* remove k8s from default clouds to run

---------

Co-authored-by: Avi Weit <weit@il.ibm.com>
Co-authored-by: Hemil Desai <hemil.desai10@gmail.com>
romilbhardwaj added a commit that referenced this pull request Aug 25, 2023
@romilbhardwaj @aviweit @hemildesai
[k8s] GPU Support and fractional resources for Kubernetes (#2328)
469a62d 
* Working Ray K8s node provider based on SSH

* wip

* working provisioning with SkyPilot and ssh config

* working provisioning with SkyPilot and ssh config

* Updates to master

* ray2.3

* Clean up docs

* multiarch build

* hacking around ray start

* more port fixes

* fix up default instance selection

* fix resource selection

* Add provisioning timeout by checking if pods are ready

* Working mounting

* Remove catalog

* fixes

* fixes

* Fix ssh-key auth to create unique secrets

* Fix for ContainerCreating timeout

* Fix head node ssh port caching

* mypy

* lint

* fix ports

* typo

* cleanup

* cleanup

* wip

* Update setup

* readme updates

* lint

* Fix failover

* Fix failover

* optimize setup

* Fix sync down logs for k8s

* test wip

* instance name parsing wip

* Fix instance name parsing

* Merge fixes for query_status

* [k8s_cloud] Delete k8s service resources. (#2105)

Delete k8s service resources.

- 'sky down' for Kubernetes cloud to remove cluster service resources.

* Status refresh WIP

* refactor to kubernetes adaptor

* tests wip

* clean up auth

* wip tests

* cli

* cli

* sky local up/down cli

* cli

* lint

* lint

* lint

* Speed up kind cluster creation

* tests

* lint

* tests

* handling for non-reachable clusters

* Invalid kubeconfig handling

* Timeout for sky check

* code cleanup

* lint

* Do not raise error if GPUs requested, return empty list

* Address comments

* comments

* lint

* Remove public key upload

* GPU support init

* wip

* add shebang

* comments

* change permissions

* remove chmod

* merge 2241

* add todo

* Handle kube config management for sky local commands (#2253)

* Set current-context (if availablee) after sky local down and remove incorrect prompt in sky local up

* Warn user of kubeconfig context switch during sky local up

* Use Optional instead of Union

* Switch context in create_cluster if cluster already exists.

* fix typo

* update sky check error msg after sky local down

* lint

* update timeout check

* fix import error

* Fix kube API access from within cluster (load_incluster_auth)

* lint

* lint

* working autodown and sky status -r

* lint

* add test_kubernetes_autodown

* lint

* address comments

* address comments

* lint

* deletion timeouts wip

* [k8s_cloud] Ray pod not created under current context namespace. (#2302)

'namespace' exists under 'context' key.

* head ssh port namespace fix

* [k8s-cloud] Typo in sky local --help. (#2308)

Typo.

* [k8s-cloud] Set build_image.sh to be executable. (#2307)

* Set build_image.sh to be executable.

* Use TAG to easily switch between registries.

* remove ingress

* remove debug statements

* UX and readme updates

* lint

* fix logging for 409 retry

* lint

* lint

* Debug dockerfile

* wip

* Fix GPU image

* Query cloud specific env vars in task setup (#2347)

* Query cloud specific env vars in task setup

* Make query_env_vars specific to Kubernetes cloud

* Address PR comments

* working GPU type selection for GKE and EKS. GFD needs work.

* TODO for auto-detection

* Add image toggling for CPU/GPU

* Add image toggling for CPU/GPU

* Fix none acce_type

* remove memory from j2

* Make resnet examples run again

* lint

* v100 readme

* dockerfile and smoketest

* fractional cpu and mem

* nits

* refactor utils

* lint and cleanup

* lint and cleanup

* lint and cleanup

* lint and cleanup

* lint and cleanup

* lint and cleanup

* lint

* lint

* manual lint

* manual isort

* test readme update

* Remove EKS

* lint

* add gpu labeler

* updates

* lint

* update script

* ux

* fix formatter

* test update

* test update

* fix test_optimizer_dryruns

* docs

* cleanup

* test readme update

* lint

* lint

* Update imagepullpolicy to always

* update image build

* typing hints

* update docstr

* some comments

* refactor

* refactor

* lint

* lint

* update gke cmd

* update monkeypatch

* yapf

* comments

* increase default mem when GPU task

* increase default mem when GPU task

* fix test_optimize_speed

* lint

* Add CPU+Mem based early filtering and better debug logging

* lint

* fix test_optimizer

* fixes

* fix k8s port fetch logic

* clean up instance make logic

* increase default mem when GPU task

* lint

* clean up fit check logic

* add catalog todo

* eksctl update

* update readme and gpu_labeler with comments

* monkeypatch in enable_all_clouds

* change to T4

---------

Co-authored-by: Avi Weit <weit@il.ibm.com>
Co-authored-by: Hemil Desai <hemil.desai10@gmail.com>
romilbhardwaj added a commit that referenced this pull request Sep 16, 2023
@romilbhardwaj @aviweit
@hemildesai @landscapepainter
[K8s] Zero config networking for Kubernetes (#2500)
f0d3dfc 
* Working Ray K8s node provider based on SSH

* wip

* working provisioning with SkyPilot and ssh config

* working provisioning with SkyPilot and ssh config

* Updates to master

* ray2.3

* Clean up docs

* multiarch build

* hacking around ray start

* more port fixes

* fix up default instance selection

* fix resource selection

* Add provisioning timeout by checking if pods are ready

* Working mounting

* Remove catalog

* fixes

* fixes

* Fix ssh-key auth to create unique secrets

* Fix for ContainerCreating timeout

* Fix head node ssh port caching

* mypy

* lint

* fix ports

* typo

* cleanup

* cleanup

* wip

* Update setup

* readme updates

* lint

* Fix failover

* Fix failover

* optimize setup

* Fix sync down logs for k8s

* test wip

* instance name parsing wip

* Fix instance name parsing

* Merge fixes for query_status

* [k8s_cloud] Delete k8s service resources. (#2105)

Delete k8s service resources.

- 'sky down' for Kubernetes cloud to remove cluster service resources.

* Status refresh WIP

* refactor to kubernetes adaptor

* tests wip

* clean up auth

* wip tests

* cli

* cli

* sky local up/down cli

* cli

* lint

* lint

* lint

* Speed up kind cluster creation

* tests

* lint

* tests

* handling for non-reachable clusters

* Invalid kubeconfig handling

* Timeout for sky check

* code cleanup

* lint

* Do not raise error if GPUs requested, return empty list

* Address comments

* comments

* lint

* Remove public key upload

* GPU support init

* wip

* add shebang

* comments

* change permissions

* remove chmod

* merge 2241

* add todo

* Handle kube config management for sky local commands (#2253)

* Set current-context (if availablee) after sky local down and remove incorrect prompt in sky local up

* Warn user of kubeconfig context switch during sky local up

* Use Optional instead of Union

* Switch context in create_cluster if cluster already exists.

* fix typo

* update sky check error msg after sky local down

* lint

* update timeout check

* fix import error

* Fix kube API access from within cluster (load_incluster_auth)

* lint

* lint

* working autodown and sky status -r

* lint

* add test_kubernetes_autodown

* lint

* address comments

* address comments

* lint

* deletion timeouts wip

* [k8s_cloud] Ray pod not created under current context namespace. (#2302)

'namespace' exists under 'context' key.

* head ssh port namespace fix

* [k8s-cloud] Typo in sky local --help. (#2308)

Typo.

* [k8s-cloud] Set build_image.sh to be executable. (#2307)

* Set build_image.sh to be executable.

* Use TAG to easily switch between registries.

* remove ingress

* remove debug statements

* UX and readme updates

* lint

* fix logging for 409 retry

* lint

* lint

* Debug dockerfile

* wip

* Fix GPU image

* Query cloud specific env vars in task setup (#2347)

* Query cloud specific env vars in task setup

* Make query_env_vars specific to Kubernetes cloud

* Address PR comments

* working GPU type selection for GKE and EKS. GFD needs work.

* TODO for auto-detection

* Add image toggling for CPU/GPU

* Add image toggling for CPU/GPU

* Fix none acce_type

* remove memory from j2

* Make resnet examples run again

* lint

* v100 readme

* dockerfile and smoketest

* fractional cpu and mem

* nits

* refactor utils

* lint and cleanup

* lint and cleanup

* lint and cleanup

* lint and cleanup

* lint and cleanup

* lint and cleanup

* lint

* lint

* manual lint

* manual isort

* test readme update

* Remove EKS

* lint

* add gpu labeler

* updates

* lint

* update script

* ux

* fix formatter

* test update

* test update

* fix test_optimizer_dryruns

* docs

* cleanup

* test readme update

* lint

* lint

* [k8s_cloud_beta1] Add sshjump host support. (#2369)

* Update build image

* fix image path

* fix merge

* cleanup

* lint

* fix utils ref

* typo

* refactor pod creation

* lint

* merge fixes

* portfix

* merge fixes

* [k8s_cloud_beta1] Sky down for a cluster deployed in Kubernetes to possibly remove sshjump pod. (#2425)

* Sky down for a kubernetes cluster to possibly terminate sshjump pod.

- If the related sshjump pod is being reported as its main container
  not have been started, then remove its pod and service. This is to
  minimize the chances for remaining with dangling sshjump pod.

* Remove sshjump service in case of an failure to analyze sshjump.

- remove _request_timeout as it might not be needed due to
  terminationGracePeriodSeconds being set in sshjump template.

* Move sshjump analysis to kubernetes_utils.

* Apply changes per ./format.sh.

* Minor comment rephrase.

* Use sshjump_name from ray pod label.

- rather than from clouds.Kubernetes

* cleanup

* Add networking benchmarks

* comment

* comment

* lint

* autodown fixes

* lint

* fix label

* [k8s_cloud_beta1] Adding support for ssh using kubectl port-forward to access k8s instance (#2412)

* Add sshjump support.

* Update lcm script.

- add comments
- rename variables
- typo

* Set imagePullPolicy to IfNotPresent.

* add support for port-forward

* remove unused

* comments

* Disable ControlMaster for ssh_options_list

* nit

* update to disable rest of the ControlMaster

* command runner rsync update

* relocating run_on_k8s

* relocate run_on_k8s

* Make Kubernetes specific env variables available when joining a cluster via SSH

* merge k8s_cloud_beta1

* format

* remove redundant utils.py

* format and comments

* update with proxy_to_k8s

* Update sky/authentication.py

Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>

* resolving comments on structures

* Update sky/utils/command_runner.py

Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>

* document on nodeport/port-forward proxycommand

* error handling when socat is not installed

* removing KUBECONFIG from port-forward shell script

* nit

* nit

* Add suport for nodeport

* Update sky/utils/kubernetes_utils.py

Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>

* update

* switch svc when conflicting jump pod svc exist

* format

* Update sky/utils/kubernetes_utils.py

Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>

* refactoring check for socat

* resolve comments

* add ServiceType enum and port-forward proxy script

* update k8s env var access

* add check for container status remove unused func

* nit

* update get_external_ip for portforward mode

* conditionally use sudo and quote values of env var

---------

Co-authored-by: Avi Weit <weit@il.ibm.com>
Co-authored-by: hemildesai <hemil.desai10@gmail.com>
Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>

* refactor

* fix

* updates

* lint

* Update sky/skylet/providers/kubernetes/node_provider.py

* fix test

* [k8s] Showing reasons for provisioning failure in K8s (#2422)

* surface provision failure message

* nit

* nit

* format

* nit

* CPU message fix

* update Insufficient memory handling

* nit

* nit

* Update sky/skylet/providers/kubernetes/node_provider.py

Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>

* Update sky/skylet/providers/kubernetes/node_provider.py

Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>

* Update sky/skylet/providers/kubernetes/node_provider.py

Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>

* Update sky/skylet/providers/kubernetes/node_provider.py

Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>

* format

* update gpu failure message and condition

* fix GPU handling cases

* fix

* comment

* nit

* add try except block with general error handling

---------

Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>

* cleanup

* lint

* fix for ssh jump image_id

* comments

* ssh jump refactor

* lint

* image build fixes

---------

Co-authored-by: Avi Weit <weit@il.ibm.com>
Co-authored-by: Hemil Desai <hemil.desai10@gmail.com>
Co-authored-by: Doyoung Kim <34902420+landscapepainter@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romilbhardwaj romilbhardwaj romilbhardwaj 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.
2 participants
@hemildesai @romilbhardwaj