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 mounting when launching from a service account #3532

Merged
merged 8 commits into from
May 10, 2024

Conversation

romilbhardwaj
Copy link
Collaborator

Closes #3530.

Adds the appropriate permissions to create the skypilot-system namespace and create resources in it.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Smoke tests - enabled test_managed_jobs_storage for kubernetes pytest -v tests/test_smoke.py::test_managed_jobs_storage --kubernetes

@concretevitamin concretevitamin requested review from Michaelvll, concretevitamin and cblmemo and removed request for concretevitamin May 10, 2024 16:07
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 fix @romilbhardwaj! Mostly looks good to me : )

Comment on lines 260 to 261
kubernetes.core_api().patch_namespaced_service_account(
name, namespace, account)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to check the config of the account first and only patch the service account if it does not have all the config?
Reason: previously, we only required read permission for the user's account, but now we require write permission as well. This is to make sure the following works: the admin create the service account for all users and create restrictive user accounts with only read permission (this is what we made sure for the other clouds). Wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! I've updated checks to patch only if the existing role does not match the required permissions.

sky/provision/kubernetes/config.py Show resolved Hide resolved
sky/provision/kubernetes/config.py Outdated Show resolved Hide resolved
sky/provision/kubernetes/config.py Outdated Show resolved Hide resolved
@romilbhardwaj
Copy link
Collaborator Author

romilbhardwaj commented May 10, 2024

Thanks @Michaelvll! Running smoke tests now:

  • Smoke tests - pytest tests/test_smoke.py --kubernetes --managed-jobs
  • Smoke tests -pytest tests/test_smoke.py --kubernetes --serve

@romilbhardwaj
Copy link
Collaborator Author

Tests pass, merging now!

@romilbhardwaj romilbhardwaj merged commit 8a0a34d into master May 10, 2024
20 checks passed
@romilbhardwaj romilbhardwaj deleted the k8s_sa_namespace_permissions_2 branch May 10, 2024 23:17
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.

[k8s] file mount fails when using job controller
2 participants