Skip to content

[Feature][Registry] Add DS K8S registry plugin#13458

Closed
EricGao888 wants to merge 1 commit intoapache:devfrom
EricGao888:Feature-13445
Closed

[Feature][Registry] Add DS K8S registry plugin#13458
EricGao888 wants to merge 1 commit intoapache:devfrom
EricGao888:Feature-13445

Conversation

@EricGao888
Copy link
Copy Markdown
Member

Purpose of the pull request

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

@EricGao888 EricGao888 added feature new feature miss:docs missing documents in PR miss:tests missing unit tests in PR 3.2.0 labels Jan 30, 2023
@EricGao888 EricGao888 added this to the 3.2.0 milestone Jan 30, 2023
@EricGao888 EricGao888 self-assigned this Jan 30, 2023
@EricGao888 EricGao888 changed the title [Feature] Add DS K8S registry plugin [Feature][Registry] Add DS K8S registry plugin Jan 30, 2023
@EricGao888
Copy link
Copy Markdown
Member Author

Action Items:

  1. Use Informer instead of plain Watch.
  2. Restrict K8S client permissions with K8S RBAC for masters and workers.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #13458 (7c5a627) into dev (ef47e7e) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                dev   #13458      +/-   ##
============================================
- Coverage     39.56%   39.54%   -0.03%     
- Complexity     4337     4346       +9     
============================================
  Files          1086     1095       +9     
  Lines         40958    41113     +155     
  Branches       4697     4694       -3     
============================================
+ Hits          16206    16259      +53     
- Misses        22949    23048      +99     
- Partials       1803     1806       +3     
Impacted Files Coverage Δ
...org/apache/dolphinscheduler/remote/utils/Host.java 42.55% <0.00%> (-2.13%) ⬇️
...dolphinscheduler/remote/future/ResponseFuture.java 81.96% <0.00%> (-1.64%) ⬇️
...hinscheduler/plugin/alert/script/ScriptSender.java 70.58% <0.00%> (-1.64%) ⬇️
...e/dolphinscheduler/remote/NettyRemotingClient.java 51.38% <0.00%> (-1.39%) ⬇️
...cheduler/plugin/alert/dingtalk/DingTalkSender.java 34.13% <0.00%> (-0.78%) ⬇️
...rver/master/runner/task/BlockingTaskProcessor.java 75.86% <0.00%> (-0.55%) ⬇️
...inscheduler/plugin/registry/etcd/EtcdRegistry.java 50.69% <0.00%> (-0.35%) ⬇️
.../dolphinscheduler/plugin/task/datax/DataxTask.java 36.62% <0.00%> (-0.26%) ⬇️
.../dolphinscheduler/plugin/task/spark/SparkTask.java 72.56% <0.00%> (-0.25%) ⬇️
.../org/apache/dolphinscheduler/api/enums/Status.java 100.00% <0.00%> (ø)
... and 38 more

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

private final Map<String, Watch> watcherMap = new ConcurrentHashMap<>();

private static final long TIME_TO_LIVE_SECONDS = 30L;
public KubernetesRegistry(KubernetesProperties registryProperties) {

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'registryProperties' is never used.
public Collection<String> children(String key) {
// Make sure the string end with '/'
// eg:change key = /nodes to /nodes/
String prefix = key.endsWith(FOLDER_SEPARATOR) ? key : key + FOLDER_SEPARATOR;

Check notice

Code scanning / CodeQL

Unread local variable

Variable 'String prefix' is never read.
@sonarqubecloud
Copy link
Copy Markdown

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 21 Code Smells

0.0% 0.0% Coverage
18.6% 18.6% Duplication


@Data
@Configuration
@ConditionalOnProperty(prefix = "registry", name = "type", havingValue = "k8s")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer we use the complete form

Suggested change
@ConditionalOnProperty(prefix = "registry", name = "type", havingValue = "k8s")
@ConditionalOnProperty(prefix = "registry", name = "type", havingValue = "kubernetes")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will change it.

private String namespace = "dolphinscheduler";
private Duration connectionTimeout = Duration.ofSeconds(9);

// TODO: add rbac related config
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I don't think we need this, I believe kubernetes registry plugin must be used when DolphinScheduler itself is deployed inside kubernetes, so we should add the rbac in our helm chart and we can assume it has sufficient permissions to access the needed resources, so no extra configurations are needed here

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 3, 2023

This pull request has been automatically marked as stale because it has not had recent activity for 120 days. It will be closed in 7 days if no further activity occurs.

@github-actions github-actions bot added the Stale label Jun 3, 2023
@github-actions
Copy link
Copy Markdown

This pull request has been closed because it has not had recent activity. You could reopen it if you try to continue your work, and anyone who are interested in it are encouraged to continue work on this pull request.

@github-actions github-actions bot closed this Jun 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend feature new feature miss:docs missing documents in PR miss:tests missing unit tests in PR Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature][Registry] Remove dependency on ZooKeeper when deployed in K8S

4 participants