Fix issue with long data source names #14620

georgew5656 · 2023-07-19T19:21:08Z

Fixes a issue where having data source names (or more generally task ids) that are too long cause the K8sTaskRunner to throw errors.

Description

The K8sTaskRunner truncates task ids in order to get a valid K8s job name (less than 64 characters) and doing this can cause loss of uniqueness. I fixed this by appending a 128 bit hash onto the end of the task id which i think should be sufficient to ensure uniqueness. (Change is in KubernetesOverlordUtils).

I also had to fix a related issue that was covered up by the fact that our truncation function can be applied repeatedly wihtout any affect.
The issue is the line in KubernetesPeonClient
K8sTaskId taskId = new K8sTaskId(job.getMetadata().getName());.
This takes the job name from the k8s api (which has already been truncated by our truncation function since it is a job name), and then passes it into the K8sTaskId constructor as the originalTaskId. The K8sTaskId calls the truncation function a second time to generate the k8sTaskId. This is okay when our truncation function just grabs the first 63 characters of the task id since repeatedly applying this won't do anything, but with my latest changes this is no longer the case.

I updated this logic to just use the job name directly from the kubernetes api (that's all it needs anyways) and I had to change a lot of function signatures to do this. I also explicitly changed the k8sTaskId field to k8sJobName since that's the only thing that field is used for.

This does make it slightly more confusing to find out which task a job corresponds to from a k8s client like k8s, but the PodTemplateTaskAdapter also applies the whole unmodified task id as a annotation.

Release note

Fixes a issue where having data source names (or more generally task ids) that are too long cause the K8sTaskRunner to throw errors.
-->

Key changed/added classes in this PR

KubernetesOverlordUtils
KubernetesPeonClient
K8sTaskId

This PR has:

[ X] been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
a release note entry in the PR description.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
[ X] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
[X ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
added integration tests.
[ X] been tested in a test Druid cluster.

…aSourceNames

abhishekagarwal87 · 2023-07-20T11:33:39Z

...d-extensions/src/main/java/org/apache/druid/k8s/overlord/common/KubernetesOverlordUtils.java

@@ -43,4 +45,10 @@ public static String convertTaskIdToK8sLabel(String taskId)
    return taskId == null ? "" : StringUtils.left(RegExUtils.replaceAll(taskId, K8S_TASK_ID_PATTERN, "")
        .toLowerCase(Locale.ENGLISH), 63);
  }
+
+  public static String convertTaskIdToJobName(String taskId)


whats the limit on length of job name?

https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/ (its 63 characters)

Do we still need to do this conversion if taskId is less than 63 chars? I mean whether it's valuable to keep taskId same as JobName if it's possible.

we do need to since there are some characters in task ids that are not valid in k8s labels/job names

YongGang · 2023-07-20T20:42:35Z

...d-extensions/src/main/java/org/apache/druid/k8s/overlord/common/KubernetesOverlordUtils.java

@@ -43,4 +45,10 @@ public static String convertTaskIdToK8sLabel(String taskId)
    return taskId == null ? "" : StringUtils.left(RegExUtils.replaceAll(taskId, K8S_TASK_ID_PATTERN, "")
        .toLowerCase(Locale.ENGLISH), 63);
  }
+
+  public static String convertTaskIdToJobName(String taskId)


Do we still need to do this conversion if taskId is less than 63 chars? I mean whether it's valuable to keep taskId same as JobName if it's possible.

YongGang · 2023-07-20T22:13:35Z

...lord-extensions/src/main/java/org/apache/druid/k8s/overlord/common/KubernetesPeonClient.java

                + " not found");
          },
          DruidK8sConstants.IS_TRANSIENT, quietTries, maxTries
      );
    }
    catch (Exception e) {
-      throw new KubernetesResourceNotFoundException("K8s pod with label: job-name=" + k8sTaskId + " not found");
+      throw new KubernetesResourceNotFoundException("K8s pod with label: job-name=" + jobName + " not found");


If we pass jobName instead of K8sTaskId, we lose the originalTaskId in the log, not sure if it's useful for troubleshooting.

hmm i think it might be, although technically the k8sTaskId being logged right now is not really the real one b/c of the bug i mentioned above. maybe i can find a way to get the actual task id in there

suneet-s · 2023-07-21T21:34:28Z

...d-extensions/src/main/java/org/apache/druid/k8s/overlord/common/KubernetesOverlordUtils.java

+  public static String convertTaskIdToJobName(String taskId)
+  {
+    return taskId == null ? "" : StringUtils.left(RegExUtils.replaceAll(taskId, K8S_TASK_ID_PATTERN, "")
+        .toLowerCase(Locale.ENGLISH), 30) + "-" + Hashing.murmur3_128().hashString(taskId, StandardCharsets.UTF_8);


Hashing.murmur3_128().hashString(taskId, StandardCharsets.UTF_8)

How many characters will this produce? I couldn't easily figure out if there is a limit on the length of the string returned from this function

128 bits -> 32 characters

suneet-s · 2023-07-21T21:39:33Z

...lord-extensions/src/main/java/org/apache/druid/k8s/overlord/common/KubernetesPeonClient.java

-      K8sTaskId taskId = new K8sTaskId(job.getMetadata().getName());
-      log.info("Successfully submitted job: %s ... waiting for job to launch", taskId);
+      String jobName = job.getMetadata().getName();
+      log.info("Successfully submitted job: %s ... waiting for job to launch", jobName);


It would be good to report the taskId in this log line, to make it easy to map to the task_id that is being started.

georgew5656 added 3 commits July 19, 2023 14:33

Fix issue with long data source names

feadcea

Merge branch 'master' of github.com:georgew5656/druid into fixLongDat…

905b2da

…aSourceNames

Use the regular library

0a42702

kfaraz added the Kubernetes label Jul 20, 2023

abhishekagarwal87 reviewed Jul 20, 2023

View reviewed changes

YongGang reviewed Jul 20, 2023

View reviewed changes

suneet-s reviewed Jul 21, 2023

View reviewed changes

fix overlord utils test

0d2d0e9

suneet-s approved these changes Jul 21, 2023

View reviewed changes

suneet-s merged commit 28914bb into apache:master Jul 24, 2023

LakshSingla added this to the 28.0 milestone Oct 12, 2023

LakshSingla mentioned this pull request Nov 4, 2023

[DRAFT] 28.0.0 release notes #15326

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with long data source names #14620

Fix issue with long data source names #14620

georgew5656 commented Jul 19, 2023 •

edited

Loading

abhishekagarwal87 Jul 20, 2023

georgew5656 Jul 20, 2023

YongGang Jul 20, 2023

georgew5656 Jul 21, 2023

YongGang Jul 20, 2023

YongGang Jul 20, 2023

georgew5656 Jul 21, 2023

suneet-s Jul 21, 2023

georgew5656 Jul 21, 2023

suneet-s Jul 21, 2023

Fix issue with long data source names #14620

Fix issue with long data source names #14620

Conversation

georgew5656 commented Jul 19, 2023 • edited Loading

Description

Release note

Key changed/added classes in this PR

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgew5656 commented Jul 19, 2023 •

edited

Loading