-
Notifications
You must be signed in to change notification settings - Fork 470
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
[Core] Admin policy enforcement plugin #3966
Conversation
examples/policy/example_policy/example_policy/skypilot_policy.py
Outdated
Show resolved
Hide resolved
examples/policy/example_policy/example_policy/skypilot_policy.py
Outdated
Show resolved
Hide resolved
examples/policy/example_policy/example_policy/skypilot_policy.py
Outdated
Show resolved
Hide resolved
examples/policy/example_policy/example_policy/skypilot_policy.py
Outdated
Show resolved
Hide resolved
sky/serve/core.py
Outdated
@@ -124,6 +125,8 @@ def up( | |||
|
|||
_validate_service_task(task) | |||
|
|||
task = policy.Policy().apply_to_task(task) |
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.
A bit unclear if it's in-place since it also returns the task. How about "apply_in_place"?
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.
It is not in-place. Either way is fine, but I found not in-place is more commonly seen operations.
Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>
Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>
Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>
Co-authored-by: Zongheng Yang <zongheng.y@gmail.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.
LGTM @Michaelvll. I think two TODOs before merging:
(1) Smoke tests, since this touches config / backend
(2) Update PR description (policy
)
examples/admin_policy/example_policy/example_policy/skypilot_policy.py
Outdated
Show resolved
Hide resolved
examples/admin_policy/example_policy/example_policy/skypilot_policy.py
Outdated
Show resolved
Hide resolved
examples/admin_policy/example_policy/example_policy/skypilot_policy.py
Outdated
Show resolved
Hide resolved
sky/execution.py
Outdated
cluster_exists = existing_handle is not None | ||
cluster_record = global_user_state.get_cluster_from_name(cluster_name) | ||
cluster_exists = cluster_record is not None | ||
cluster_running = (cluster_record is not None and |
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.
Q: what's the latency impact of this policy on sky launch/exec
? Is it small? Do we expect users to use this example policy and be ok with the latency?
Co-authored-by: Zongheng Yang <zongheng.y@gmail.com>
Tested:
|
This PR allows the admin to bring a customized policy enforcement script in Python, which can apply any mutation to all user tasks based on customized policy requirements.
Usage:
User-side
In
~/.sky/config.yaml
:Admin-side
Create the
skypilot_policy_fn
function with the following signature:See
policy.py
for the definition of these two types.TODO:
exec
? (Let's apply for both for now)config.rst
labels
work in task.yaml without the requirement for specifyingcloud
#3965)Tested (run the relevant ones):
bash format.sh
pytest tests/test_smoke.py
pytest tests/test_smoke.py::test_fill_in_the_name
conda deactivate; bash -i tests/backward_compatibility_tests.sh