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

[api server] enable job spec server #416

Merged
merged 10 commits into from
Aug 3, 2022

Conversation

Basasuya
Copy link
Contributor

Why are these changes needed?

add job spec server implement for job server

Related issue number

#389

Checks

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

@Basasuya Basasuya force-pushed the basasuya_master_job_api_server branch from 01cbeb7 to 254e64f Compare July 26, 2022 13:14
@Jeffwan Jeffwan added this to the v0.4.0 release milestone Jul 26, 2022
@Jeffwan Jeffwan self-requested a review July 26, 2022 18:40
apiserver/cmd/main.go Outdated Show resolved Hide resolved
apiserver/pkg/client/job.go Outdated Show resolved Hide resolved
apiserver/pkg/manager/client_manager.go Outdated Show resolved Hide resolved
apiserver/pkg/manager/resource_manager.go Show resolved Hide resolved
apiserver/pkg/model/converter.go Outdated Show resolved Hide resolved
proto/job.proto Outdated Show resolved Hide resolved
apiserver/pkg/util/job.go Outdated Show resolved Hide resolved
apiserver/pkg/manager/resource_manager.go Show resolved Hide resolved
apiserver/pkg/manager/resource_manager.go Show resolved Hide resolved

func (s *RayJobServer) CreateRayJob(ctx context.Context, request *api.CreateRayJobRequest) (*api.RayJob, error) {
// use the namespace in the request to override the namespace in the job definition
request.Job.Namespace = request.Namespace
Copy link
Collaborator

Choose a reason for hiding this comment

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

for creation, let's do a validation first. check how raycluster did and we can raise some 400 issues.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Basasuya This is necessary to reduce error creation in cluster. Let's add some validation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after manual test, job create/list/delete would be OK. but after ray cluster create, seems job can not run through.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel that's a job configuration issue. It works on my side and let's discuss this issue offline.

@Jeffwan
Copy link
Collaborator

Jeffwan commented Jul 28, 2022

Let's run this command to fix the lint issue

goimports -w apiserver/pkg/model/converter.go

@Basasuya
Copy link
Contributor Author

Let's run this command to fix the lint issue

goimports -w apiserver/pkg/model/converter.go

@Jeffwan fixed

@Basasuya Basasuya changed the title [WIP] [api server] enable job spec server [api server] enable job spec server Jul 29, 2022
@Jeffwan
Copy link
Collaborator

Jeffwan commented Jul 29, 2022

Overall looks good to me. I will leave this change for a while. after v0.3.0 official release, we can merge it to avoid conflicts (fix on master can not be easily cherry-picked into v0.3.0)

@Jeffwan Jeffwan merged commit f2e689a into ray-project:master Aug 3, 2022
@scarlet25151 scarlet25151 mentioned this pull request Aug 19, 2022
4 tasks
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
* [api server] enable job spec server

* fix for lint

* fix for review

* fix

* fix for lint

* fix for review

* fix UT / add encoding for runtime env
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.

None yet

2 participants