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

xdu-chenrj · 2023-09-28T12:24:03Z

Related issues: [DSIP-19][Feature] Add connection center feature for DS #10283

Mail: https://lists.apache.org/thread/xl6pb3sbrt0ffrf1fltcph39s9w1pjlx

...va/org/apache/dolphinscheduler/plugin/datasource/zeppelin/param/ZeppelinConnectionParam.java

+
+import com.fasterxml.jackson.annotation.JsonInclude;
+
+@Data


...va/org/apache/dolphinscheduler/plugin/datasource/zeppelin/param/ZeppelinConnectionParam.java

+
+    protected String username;
+
+    protected String password;


EricGao888 · 2023-10-30T09:24:42Z

@xdu-chenrj The commit history seems not correct.

xdu-chenrj · 2023-10-30T17:05:11Z

@xdu-chenrj The commit history seems not correct.

yes

EricGao888

Waiting for CI to pass

EricGao888 · 2023-10-31T04:12:41Z

@xdu-chenrj

Commit history seems still incorrect.

codecov-commenter · 2023-10-31T05:27:09Z

Codecov Report

Merging #14977 (bf7a5b5) into dev (5a3827e) will increase coverage by 0.01%.
The diff coverage is 52.17%.

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

@@             Coverage Diff              @@
##                dev   #14977      +/-   ##
============================================
+ Coverage     38.19%   38.21%   +0.01%     
- Complexity     4684     4693       +9     
============================================
  Files          1275     1280       +5     
  Lines         45329    45410      +81     
  Branches       4949     4953       +4     
============================================
+ Hits          17315    17354      +39     
- Misses        26121    26163      +42     
  Partials       1893     1893

Files	Coverage Δ
...ache/dolphinscheduler/plugin/task/k8s/K8sTask.java	`84.90% <100.00%> (+2.68%)`	⬆️
...inscheduler/plugin/task/zeppelin/ZeppelinTask.java	`55.33% <100.00%> (ø)`
...in/datasource/k8s/param/K8sDataSourceParamDTO.java	`0.00% <0.00%> (ø)`
...lphinscheduler/plugin/task/k8s/K8sTaskChannel.java	`0.00% <0.00%> (ø)`
.../org/apache/dolphinscheduler/spi/enums/DbType.java	`0.00% <0.00%> (ø)`
...duler/plugin/task/api/K8sTaskExecutionContext.java	`0.00% <0.00%> (ø)`
...ugin/task/api/am/KubernetesApplicationManager.java	`5.33% <0.00%> (-0.15%)`	⬇️
...er/plugin/datasource/k8s/K8sDataSourceChannel.java	`0.00% <0.00%> (ø)`
...in/datasource/k8s/K8sDataSourceChannelFactory.java	`0.00% <0.00%> (ø)`
...uler/plugin/task/api/k8s/impl/K8sTaskExecutor.java	`36.12% <0.00%> (-0.33%)`	⬇️
... and 3 more

... and 2 files with indirect coverage changes

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

...ce-k8s/src/main/java/org/apache/dolphinscheduler/plugin/datasource/k8s/K8sClientWrapper.java

sonarcloud · 2023-10-31T08:27:20Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
8 Code Smells

50.5% Coverage
2.3% Duplication

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

Radeity · 2023-10-31T10:50:57Z

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

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


You can not directly change this. For normal task, like spark on k8s, K8sTaskExecutionContext is constructed by TaskExecutionContextFactory, when they call getClient, connectionParams is null.

Thank you for your suggestion

.../src/main/java/org/apache/dolphinscheduler/plugin/task/api/parameters/K8sTaskParameters.java

Radeity · 2023-10-31T10:56:54Z

Hi @xdu-chenrj, non-K8S task should better be refactored and use k8s connection together with k8s task, maybe we have to revert this PR, or to fix this bug (refactor) ASAP. cc @EricGao888

xdu-chenrj · 2023-10-31T11:05:27Z

Hi @xdu-chenrj, non-K8S task should better be refactored and use k8s connection together with k8s task, maybe we have to revert this PR, or to fix this bug (refactor) ASAP. cc @EricGao888

ok

EricGao888 · 2023-10-31T11:08:29Z

