Add kubectl ray cluster log command

[chiayi]

Why are these changes needed?

This PR adds the kubectl ray cluster logs command to retrieve the head node logs for a ray cluster.

Related issue number

Checks

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

[MortalHappiness]

You forget to handle the case that --out-dir is not provided by the user. It should be handled in the Complete function above.

[chiayi]

If the --out-dir flag is not set by the user, it will fail the proceeding validation giving a no such file or directory error. I don't think thats a good error msg for the user so I will be adding another check that returns something along the lines of "No output directory, please set --out-dir flag". As for handling this case in the Complete function, I think we should keep all the validation should be kept in the Validation function. WDYT?

[MortalHappiness]

I borrow the idea from kubectl source code.

https://github.com/kubernetes/kubectl/blob/b2a7d2626dc56b2edf952ab4031fcc20a7f441e4/pkg/cmd/logs/logs.go#L233-L236

Here the Pod name is required and it is handled in the Complete function instead of Validate function.

[chiayi]

As discussed offline, --out-dir flag will not be required and will default to current directory with the head name being the destination directory name.

With that, I think it make sense to do the validation check in the Validation method. It seems like the check in the kubectl source code is checking for completion? WDYT?

[andrewsykim]

@chiayi can you share the output of running the kubectl ray cluster logs command?

[chiayi]

This is what it looks like when it downloads the file. Please let me know if I should add more logs.

Result:

[andrewsykim]

@chiayi in your last screenshot, why is there two directories named <pod-name> and <pod-name>-logs?

[chiayi]

One of the them (stable-diffusion-cluster-head-hqcxt) isn't a directory and is the pod logs. and another one is the directory that contains all the files from /tmp/ray/session_latest/logs/.

[chiayi]

I'm going to rename the pod log one to include pod-logs. Let me know if there is something else I should as well. @andrewsykim

[andrewsykim]

I suggest having just one directory per pod named after the pod name and just add a stdout.log file in there

[chiayi]

New output directory format:

[chiayi]

Current flow:

Out-dir is no longer required and will download to current dir if not specified.
Syntax changed to kubectl ray logs CLUSTERNAME

[andrewsykim]

Out-dir is no longer required and will download to current dir if not specified.

Instead of defaulting to current-dir, can we default to ./<cluster-name> ?

Otherwise LGTM!

[chiayi]

[andrewsykim]

FYI, the lint failure is fixed in #2366

[andrewsykim]

LGTM

[kevin85421]

@chiayi would you mind fixing the linter issue? Thanks!

[chiayi]

@kevin85421 I merged in the pervious linter fix, the current linter issues are being fixed in #2368.

[kevin85421]

I will merge the PR after CI passes because Andrew has already approved the PR.