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

Add namespace scope to compute template operations #244

Merged
merged 8 commits into from
May 19, 2022
Merged

Add namespace scope to compute template operations #244

merged 8 commits into from
May 19, 2022

Conversation

daikeshi
Copy link
Contributor

@daikeshi daikeshi commented Apr 29, 2022 •
edited
Loading

Why are these changes needed?

This PR adds the namespace scope to get/list/delete for compute templates.

  • Added a namespace field to compute template
  • Added a namespace field to compute template get/list/delete request
  • Added a namespace option to cli for compute templates
  • If namespace is not provided, it will use ray-system as the default

*** Update 1 ***
To follow the decisions made in #237, following changes are also included in this PR:

  • Make namespace a required parameter for compute template operations
  • Bump corresponding apiserver API version to v1alpha2
  • Updated the doc

*** Update 2 ***

  • Add an endpoint to list all compute templates in all namespaces

Related issue number

Closes #238

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

@daikeshi daikeshi changed the title Keshi/add template namespace Add template namespace May 3, 2022
@daikeshi daikeshi changed the title Add template namespace Add namespace scope to compute template operations May 3, 2022
@daikeshi daikeshi force-pushed the keshi/add-template-namespace branch from 0f52403 to 371ca46 Compare May 12, 2022 20:17
@daikeshi daikeshi force-pushed the keshi/add-template-namespace branch from 371ca46 to 7eb2ede Compare May 13, 2022 02:55
@daikeshi
Copy link
Contributor Author

daikeshi commented May 13, 2022 •
edited
Loading

@Jeffwan This PR is ready for review now. It has been updated based on the changes I did in #237

daikeshi
daikeshi commented May 13, 2022
View reviewed changes
proto/config.proto
@@ -151,30 +172,41 @@ message ListImageTemplatesResponse {
repeated ImageTemplate image_templates = 1;
}

message ListAllImageTemplatesRequest {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also made the changes to the Image template, but let me know if this is not necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is ok

@Jeffwan
Copy link
Collaborator

Jeffwan commented May 19, 2022

The downside is we may introduce namespace (kubernetes domain concept) to generic configs like compute templates which should be simple and clean. Seems we don't have other ways at this moment. Once we supports DB, I think these values can be moved outside kubernetes.

The other thing is whether we should use different folder for v1alpha1 and v1alpha2 for PB definitions? This is out of scope of this story and we can have some follow up discussion later.

@Jeffwan
Copy link
Collaborator

Jeffwan commented May 19, 2022

Thanks for the contribution. Everything looks good to me and let's merge the change

@Jeffwan Jeffwan merged commit 9dbff52 into ray-project:master May 19, 2022
@daikeshi
Copy link
Contributor Author

The downside is we may introduce namespace (kubernetes domain concept) to generic configs like compute templates which should be simple and clean. Seems we don't have other ways at this moment. Once we supports DB, I think these values can be moved outside kubernetes.

I see your points. Yeah, I agree. We can address this when we start adopting DB to store configs.

The other thing is whether we should use different folder for v1alpha1 and v1alpha2 for PB definitions? This is out of scope of this story and we can have some follow up discussion later.

That's a good point. I guess it depends on if we would like to support multiple API versions at the same time. We can discuss more on this.

lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
@daikeshi
Add namespace scope to compute template operations (ray-project#244)
768eb69 
* Update protos to include namespace for template configs

* Add namespace scope to compute template operations

* Add namespace to compute template cli

* Fix proto

* Fix api server

* Fix cli

* Update docs

* Add ListAllComputeTemplates endpoint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jeffwan Jeffwan Jeffwan 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.

[Feature] Add namespace scope to compute template configs
2 participants
@daikeshi @Jeffwan