Hi @xdu-chenrj, non-K8S task should better be refactored and use k8s connection together with k8s task, maybe we have to revert this PR, or to fix this bug (refactor) ASAP. cc @EricGao888

@xdu-chenrj I suggest submitting a patch to fix it instead of reverting the whole PR. BTW, as the PR passed the CI, it seems the UT hadn't covered this case. I think it will be better to add related UT to cover this scenario for those tasks using k8s connection.

xdu-chenrj · 2023-10-31T11:17:28Z

Hi @xdu-chenrj, non-K8S task should better be refactored and use k8s connection together with k8s task, maybe we have to revert this PR, or to fix this bug (refactor) ASAP. cc @EricGao888

@xdu-chenrj I suggest submitting a patch to fix it instead of reverting the whole PR. BTW, as the PR passed the CI, it seems the UT hadn't covered this case. I think it will be better to add related UT to cover this scenario for those tasks using k8s connection.

Yes, thank you for your suggestion

Radeity · 2023-10-31T11:24:15Z

Hi @xdu-chenrj, non-K8S task should better be refactored and use k8s connection together with k8s task, maybe we have to revert this PR, or to fix this bug (refactor) ASAP. cc @EricGao888

@xdu-chenrj I suggest submitting a patch to fix it instead of reverting the whole PR. BTW, as the PR passed the CI, it seems the UT hadn't covered this case. I think it will be better to add related UT to cover this scenario for those tasks using k8s connection.

Yes, thank you for your suggestion

You can see TaskExecutionContextFactory.setK8sTaskRelation, there are several types of task which already construct K8sTaskExecutionContext in Master. Anyway, you can ping me anytime if you need help with this patch :D

EricGao888 · 2023-10-31T11:26:18Z

Hi @xdu-chenrj, non-K8S task should better be refactored and use k8s connection together with k8s task, maybe we have to revert this PR, or to fix this bug (refactor) ASAP. cc @EricGao888

@xdu-chenrj I suggest submitting a patch to fix it instead of reverting the whole PR. BTW, as the PR passed the CI, it seems the UT hadn't covered this case. I think it will be better to add related UT to cover this scenario for those tasks using k8s connection.

Yes, thank you for your suggestion

You can see TaskExecutionContextFactory.setK8sTaskRelation, there are several types of task which already construct K8sTaskExecutionContext in Master. Anyway, you can ping me anytime if you need help with this patch :D

pretty cool

xdu-chenrj · 2023-10-31T13:30:47Z

You can see TaskExecutionContextFactory.setK8sTaskRelation, there are several types of task which already construct K8sTaskExecutionContext in Master. Anyway, you can ping me anytime if you need help with this patch :D

Thank you very much for your suggestion :D

xdu-chenrj · 2023-10-31T15:43:03Z

Hi @xdu-chenrj, non-K8S task should better be refactored and use k8s connection together with k8s task, maybe we have to revert this PR, or to fix this bug (refactor) ASAP. cc @EricGao888

@xdu-chenrj I suggest submitting a patch to fix it instead of reverting the whole PR. BTW, as the PR passed the CI, it seems the UT hadn't covered this case. I think it will be better to add related UT to cover this scenario for those tasks using k8s connection.

Yes, thank you for your suggestion

You can see TaskExecutionContextFactory.setK8sTaskRelation, there are several types of task which already construct K8sTaskExecutionContext in Master. Anyway, you can ping me anytime if you need help with this patch :D

    private K8sTaskExecutionContext setK8sTaskRelation(TaskInstance taskInstance) {
        K8sTaskExecutionContext k8sTaskExecutionContext = null;
        String namespace = "", kubeConfig = "";
        log.info("getTaskParams {}", taskInstance.getTaskParams());
        switch (taskInstance.getTaskType()) {
            case "K8S":
            case "KUBEFLOW":
                K8sTaskParameters k8sTaskParameters =
                        JSONUtils.parseObject(taskInstance.getTaskParams(), K8sTaskParameters.class);
                namespace = k8sTaskParameters.getNamespace();
                kubeConfig = k8sTaskParameters.getKubeConfig();
                break;
            case "SPARK":
                SparkParameters sparkParameters =
                        JSONUtils.parseObject(taskInstance.getTaskParams(), SparkParameters.class);
                if (StringUtils.isNotEmpty(sparkParameters.getNamespace())) {
                    namespace = sparkParameters.getNamespace();
                }
                break;
            default:
                break;
        }

        if (StringUtils.isNotEmpty(namespace)) {
            String configYaml = kubeConfig;
            log.info("String configYaml {}", configYaml);
            if (configYaml != null) {
                k8sTaskExecutionContext =
                        new K8sTaskExecutionContext(configYaml, JSONUtils.toMap(namespace).get(NAMESPACE_NAME));
            }
        }
        return k8sTaskExecutionContext;
    }

