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] Add Ray Job output - start/end time and ray cluster name #2533

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

han-steve
Copy link
Contributor

Why are these changes needed?

Adding the starttime, endtime, and the rayclustername fields to the api server output to match the status fields on the rayjob CRD.

These fields are useful for displaying the job start/end time information and easily pulling the cluster information. For us, they are also useful for generating the grafana links to show the Prometheus metrics collected during the job's run.

Checks

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

@han-steve
Copy link
Contributor Author

han-steve commented Nov 12, 2024

/assign @YQ-Wang (how do I assign people?)

@@ -789,7 +789,7 @@ func RegisterImageTemplateServiceHandlerServer(ctx context.Context, mux *runtime
// RegisterComputeTemplateServiceHandlerFromEndpoint is same as RegisterComputeTemplateServiceHandler but
// automatically dials to "endpoint" and closes the connection when "ctx" gets done.
func RegisterComputeTemplateServiceHandlerFromEndpoint(ctx context.Context, mux *runtime.ServeMux, endpoint string, opts []grpc.DialOption) (err error) {
conn, err := grpc.NewClient(endpoint, opts...)
conn, err := grpc.Dial(endpoint, opts...)
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed this change last time as well. I manually put it back to the original code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ok... Wonder why the generated pb is inconsistent.

@YQ-Wang
Copy link
Contributor

YQ-Wang commented Nov 12, 2024

LGTM

@YQ-Wang
Copy link
Contributor

YQ-Wang commented Nov 12, 2024

cc @kevin85421 @andrewsykim

@kevin85421
Copy link
Member

We are currently in KubeCon. I have already approved the CI. I will merge it if CI passes because @YQ-Wang has already approved this API server PR.

@kevin85421 kevin85421 self-assigned this Nov 13, 2024
@kevin85421
Copy link
Member

@han-steve would you mind taking a look at the lint error? Thanks!

@han-steve
Copy link
Contributor Author

Yep!

@kevin85421
Copy link
Member

@han-steve would you mind fixing the conflict? Thanks!

Signed-off-by: Steve Han <36038610+han-steve@users.noreply.github.com>
@han-steve
Copy link
Contributor Author

Done :)

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