[Feature-13763][K8S Task] collect real-time log #14379

Radeity · 2023-06-19T18:15:31Z

Purpose of the pull request

This PR closes [Improvement][K8S Task] K8S task should collect whole pod log #13763

Brief change log

Use log watcher to collect real time, same with [Improvement-13751][Worker] Support real-time pod log collection #13752
Now, can only collect log of the first pod, have to consider in the future if one job launch multi pods.

Verify this pull request

Manually tested.

codecov-commenter · 2023-06-20T03:53:31Z

Codecov Report

Merging #14379 (fa7e57e) into dev (1d0d85b) will decrease coverage by 0.03%.
The diff coverage is 7.44%.

❗ Current head fa7e57e differs from pull request most recent head b019235. Consider uploading reports for the commit b019235 to get more accurate results

@@             Coverage Diff              @@
##                dev   #14379      +/-   ##
============================================
- Coverage     38.50%   38.48%   -0.03%     
- Complexity     4562     4564       +2     
============================================
  Files          1260     1260              
  Lines         43791    43828      +37     
  Branches       4829     4832       +3     
============================================
+ Hits          16861    16866       +5     
- Misses        25057    25089      +32     
  Partials       1873     1873

Impacted Files	Coverage Δ
...duler/plugin/task/api/AbstractCommandExecutor.java	`0.00% <0.00%> (ø)`
...ugin/task/api/am/KubernetesApplicationManager.java	`0.00% <0.00%> (ø)`
...scheduler/plugin/task/api/k8s/AbstractK8sTask.java	`0.00% <0.00%> (ø)`
...r/plugin/task/api/k8s/AbstractK8sTaskExecutor.java	`77.77% <ø> (+24.44%)`	⬆️
...nscheduler/plugin/task/api/utils/ProcessUtils.java	`0.00% <0.00%> (ø)`
...uler/plugin/task/api/k8s/impl/K8sTaskExecutor.java	`36.11% <13.20%> (-2.19%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Radeity · 2023-07-10T05:50:38Z

Hi, @caishunfeng , would you like to help review this PR?

Gallardot · 2023-07-10T11:53:45Z

HI @Radeity , good job!
I tested this PR. Sometimes it's OK, but sometimes there's an error log.

[INFO] 2023-07-10 19:42:57.004 +0800 - [K8sJobExecutor-echo-77] start to submit job
[INFO] 2023-07-10 19:42:57.073 +0800 - [K8sJobExecutor-echo-77] submitted job successfully
[INFO] 2023-07-10 19:42:57.095 +0800 - event received : job:echo-77 action:ADDED
[INFO] 2023-07-10 19:42:57.421 +0800 - event received : job:echo-77 action:MODIFIED
[INFO] 2023-07-10 19:42:57.421 +0800 - job echo-77 status 1
[INFO] 2023-07-10 19:43:02.459 +0800 - [K8S-pod-log] {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"container \"echo-77\" in pod \"echo-77-pngf7\" is waiting to start: ContainerCreating","reason":"BadRequest","code":400}
[INFO] 2023-07-10 19:43:25.109 +0800 - event received : job:echo-77 action:MODIFIED
[INFO] 2023-07-10 19:43:25.109 +0800 - job echo-77 status 0
[INFO] 2023-07-10 19:43:25.110 +0800 - [K8sJobExecutor-echo-77] succeed in k8s

Maybe some fault tolerance work is needed here. For example, before getting pod logs, check the status of the current task pod?

Radeity · 2023-07-10T12:08:14Z

Hi @Gallardot, thanks very much for your comment and test!

Maybe some fault tolerance work is needed here. For example, before getting pod logs, check the status of the current task pod?

Agree with you. I will add the status check to handle different pod status :D

Gallardot · 2023-07-10T12:36:26Z

...c/main/java/org/apache/dolphinscheduler/plugin/task/api/am/KubernetesApplicationManager.java

            return null;
        }
-        Pod driver = driverPod.get(0);
+        Pod pod = podList.get(0);


Not sure if there will be multiple Pods. Maybe you should filter the latest pods based on when they were created?

https://github.com/apache/dolphinscheduler/blob/dev/dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/am/KubernetesApplicationManager.java#L100-L110

When getting the pod list, maybe add jobname as a label filter?

Not sure if there will be multiple Pods. Maybe you should filter the latest pods based on when they were created?

You are right, one job can launch multiple pods. For this scenario, I think it's better support to read logs from different pods in future PR, rather read from the latest one, WDYT?

When getting the pod list, maybe add jobname as a label filter?

The label value here is taskAppId which can behave like a unique identifier of one job.

Got it. After the current PR merge, we create a new issue to track the issue

… Improvement-13763

Radeity · 2023-07-11T01:50:58Z

Hi, @Gallardot , I have added a check only for PENDING pod, do you have some other suggestions here?

dolphinscheduler/dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/am/KubernetesApplicationManager.java

Lines 195 to 199 in 2f9d915

    
           if (pod.getStatus().getPhase().equals(PENDING)) { 
        
               ThreadUtils.sleep(SLEEP_TIME_MILLIS); 
        
           } else { 
        
               podIsReady = true; 
        
           }

Gallardot · 2023-07-11T02:05:13Z

Hi, @Gallardot , I have added a check only for PENDING pod, do you have some other suggestions here?

dolphinscheduler/dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/am/KubernetesApplicationManager.java

Lines 195 to 199 in 2f9d915

if (pod.getStatus().getPhase().equals(PENDING)) {

ThreadUtils.sleep(SLEEP_TIME_MILLIS);

} else {

podIsReady = true;

}

I think we can also add UNKNOWN.

Gallardot

Rest LGTM

...c/main/java/org/apache/dolphinscheduler/plugin/task/api/am/KubernetesApplicationManager.java

Radeity · 2023-07-11T12:44:39Z

Hi, @Gallardot , I've made some changes based on your suggestions, please help review :D

...k-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/AbstractCommandExecutor.java

...c/main/java/org/apache/dolphinscheduler/plugin/task/api/am/KubernetesApplicationManager.java

Radeity · 2023-07-14T08:17:48Z

Hi, @caishunfeng @SbloodyS , would you guys like to help double check?

zhongjiajie · 2023-07-20T06:48:58Z

re-run the failed ci to find due to code bug or flaky test

SbloodyS

+1

zhongjiajie · 2023-07-20T07:20:34Z

seems it still when I re run the CI, could you please take a look @Radeity

sonarqubecloud · 2023-07-20T09:46:57Z

SonarCloud Quality Gate failed.

1 Bug
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

6.8% Coverage
0.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

* [Improvement-13763][K8S Task] collect real-time log * fix codesmell * get pod watcher until pod is ready * fix codesmell * specify container name & loop waiting pod creation * sleep when pod is not initialized --------- Co-authored-by: Jay Chung <zhongjiajie955@gmail.com>

* [Improvement-13763][K8S Task] collect real-time log * fix codesmell * get pod watcher until pod is ready * fix codesmell * specify container name & loop waiting pod creation * sleep when pod is not initialized --------- Co-authored-by: Jay Chung <zhongjiajie955@gmail.com> (cherry picked from commit 04800a4)

[Improvement-13763][K8S Task] collect real-time log

ed3d9d9

Radeity requested review from caishunfeng, SbloodyS and zhuangchong as code owners June 19, 2023 18:15

github-actions bot added the backend label Jun 19, 2023

Radeity self-assigned this Jun 19, 2023

Radeity added the 3.2.0 for 3.2.0 version label Jun 19, 2023

Radeity added this to the 3.2.0 milestone Jun 19, 2023

fix codesmell

4eea14f

Radeity added the feature new feature label Jun 21, 2023

Gallardot reviewed Jul 10, 2023

View reviewed changes

Radeity added 3 commits July 10, 2023 21:29

Merge branch 'dev' of https://github.com/apache/dolphinscheduler into…

23b9b7c

… Improvement-13763

get pod watcher until pod is ready

ba99579

fix codesmell

2f9d915

Gallardot requested changes Jul 11, 2023

View reviewed changes

...c/main/java/org/apache/dolphinscheduler/plugin/task/api/am/KubernetesApplicationManager.java Show resolved Hide resolved

...c/main/java/org/apache/dolphinscheduler/plugin/task/api/am/KubernetesApplicationManager.java Show resolved Hide resolved

specify container name & loop waiting pod creation

6385fbb

Gallardot reviewed Jul 12, 2023

View reviewed changes

...k-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/AbstractCommandExecutor.java Show resolved Hide resolved

...c/main/java/org/apache/dolphinscheduler/plugin/task/api/am/KubernetesApplicationManager.java Show resolved Hide resolved

sleep when pod is not initialized

7da8707

Radeity added the kubernetes label Jul 14, 2023

Merge branch 'dev' into Improvement-13763

840a9e5

zhongjiajie approved these changes Jul 20, 2023

View reviewed changes

zhongjiajie added the ready-to-merge label Jul 20, 2023

Merge branch 'dev' into Improvement-13763

69ff03d

SbloodyS approved these changes Jul 20, 2023

View reviewed changes

Merge branch 'dev' into Improvement-13763

79fb7e8

Radeity changed the title ~~[Improvement-13763][K8S Task] collect real-time log~~ [Feature-13763][K8S Task] collect real-time log Jul 20, 2023

Merge branch 'dev' into Improvement-13763

b019235

zhongjiajie mentioned this pull request Jul 20, 2023

[Improvement] Dataquality code style enhance #14592

Merged

SbloodyS merged commit 04800a4 into apache:dev Jul 20, 2023

SbloodyS removed the ready-to-merge label Jul 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature-13763][K8S Task] collect real-time log #14379

[Feature-13763][K8S Task] collect real-time log #14379

Radeity commented Jun 19, 2023 •

edited

Loading

codecov-commenter commented Jun 20, 2023 •

edited

Loading

Radeity commented Jul 10, 2023

Gallardot commented Jul 10, 2023

Radeity commented Jul 10, 2023

Gallardot Jul 10, 2023

Radeity Jul 10, 2023

Gallardot Jul 10, 2023

Radeity commented Jul 11, 2023

Gallardot commented Jul 11, 2023

Gallardot left a comment

Radeity commented Jul 11, 2023

Radeity commented Jul 14, 2023

zhongjiajie commented Jul 20, 2023

SbloodyS left a comment

zhongjiajie commented Jul 20, 2023

sonarqubecloud bot commented Jul 20, 2023

[Feature-13763][K8S Task] collect real-time log #14379

[Feature-13763][K8S Task] collect real-time log #14379

Conversation

Radeity commented Jun 19, 2023 • edited Loading

Purpose of the pull request

Brief change log

Verify this pull request

codecov-commenter commented Jun 20, 2023 • edited Loading

Codecov Report

Radeity commented Jul 10, 2023

Gallardot commented Jul 10, 2023

Radeity commented Jul 10, 2023

Gallardot Jul 10, 2023

Choose a reason for hiding this comment

Radeity Jul 10, 2023

Choose a reason for hiding this comment

Gallardot Jul 10, 2023

Choose a reason for hiding this comment

Radeity commented Jul 11, 2023

Gallardot commented Jul 11, 2023

Gallardot left a comment

Choose a reason for hiding this comment

Radeity commented Jul 11, 2023

Radeity commented Jul 14, 2023

zhongjiajie commented Jul 20, 2023

SbloodyS left a comment

Choose a reason for hiding this comment

zhongjiajie commented Jul 20, 2023

sonarqubecloud bot commented Jul 20, 2023

Radeity commented Jun 19, 2023 •

edited

Loading

codecov-commenter commented Jun 20, 2023 •

edited

Loading