hi, I want to assign the value configYaml to k8sTaskExecutionContext through this method, but taskInstance. getTaskParams() lacks the kubeConfig required in K8sTaskParameters. I would like to receive your specific advice

Radeity · 2023-10-31T18:32:50Z

    private K8sTaskExecutionContext setK8sTaskRelation(TaskInstance taskInstance) {
        K8sTaskExecutionContext k8sTaskExecutionContext = null;
        String namespace = "", kubeConfig = "";
        log.info("getTaskParams {}", taskInstance.getTaskParams());
        switch (taskInstance.getTaskType()) {
            case "K8S":
            case "KUBEFLOW":
                K8sTaskParameters k8sTaskParameters =
                        JSONUtils.parseObject(taskInstance.getTaskParams(), K8sTaskParameters.class);
                namespace = k8sTaskParameters.getNamespace();
                kubeConfig = k8sTaskParameters.getKubeConfig();
                break;
            case "SPARK":
                SparkParameters sparkParameters =
                        JSONUtils.parseObject(taskInstance.getTaskParams(), SparkParameters.class);
                if (StringUtils.isNotEmpty(sparkParameters.getNamespace())) {
                    namespace = sparkParameters.getNamespace();
                }
                break;
            default:
                break;
        }

        if (StringUtils.isNotEmpty(namespace)) {
            String configYaml = kubeConfig;
            log.info("String configYaml {}", configYaml);
            if (configYaml != null) {
                k8sTaskExecutionContext =
                        new K8sTaskExecutionContext(configYaml, JSONUtils.toMap(namespace).get(NAMESPACE_NAME));
            }
        }
        return k8sTaskExecutionContext;
    }

hi, I want to assign the value configYaml to k8sTaskExecutionContext through this method, but taskInstance. getTaskParams() lacks the kubeConfig required in K8sTaskParameters. I would like to receive your specific advice

Can we move method K8sTaskParameters .generateExtendedContext to AbstractParameters and name it generateK8sTaskExecutionContext. Then, for KUBEFLOW, Kubernetes, Spark task, call generateK8sTaskExecutionContext to construct K8sTaskExecutionContext inside their parameters' construction function. In this way, we don't have to setK8sTaskRelation in Master, WDYT?

xdu-chenrj · 2023-11-02T14:16:16Z

    private K8sTaskExecutionContext setK8sTaskRelation(TaskInstance taskInstance) {
        K8sTaskExecutionContext k8sTaskExecutionContext = null;
        String namespace = "", kubeConfig = "";
        log.info("getTaskParams {}", taskInstance.getTaskParams());
        switch (taskInstance.getTaskType()) {
            case "K8S":
            case "KUBEFLOW":
                K8sTaskParameters k8sTaskParameters =
                        JSONUtils.parseObject(taskInstance.getTaskParams(), K8sTaskParameters.class);
                namespace = k8sTaskParameters.getNamespace();
                kubeConfig = k8sTaskParameters.getKubeConfig();
                break;
            case "SPARK":
                SparkParameters sparkParameters =
                        JSONUtils.parseObject(taskInstance.getTaskParams(), SparkParameters.class);
                if (StringUtils.isNotEmpty(sparkParameters.getNamespace())) {
                    namespace = sparkParameters.getNamespace();
                }
                break;
            default:
                break;
        }

        if (StringUtils.isNotEmpty(namespace)) {
            String configYaml = kubeConfig;
            log.info("String configYaml {}", configYaml);
            if (configYaml != null) {
                k8sTaskExecutionContext =
                        new K8sTaskExecutionContext(configYaml, JSONUtils.toMap(namespace).get(NAMESPACE_NAME));
            }
        }
        return k8sTaskExecutionContext;
    }

