SUBMARINE-442. Support get job's log in submarine-server REST API #263

JohnTing · 2020-04-15T14:31:20Z

What is this PR for?

Now we have the "jobs" resource in REST which can do CRUD. We also need a "logs" API to get the job's log output. The URI could be "api/v1/logs"
It should accept parameters like "jobid". Initially, the logs could be aggregated logs of all containers.
Streaming is preferred so that the python client can enable a fancy way for the end-user to check logs

What type of PR is it?

Feature

Todos

- get logs so far

What is the Jira issue?

https://issues.apache.org/jira/projects/SUBMARINE/issues/SUBMARINE-442

How should this be tested?

Create a job
Visit /api/v1/logs or /api/v1/logs/{jobid} with a browser

Screenshots (if appropriate)

http://127.0.0.1:8080/api/v1/jobs/logs

http://127.0.0.1:8080/api/v1/jobs/logs/job_1587481945001_0001

Questions:

Does the licenses files need update? No
Is there breaking changes for older versions? No
Does this needs documentation? No

tangzhankun

Thanks a lot for the contribution! @JohnTing Please see my comments below:

tangzhankun · 2020-04-16T04:11:03Z

...r/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sJobSubmitter.java

+          getJobLabelSelector(job), null, null,
+          null, null);
+
+      final V1Pod pod = podList.getItems().get(0);


It would be several containers in the "podList", here it only gets the first one. Is it better that we merge all container's pod with a format? Like this:
{
jobId: job_123,
jobType: tensorflow,
LogContent: [
{podName: tf-ps-0,
podLog: 123123123},
{podName: tf-worker-0,
podLog: 456456456},
...
]
}

Ok, I will modify it

tangzhankun · 2020-04-16T04:14:05Z

...r/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sJobSubmitter.java

+          "false", null, null,
+          getJobLabelSelector(job), null, null,
+          null, null);
+      final V1Pod pod = podList.getItems().get(0);


Also, is it possible that we merge different streams into one? And update the aggregated log stream?

