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

[k8s] Fix --purge not cleaning up cluster in stale k8s context #4514

Merged
merged 6 commits into from
Jan 6, 2025

Conversation

romilbhardwaj
Copy link
Collaborator

Fixes issue reported here: #2807 (comment)

sky down --purge now works again even with stale or invalid context.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Tested with sky local up; sky launch -c test --cloud kubernetes -- echo hi; mv ~/.kube/config ~/.kube/config.bak; sky down --purge test

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 @romilbhardwaj! Left a comment for discussion.

Comment on lines 4230 to 4231
raise RuntimeError(f'Instance {node_id} in unexpected '
'state {node_status}.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to have the purge check here instead, so that we can correctly print out some warnings for the clusters not correctly cleaned up even when --purge is specified.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yeah good point, updated the purge condition checks

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.

Nice! Thanks @romilbhardwaj! LGTM

Copy link
Collaborator

@cg505 cg505 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 fix, Romil!

@romilbhardwaj romilbhardwaj merged commit 59cb4e9 into master Jan 6, 2025
18 of 19 checks passed
@romilbhardwaj romilbhardwaj deleted the core_fix_purge branch January 6, 2025 22:10
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.

3 participants