hi, I want to assign the value configYaml to k8sTaskExecutionContext through this method, but taskInstance. getTaskParams() lacks the kubeConfig required in K8sTaskParameters. I would like to receive your specific advice

Can we move method K8sTaskParameters .generateExtendedContext to AbstractParameters and name it generateK8sTaskExecutionContext. Then, for KUBEFLOW, Kubernetes, Spark task, call generateK8sTaskExecutionContext to construct K8sTaskExecutionContext inside their parameters' construction function. In this way, we don't have to setK8sTaskRelation in Master, WDYT?

I fixed this issue, and then I will create an issue and PR

xdu-chenrj · 2023-11-02T14:19:26Z

    private K8sTaskExecutionContext setK8sTaskRelation(TaskInstance taskInstance) {
        K8sTaskExecutionContext k8sTaskExecutionContext = null;
        String namespace = "", kubeConfig = "";
        log.info("getTaskParams {}", taskInstance.getTaskParams());
        switch (taskInstance.getTaskType()) {
            case "K8S":
            case "KUBEFLOW":
                K8sTaskParameters k8sTaskParameters =
                        JSONUtils.parseObject(taskInstance.getTaskParams(), K8sTaskParameters.class);
                namespace = k8sTaskParameters.getNamespace();
                kubeConfig = k8sTaskParameters.getKubeConfig();
                break;
            case "SPARK":
                SparkParameters sparkParameters =
                        JSONUtils.parseObject(taskInstance.getTaskParams(), SparkParameters.class);
                if (StringUtils.isNotEmpty(sparkParameters.getNamespace())) {
                    namespace = sparkParameters.getNamespace();
                }
                break;
            default:
                break;
        }

        if (StringUtils.isNotEmpty(namespace)) {
            String configYaml = kubeConfig;
            log.info("String configYaml {}", configYaml);
            if (configYaml != null) {
                k8sTaskExecutionContext =
                        new K8sTaskExecutionContext(configYaml, JSONUtils.toMap(namespace).get(NAMESPACE_NAME));
            }
        }
        return k8sTaskExecutionContext;
    }

hi, I want to assign the value configYaml to k8sTaskExecutionContext through this method, but taskInstance. getTaskParams() lacks the kubeConfig required in K8sTaskParameters. I would like to receive your specific advice

Can we move method K8sTaskParameters .generateExtendedContext to AbstractParameters and name it generateK8sTaskExecutionContext. Then, for KUBEFLOW, Kubernetes, Spark task, call generateK8sTaskExecutionContext to construct K8sTaskExecutionContext inside their parameters' construction function. In this way, we don't have to setK8sTaskRelation in Master, WDYT?

I fixed this issue, and then I will create an issue and PR

and I plan to submit a PR to fix the problem first, and then add UT. Is that okay?

Radeity · 2023-11-02T14:23:18Z

I fixed this issue, and then I will create an issue and PR

Glad to hear that!

and I plan to submit a PR to fix the problem first, and then add UT. Is that okay?

Sure, you can. I think, for a single pr, sometimes, small is better :D

xdu-chenrj · 2023-11-03T07:33:48Z

I fixed this issue, and then I will create an issue and PR

Glad to hear that!

and I plan to submit a PR to fix the problem first, and then add UT. Is that okay?

Sure, you can. I think, for a single pr, sometimes, small is better :D

issue: #15114
pr: #15116