I am not sure how to show the mixed stream in the browser ...
Maybe something like this?
[
{"pod1": "log start"},
{"pod1": "1%"},
{"pod2": "log start"},
{"pod1": "2%"},
{"pod2": "1%"},
....

tangzhankun · 2020-04-16T04:17:29Z

...ine-server/server-core/src/test/java/org/apache/submarine/server/rest/JobLogRestApiTest.java

+    String jobId = "job_1586957273819_0001";
+    GetMethod response = httpGet("/api/" + RestConstants.V1 + "/"
+        + RestConstants.LOGS + "/" + jobId);
+    LOG.info(response.toString());


Can we have some assertions in this test case? Usually, if it could run some job and get the job log would be easy. If we cannot do any assertion here. I suggest we remove this end-to-end unit test.

Ok, I will add it

pingsutw · 2020-04-16T06:45:00Z

...r/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sJobSubmitter.java

  // K8s API client for CRD
  private CustomObjectsApi api;

+  ApiClient client;


Suggested change

ApiClient client;

private ApiClient client;

pingsutw · 2020-04-16T06:48:42Z

...r/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sJobSubmitter.java

+            null,
+            Boolean.FALSE,
+            Integer.MAX_VALUE,
+            40,


We should use a constant variable instead of hardcode 40.

pingsutw · 2020-04-16T06:50:53Z

...r/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sJobSubmitter.java

+      return inputStream;
+
+    } catch (final ApiException e) {
+      LOG.warn("Error when listing pod for job:" + job.toString(), e.getMessage());


Suggested change

LOG.warn("Error when listing pod for job:" + job.toString(), e.getMessage());

LOG.error("Error when listing pod for job:" + job.toString(), e.getMessage());

pingsutw · 2020-04-16T06:51:13Z

...r/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sJobSubmitter.java

+    } catch (final ApiException e) {
+      LOG.warn("Error when listing pod for job:" + job.toString(), e.getMessage());
+    } catch (final IOException e) {
+      LOG.warn("Error when get pod log stream", e.getMessage());


Suggested change

LOG.warn("Error when get pod log stream", e.getMessage());

LOG.error("Error when get pod log stream", e.getMessage());

jiwq

Thanks @JohnTing for the contribution.

Considering the new REST is related to job, so we'd better rebase it on /api/v1/jobs/{id}/logs. If it accepted the JobLogsRestApi's content should move into JobManagerRestApi.
In the REST controller layer, we'd better use the JobManager rather than use submitter directly.
ApiClient would not need to make it to class member, because the CoreV1Api's non argument constructor can set the client from Configuration which had set in the client instance stage.
Should add the integration test in submarine-test/test-k8s and the unit test should better removed if the UT can't mock the process.
The JobLogHandler can be deleted, because it owned to the submitter.

If I understand wrong, please correct me. Thanks again for your contribution!

pingsutw · 2020-04-17T13:47:22Z

submarine-server/server-api/src/main/java/org/apache/submarine/server/api/job/JobLog.java

+
+package org.apache.submarine.server.api.job;
+
+// import java.io.InputStream;


Need to remove unused import

JohnTing · 2020-04-17T18:56:56Z

@tangzhankun, @jiwq, @pingsutw
Thank you for the advice.
I have updated done. Please help me preview this PR, thanks a lot.

yuanzac · 2020-04-18T10:28:40Z

Trigger the travis ci again.

xunliu

@JohnTing Thank you contribution this feature, very cool!

xunliu · 2020-04-18T12:37:17Z

...ine-server/server-core/src/main/java/org/apache/submarine/server/rest/JobManagerRestApi.java

+
+  @GET
+  @Path("/logs")
+  public Response listlog(@QueryParam("status") String status) {


The hump command method should be used.
Need change to listLog(...

xunliu · 2020-04-18T12:37:28Z

...ine-server/server-core/src/main/java/org/apache/submarine/server/rest/JobManagerRestApi.java

+
+  @GET
+  @Path("/logs/{id}")
+  public Response getlog(@PathParam(RestConstants.JOB_ID) String id) {


The hump command method should be used.
Need change to getLog(...

xunliu · 2020-04-18T12:45:03Z

...r/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sJobSubmitter.java

  // K8s API client for CRD
  private CustomObjectsApi api;

+  private CoreV1Api COREV1_API;


The hump command method should be used.
Need change to coreApi

xunliu · 2020-04-18T12:53:47Z

@JohnTing You need to add test cases to ensure the correctness of your code. It can also ensure that the code you submit will not be modified by others.
like this, https://github.com/apache/submarine/blob/master/submarine-test/test-k8s/src/test/java/org/apache/submarine/rest/JobManagerRestApiIT.java#L134

xunliu · 2020-04-23T04:17:19Z

@JohnTing This branch has conflicts, please rebase this commits.

xunliu

@JohnTing Thank you contribution this feature.
Will merge if no more comments.

tangzhankun reviewed Apr 16, 2020

View reviewed changes

pingsutw reviewed Apr 16, 2020

View reviewed changes

jiwq reviewed Apr 16, 2020

View reviewed changes

JohnTing force-pushed the SUBMARINE-442 branch from 66c66f0 to 0f85849 Compare April 17, 2020 09:09

pingsutw reviewed Apr 17, 2020

View reviewed changes

xunliu reviewed Apr 18, 2020

View reviewed changes

JohnTing force-pushed the SUBMARINE-442 branch from 058c259 to de45aab Compare April 20, 2020 06:03

SUBMARINE-442

319222c

JohnTing force-pushed the SUBMARINE-442 branch from 7d54e52 to 319222c Compare April 23, 2020 08:51

JohnTing added 2 commits April 23, 2020 21:11

test12

1bb806f

test12

7195a77

xunliu approved these changes Apr 28, 2020

View reviewed changes

asfgit closed this in 87b2296 Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SUBMARINE-442. Support get job's log in submarine-server REST API #263

SUBMARINE-442. Support get job's log in submarine-server REST API #263

JohnTing commented Apr 15, 2020 •

edited

tangzhankun left a comment

tangzhankun Apr 16, 2020

JohnTing Apr 16, 2020

tangzhankun Apr 16, 2020

JohnTing Apr 16, 2020 •

edited

tangzhankun Apr 16, 2020

JohnTing Apr 16, 2020

pingsutw Apr 16, 2020

pingsutw Apr 16, 2020 •

edited

pingsutw Apr 16, 2020

pingsutw Apr 16, 2020

jiwq left a comment

pingsutw Apr 17, 2020

JohnTing commented Apr 17, 2020

yuanzac commented Apr 18, 2020

xunliu left a comment

xunliu Apr 18, 2020

xunliu Apr 18, 2020

xunliu Apr 18, 2020

xunliu commented Apr 18, 2020

xunliu commented Apr 23, 2020

xunliu left a comment

	LOG.warn("Error when listing pod for job:" + job.toString(), e.getMessage());
	LOG.error("Error when listing pod for job:" + job.toString(), e.getMessage());

	LOG.warn("Error when get pod log stream", e.getMessage());
	LOG.error("Error when get pod log stream", e.getMessage());


		package org.apache.submarine.server.api.job;

		// import java.io.InputStream;

SUBMARINE-442. Support get job's log in submarine-server REST API #263

SUBMARINE-442. Support get job's log in submarine-server REST API #263

Conversation

JohnTing commented Apr 15, 2020 • edited

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

tangzhankun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnTing Apr 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pingsutw Apr 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiwq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnTing commented Apr 17, 2020

yuanzac commented Apr 18, 2020

xunliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu commented Apr 18, 2020

xunliu commented Apr 23, 2020

xunliu left a comment

Choose a reason for hiding this comment

JohnTing commented Apr 15, 2020 •

edited

JohnTing Apr 16, 2020 •

edited

pingsutw Apr 16, 2020 •

edited