[Fix] Fix k8sTaskExecutionContext setting configYaml #15116

xdu-chenrj · 2023-11-03T07:28:28Z

close #15114
Fix k8sTask ExecutionContext setting configYaml

xdu-chenrj · 2023-11-03T07:30:05Z

I fixed this issue using other methods, but there may be other issues. Please review :D

Radeity · 2023-11-04T15:09:10Z

Hi @xdu-chenrj , I don't understand how you fix it. Actually, there's nothing wrong with K8S task, but for Spark on K8S, k8sTaskExecutionContext.getConnectionParams() is null.

In addition, for other types of task which use K8sTaskExecutionContext, like Kubeflow, you should better replace the current way to declare the Kubernetes cluster with your connection center.

xdu-chenrj · 2023-11-05T15:13:51Z

Hi @xdu-chenrj , I don't understand how you fix it. Actually, there's nothing wrong with K8S task, but for Spark on K8S, k8sTaskExecutionContext.getConnectionParams() is null.

In addition, for other types of task which use K8sTaskExecutionContext, like Kubeflow, you should better replace the current way to declare the Kubernetes cluster with your connection center.

I will use the Spark task to further correct my fix

Radeity · 2023-11-05T15:25:30Z

I will use the Spark task to further correct my fix

I thought you mainly fix it in this PR, and why setting configYaml is relevant to the bug description in issue?

xdu-chenrj · 2023-11-05T16:08:15Z

I will use the Spark task to further correct my fix

I thought you mainly fix it in this PR, and why setting configYaml is relevant to the bug description in issue?

we need to fix all tasks that use the k8s namespace, such as the Spark task. i didn't get what you meant before. i just looked at some parameters required to create the Spark task. the namespace selected for the previous spark task was created in the cluster management of the security center. now, the Spark task needs to select the k8s connection created in the connection center. we need to refactor other tasks that use the k8s namespace, just like we refactor the k8s task, what do you think?

Radeity · 2023-11-05T16:28:09Z

we need to fix all tasks that use the k8s namespace, such as the Spark task. i didn't get what you meant before. i just looked at some parameters required to create the Spark task. the namespace selected for the previous spark task was created in the cluster management of the security center. now, the Spark task needs to select the k8s connection created in the connection center. we need to refactor other tasks that use the k8s namespace, just like we refactor the k8s task, what do you think?

For Spark on K8S, connectionParams in k8sTaskExecutionContext is null, cuz k8sTaskExecutionContext is constructed in Master, and don't set any value during construction which will cause NPE.

k8sTaskExecutionContext
                .setConfigYaml(JSONUtils.getNodeString(k8sTaskExecutionContext.getConnectionParams(), "kubeConfig"));

xdu-chenrj · 2023-11-06T02:16:18Z

we need to fix all tasks that use the k8s namespace, such as the Spark task. i didn't get what you meant before. i just looked at some parameters required to create the Spark task. the namespace selected for the previous spark task was created in the cluster management of the security center. now, the Spark task needs to select the k8s connection created in the connection center. we need to refactor other tasks that use the k8s namespace, just like we refactor the k8s task, what do you think?

For Spark on K8S, connectionParams in k8sTaskExecutionContext is null, cuz k8sTaskExecutionContext is constructed in Master, and don't set any value during construction which will cause NPE.
k8sTaskExecutionContext
                .setConfigYaml(JSONUtils.getNodeString(k8sTaskExecutionContext.getConnectionParams(), "kubeConfig"));

I have made this modification to the current code

codecov-commenter · 2023-11-08T01:30:49Z

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (159179a) 37.80% compared to head (28af50c) 37.81%.

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

Files	Patch %	Lines
...plugin/task/api/parameters/AbstractParameters.java	0.00%	6 Missing ⚠️
...uler/plugin/task/api/k8s/impl/K8sTaskExecutor.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##                dev   #15116   +/-   ##
=========================================
  Coverage     37.80%   37.81%           
  Complexity     4701     4701           
=========================================
  Files          1304     1304           
  Lines         45219    45217    -2     
  Branches       4859     4859           
=========================================
+ Hits          17097    17099    +2     
+ Misses        26259    26255    -4     
  Partials       1863     1863

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sonarqubecloud · 2023-11-10T08:21:40Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

30.8% Coverage
0.0% Duplication

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

Radeity · 2023-11-11T14:09:48Z

I have made this modification to the current code

I'm still confused, you should modify this part to avoid NPE

k8sTaskExecutionContext
                .setConfigYaml(JSONUtils.getNodeString(k8sTaskExecutionContext.getConnectionParams(), "kubeConfig"));

OR

replace original way to use K8S cluster in other tasks(Spark, KUBEFLOW) with your connection way.

In current PR, I don't get how you solve the problem.

xdu-chenrj · 2023-11-13T08:33:09Z

I have made this modification to the current code

I'm still confused, you should modify this part to avoid NPE
k8sTaskExecutionContext
                .setConfigYaml(JSONUtils.getNodeString(k8sTaskExecutionContext.getConnectionParams(), "kubeConfig"));
OR

replace original way to use K8S cluster in other tasks(Spark, KUBEFLOW) with your connection way.