…naged in connection center (#14977)" This reverts commit c532fea.

…naged in connection center (apache#14977)" This reverts commit c532fea.

* 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 requested review from songjianet, Amy0104, caishunfeng, SbloodyS and zhuangchong as code owners September 28, 2023 12:24

github-actions bot added UI ui and front end related backend e2e e2e test labels Sep 28, 2023

mergeable bot removed UI ui and front end related backend e2e e2e test labels Sep 28, 2023

github-actions bot added UI ui and front end related backend e2e e2e test labels Oct 12, 2023

github-advanced-security bot found potential problems Oct 12, 2023

View reviewed changes

xdu-chenrj requested review from zhongjiajie, EricGao888 and ruanwenjun as code owners October 30, 2023 09:10

github-actions bot added CI&CD document labels Oct 30, 2023

github-actions bot removed CI&CD document labels Oct 30, 2023

EricGao888 closed this Oct 31, 2023

EricGao888 reopened this Oct 31, 2023

EricGao888 previously approved these changes Oct 31, 2023

View reviewed changes

EricGao888 assigned xdu-chenrj Oct 31, 2023

EricGao888 added feature new feature and removed feature new feature labels Oct 31, 2023

xdu-chenrj dismissed EricGao888’s stale review via 97c9f38 October 31, 2023 03:49

github-advanced-security bot found potential problems Oct 31, 2023

View reviewed changes

...ce-k8s/src/main/java/org/apache/dolphinscheduler/plugin/datasource/k8s/K8sClientWrapper.java Fixed Show fixed Hide fixed

...ce-k8s/src/main/java/org/apache/dolphinscheduler/plugin/datasource/k8s/K8sClientWrapper.java Fixed Show fixed Hide fixed

support k8s connection

de8c741

xdu-chenrj force-pushed the k8s branch from 97c9f38 to de8c741 Compare October 31, 2023 07:47

EricGao888 approved these changes Oct 31, 2023

View reviewed changes

EricGao888 merged commit c532fea into apache:dev Oct 31, 2023
53 of 56 checks passed

Radeity reviewed Oct 31, 2023

View reviewed changes

fuchanghai mentioned this pull request Nov 2, 2023

[improvement-#14995] get namespace in real time #15106

Closed

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

Revert "[Improvement] Refactoring K8S task plugin with connections ma…

097ad90

…naged in connection center (#14977)" This reverts commit c532fea.

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

Revert "[Improvement] Refactoring K8S task plugin with connections ma…

1da4dc2

…naged in connection center (apache#14977)" This reverts commit c532fea.

SbloodyS added the 3.3.0 label Jun 26, 2024

SbloodyS mentioned this pull request Jun 26, 2024

[Chore] Revert some incompatible change for 3.2.2 #16214

Merged

SbloodyS added the DSIP label Aug 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

xdu-chenrj commented Sep 28, 2023

EricGao888 commented Oct 30, 2023

xdu-chenrj commented Oct 30, 2023

EricGao888 left a comment

EricGao888 commented Oct 31, 2023

codecov-commenter commented Oct 31, 2023 •

edited

Loading

sonarcloud bot commented Oct 31, 2023

Radeity Oct 31, 2023

xdu-chenrj Oct 31, 2023

Radeity commented Oct 31, 2023

xdu-chenrj commented Oct 31, 2023

EricGao888 commented Oct 31, 2023

xdu-chenrj commented Oct 31, 2023

Radeity commented Oct 31, 2023

EricGao888 commented Oct 31, 2023

xdu-chenrj commented Oct 31, 2023

xdu-chenrj commented Oct 31, 2023

Radeity commented Oct 31, 2023

xdu-chenrj commented Nov 2, 2023

xdu-chenrj commented Nov 2, 2023

Radeity commented Nov 2, 2023

xdu-chenrj commented Nov 3, 2023

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


		import com.fasterxml.jackson.annotation.JsonInclude;

		@Data

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

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

Conversation

xdu-chenrj commented Sep 28, 2023

EricGao888 commented Oct 30, 2023

xdu-chenrj commented Oct 30, 2023

EricGao888 left a comment

Choose a reason for hiding this comment

EricGao888 commented Oct 31, 2023

codecov-commenter commented Oct 31, 2023 • edited Loading

Codecov Report

sonarcloud bot commented Oct 31, 2023

Radeity Oct 31, 2023

Choose a reason for hiding this comment

xdu-chenrj Oct 31, 2023

Choose a reason for hiding this comment

Radeity commented Oct 31, 2023

xdu-chenrj commented Oct 31, 2023

EricGao888 commented Oct 31, 2023

xdu-chenrj commented Oct 31, 2023

Radeity commented Oct 31, 2023

EricGao888 commented Oct 31, 2023

xdu-chenrj commented Oct 31, 2023

xdu-chenrj commented Oct 31, 2023

Radeity commented Oct 31, 2023

xdu-chenrj commented Nov 2, 2023

xdu-chenrj commented Nov 2, 2023

Radeity commented Nov 2, 2023

xdu-chenrj commented Nov 3, 2023

codecov-commenter commented Oct 31, 2023 •

edited

Loading