In current PR, I don't get how you solve the problem.

I think the current solution to this problem can only be achieved by adding special judgments until the Spark and Kubelow tasks are refactored

Radeity

LGTM

Radeity · 2023-11-17T01:46:03Z

Please run mvn spotless:apply.

xdu-chenrj · 2023-12-08T13:20:17Z

Please run mvn spotless:apply.

sorry

sonarqubecloud · 2023-12-14T08:51:23Z

Quality Gate failed

Failed conditions

22.2% Coverage on New Code (required ≥ 60%)

See analysis details on SonarCloud

@DaTa

fix typo bug in JSONUtils.java add DBType dolphindb spotless apply & modify ui fix mvn package OK TO DO: Modify DolphinDBDataSourceProcessor fix datasource bug -> connect OK [Improvement-15260][dolphinscheduler-datasource-hana] add hana related dependencies (apache#15260) * add hana related dependencies * optimizing HANA data source bugs * run mvn spotless:apply * Fix hana datasource getValidationQuery() * Fix hana datasource testGetJdbcUrl() --------- Co-authored-by: xujiaqiang <“[email protected]”> Co-authored-by: David Zollo <[email protected]> [Feature-15248][dolphinscheduler-alert-plugins] add alert plugin aliyun-voice (apache#15248) * add alert aliyun-voice * VoiceParam replace these with lombok @DaTa * security.ts add alarm_instance params * optimize alert aliyun vocie code * Merge Code * rollback pnpm-lock.yaml to branch :origin/dev version * add the doc about the new plugin * add com.aliyun.dyvmsapi20170525 version 2.14 * completed * run mvn spotless:apply * Code optimization * Change to @DaTa * Code specification optimization * add com.aliyund.yvmsapi20170525.jar license * modify com.aliyund.yvmsapi20170525.jar license * modify com.aliyund.yvmsapi20170525.jar license * add aliyun-vocie licenses * add aliyun-voice link to docsdev.js * modify vocie module to aliyunVoice * add lisense --------- Co-authored-by: xujiaqiang <“[email protected]”> Co-authored-by: 旺阳 <[email protected]> Co-authored-by: David Zollo <[email protected]> [Feature-15146][dolphinscheduler-task-sqoop] add sqoop source/target type (apache#15146) * task list: sgoop node params optimize * security.ts add alarm_instance params * 1 add SqoopTask params 2 add alert plugin aliyun-voice * add license header * commit sqhoop optimize * pnpm-locl.yaml supplement annotation * remove irrelevent commit. * Code specification optimization * optimize sqoop task ui * Merge Code * add the license header to pnpm-locl.yaml * format the code * format the code * Fix sqoop task echo error --------- Co-authored-by: xujiaqiang <[email protected]> Co-authored-by: xujiaqiang <“[email protected]”> Co-authored-by: David Zollo <[email protected]> finishgit add dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/constants/DataSourceConstants.java Fix k8sTaskExecutionContext setting configYaml (apache#15116) * fixed the issue of obtaining kubeConfig of k8s tasks * removed excess files * removed excess files * formatted the code --------- Co-authored-by: xiangzihao <[email protected]> Co-authored-by: Aaron Wang <[email protected]>

@DaTa

fix typo bug in JSONUtils.java add DBType dolphindb spotless apply & modify ui fix mvn package OK TO DO: Modify DolphinDBDataSourceProcessor fix datasource bug -> connect OK [Improvement-15260][dolphinscheduler-datasource-hana] add hana related dependencies (apache#15260) * add hana related dependencies * optimizing HANA data source bugs * run mvn spotless:apply * Fix hana datasource getValidationQuery() * Fix hana datasource testGetJdbcUrl() --------- Co-authored-by: xujiaqiang <“[email protected]”> Co-authored-by: David Zollo <[email protected]> [Feature-15248][dolphinscheduler-alert-plugins] add alert plugin aliyun-voice (apache#15248) * add alert aliyun-voice * VoiceParam replace these with lombok @DaTa * security.ts add alarm_instance params * optimize alert aliyun vocie code * Merge Code * rollback pnpm-lock.yaml to branch :origin/dev version * add the doc about the new plugin * add com.aliyun.dyvmsapi20170525 version 2.14 * completed * run mvn spotless:apply * Code optimization * Change to @DaTa * Code specification optimization * add com.aliyund.yvmsapi20170525.jar license * modify com.aliyund.yvmsapi20170525.jar license * modify com.aliyund.yvmsapi20170525.jar license * add aliyun-vocie licenses * add aliyun-voice link to docsdev.js * modify vocie module to aliyunVoice * add lisense --------- Co-authored-by: xujiaqiang <“[email protected]”> Co-authored-by: 旺阳 <[email protected]> Co-authored-by: David Zollo <[email protected]> [Feature-15146][dolphinscheduler-task-sqoop] add sqoop source/target type (apache#15146) * task list: sgoop node params optimize * security.ts add alarm_instance params * 1 add SqoopTask params 2 add alert plugin aliyun-voice * add license header * commit sqhoop optimize * pnpm-locl.yaml supplement annotation * remove irrelevent commit. * Code specification optimization * optimize sqoop task ui * Merge Code * add the license header to pnpm-locl.yaml * format the code * format the code * Fix sqoop task echo error --------- Co-authored-by: xujiaqiang <[email protected]> Co-authored-by: xujiaqiang <“[email protected]”> Co-authored-by: David Zollo <[email protected]> finishgit add dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/constants/DataSourceConstants.java Fix k8sTaskExecutionContext setting configYaml (apache#15116) * fixed the issue of obtaining kubeConfig of k8s tasks * removed excess files * removed excess files * formatted the code --------- Co-authored-by: xiangzihao <[email protected]> Co-authored-by: Aaron Wang <[email protected]>

This reverts commit ce11674.

* Revert "Fix k8sTaskExecutionContext setting configYaml (#15116)" This reverts commit ce11674. * Revert "[Improvement] Refactoring K8S task plugin with connections managed in connection center (#14977)" This reverts commit c532fea.

xdu-chenrj added 2 commits November 3, 2023 15:14

fixed the issue of obtaining kubeConfig of k8s tasks

972d16a

removed excess files

eeddd54

xdu-chenrj requested review from SbloodyS, caishunfeng and zhuangchong as code owners November 3, 2023 07:28

github-actions bot added the backend label Nov 3, 2023

xdu-chenrj mentioned this pull request Nov 3, 2023

[DSIP-19] Support k8s connections in the connection center, as well as external connections to the connection center in k8s tasks #14977

Merged

Radeity self-requested a review November 3, 2023 08:31

Radeity assigned xdu-chenrj Nov 3, 2023

Radeity added the 3.3.0 label Nov 3, 2023

Radeity added this to the 3.3.0 milestone Nov 3, 2023

Merge branch 'dev' into fix-k8s

76af727

Merge branch 'dev' into fix-k8s

77522e7

xdu-chenrj and others added 2 commits November 15, 2023 18:43

removed excess files

0b4257e

Merge branch 'dev' into fix-k8s

81601f7

Radeity added the ready-to-merge label Nov 16, 2023

Radeity previously approved these changes Nov 16, 2023

View reviewed changes

Merge branch 'dev' into fix-k8s

67f3474

Merge branch 'dev' into fix-k8s

c4b73e3

xdu-chenrj and others added 2 commits December 12, 2023 22:44

Merge branch 'dev' into fix-k8s

71c259f

formatted the code

d55929d

xdu-chenrj dismissed Radeity’s stale review via d55929d December 12, 2023 14:51

EricGao888 self-requested a review December 13, 2023 03:39

EricGao888 mentioned this pull request Dec 13, 2023

[Bug] K8sTaskExecutionContext incorrectly set configYaml #15114

Closed

3 tasks

Merge branch 'dev' into fix-k8s

c9a6a01

Radeity added the bug Something isn't working label Dec 14, 2023

Radeity approved these changes Dec 14, 2023

View reviewed changes

Radeity merged commit ce11674 into apache:dev Dec 14, 2023

zhongjiajie added a commit that referenced this pull request Feb 6, 2024

Revert "Fix k8sTaskExecutionContext setting configYaml (#15116)"

57a4325

This reverts commit ce11674.

zhongjiajie added a commit to zhongjiajie/dolphinscheduler that referenced this pull request Feb 7, 2024

Revert "Fix k8sTaskExecutionContext setting configYaml (apache#15116)"

ad80c6c

This reverts commit ce11674.

SbloodyS changed the title ~~Fix k8sTaskExecutionContext setting configYaml~~ [Fix] Fix k8sTaskExecutionContext setting configYaml Aug 30, 2024

[Fix] Fix k8sTaskExecutionContext setting configYaml #15116

[Fix] Fix k8sTaskExecutionContext setting configYaml #15116

Uh oh!

Conversation

xdu-chenrj commented Nov 3, 2023 • edited by SbloodyS Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xdu-chenrj commented Nov 3, 2023

Uh oh!

Radeity commented Nov 4, 2023

Uh oh!

xdu-chenrj commented Nov 5, 2023 • edited by Radeity Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Radeity commented Nov 5, 2023

Uh oh!

xdu-chenrj commented Nov 5, 2023

Uh oh!

Radeity commented Nov 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xdu-chenrj commented Nov 6, 2023

Uh oh!

codecov-commenter commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sonarqubecloud bot commented Nov 10, 2023

Uh oh!

Radeity commented Nov 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xdu-chenrj commented Nov 13, 2023

Uh oh!

Radeity left a comment

Choose a reason for hiding this comment

Uh oh!

Radeity commented Nov 17, 2023

Uh oh!

xdu-chenrj commented Dec 8, 2023

Uh oh!

sonarqubecloud bot commented Dec 14, 2023

Quality Gate failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xdu-chenrj commented Nov 3, 2023 •

edited by SbloodyS

Loading

xdu-chenrj commented Nov 5, 2023 •

edited by Radeity

Loading

Radeity commented Nov 5, 2023 •

edited

Loading

codecov-commenter commented Nov 8, 2023 •

edited

Loading

Radeity commented Nov 11, 2023 •

edited

Loading