You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "georgew5656 (via GitHub)" <gi...@apache.org> on 2023/08/04 16:49:55 UTC
[PR] Add pod name to TaskLocation for easier observability and debugging. (druid)
georgew5656 opened a new pull request, #14758:
URL: https://github.com/apache/druid/pull/14758
Adds pod name to the TaskLocation object for Kubernetes task scheduling to make debugging easier.
### Description
Since the task id <-> kubernetse pod name mapping is not direct and hard to figure out, its helpful for debugging to show the pod name in the druid console. I think it makes sense to add this to task location since the pod the task is running on is sort of like the location when running tasks in K8s.
I also added a log message on the overlord when the task location is set because this is useful for forensics (taskLocation is cleared when the task finishes)
#### Fixed the bug ...
#### Renamed the class ...
#### Added a forbidden-apis entry ...
I thought about just putting the pod name into the task object itself, but i liked the idea of having it in taskLocation in case we ever have some concept of retrying tasks on pods (which would result in a task running as a different pod name)
#### Release note
- Surface kubernetes pod name in the K8sTaskRunner to improve visibility and forensics.
##### Key changed/added classes in this PR
* TaskLocation
* KubernetesPeonLifecycle
This is what the task location looks like in the druid console now.
<img width="1405" alt="Screenshot 2023-08-04 at 10 54 24 AM" src="https://github.com/apache/druid/assets/17736581/3c5a01dc-78eb-496a-a797-894fe8b31788">
If the field is null (e.g. when not running the k8s task runner) nothing will show up under k8sPodName.
This PR has:
- [X] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (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](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] 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](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [X] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add pod name to TaskLocation for easier observability and debugging. (druid)
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on code in PR #14758:
URL: https://github.com/apache/druid/pull/14758#discussion_r1284961786
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesPeonLifecycle.java:
##########
@@ -250,8 +250,10 @@ protected TaskLocation getTaskLocation()
podStatus.getPodIP(),
DruidK8sConstants.PORT,
DruidK8sConstants.TLS_PORT,
- Boolean.parseBoolean(pod.getMetadata().getAnnotations().getOrDefault(DruidK8sConstants.TLS_ENABLED, "false"))
+ Boolean.parseBoolean(pod.getMetadata().getAnnotations().getOrDefault(DruidK8sConstants.TLS_ENABLED, "false")),
+ pod.getMetadata() != null ? pod.getMetadata().getName() : ""
);
+ log.info(StringUtils.format("K8s task %s is running at location %s", taskId.getOriginalTaskId(), taskLocation));
Review Comment:
It is better not to use StringUtils.format in the log message directly as this will cause the string to be formatted even if the log level is set to a higher level like `warn`
```suggestion
log.info("K8s task %s is running at location %s", taskId.getOriginalTaskId(), taskLocation);
```
Also, would a better place for this log line be in the `K8sTaskId` constructor? That way we are guaranteed that the log will be written once for any task with the original task id and the k8sJobName, even if getTaskLocation is never called.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add pod name to TaskLocation for easier observability and debugging. (druid)
Posted by "georgew5656 (via GitHub)" <gi...@apache.org>.
georgew5656 commented on code in PR #14758:
URL: https://github.com/apache/druid/pull/14758#discussion_r1285877373
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesPeonLifecycle.java:
##########
@@ -250,8 +250,10 @@ protected TaskLocation getTaskLocation()
podStatus.getPodIP(),
DruidK8sConstants.PORT,
DruidK8sConstants.TLS_PORT,
- Boolean.parseBoolean(pod.getMetadata().getAnnotations().getOrDefault(DruidK8sConstants.TLS_ENABLED, "false"))
+ Boolean.parseBoolean(pod.getMetadata().getAnnotations().getOrDefault(DruidK8sConstants.TLS_ENABLED, "false")),
+ pod.getMetadata() != null ? pod.getMetadata().getName() : ""
);
+ log.info(StringUtils.format("K8s task %s is running at location %s", taskId.getOriginalTaskId(), taskLocation));
Review Comment:
we will not have the correct pod name in the constructor
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add pod name to TaskLocation for easier observability and debugging. (druid)
Posted by "georgew5656 (via GitHub)" <gi...@apache.org>.
georgew5656 commented on code in PR #14758:
URL: https://github.com/apache/druid/pull/14758#discussion_r1285877373
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesPeonLifecycle.java:
##########
@@ -250,8 +250,10 @@ protected TaskLocation getTaskLocation()
podStatus.getPodIP(),
DruidK8sConstants.PORT,
DruidK8sConstants.TLS_PORT,
- Boolean.parseBoolean(pod.getMetadata().getAnnotations().getOrDefault(DruidK8sConstants.TLS_ENABLED, "false"))
+ Boolean.parseBoolean(pod.getMetadata().getAnnotations().getOrDefault(DruidK8sConstants.TLS_ENABLED, "false")),
+ pod.getMetadata() != null ? pod.getMetadata().getName() : ""
);
+ log.info(StringUtils.format("K8s task %s is running at location %s", taskId.getOriginalTaskId(), taskLocation));
Review Comment:
we will not have the correct pod name in the constructor, i think that would be too many logs as well potentially
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add pod name to TaskLocation for easier observability and debugging. (druid)
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s merged PR #14758:
URL: https://github.com/apache/druid/pull/14758
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add pod name to TaskLocation for easier observability and debugging. (druid)
Posted by "georgew5656 (via GitHub)" <gi...@apache.org>.
georgew5656 commented on code in PR #14758:
URL: https://github.com/apache/druid/pull/14758#discussion_r1285906574
##########
processing/src/test/java/org/apache/druid/indexer/TaskLocationTest.java:
##########
@@ -54,6 +54,18 @@ public void testTlsForPeonJobs()
Assert.assertEquals(2, tls.getTlsPort());
}
+ @Test
+ public void testDefaultK8sJobName()
+ {
+ TaskLocation noK8sJobName = TaskLocation.create("foo", 1, 2, false);
+ Assert.assertNull(noK8sJobName.getK8sPodName());
+ noK8sJobName = TaskLocation.create("foo", 1, 2);
+ Assert.assertNull(noK8sJobName.getK8sPodName());
+
+ TaskLocation k8sJobName = TaskLocation.create("foo", 1, 2, false, "job-name");
+ Assert.assertEquals("job-name", k8sJobName.getK8sPodName());
Review Comment:
fixed the tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add pod name to TaskLocation for easier observability and debugging. (druid)
Posted by "YongGang (via GitHub)" <gi...@apache.org>.
YongGang commented on code in PR #14758:
URL: https://github.com/apache/druid/pull/14758#discussion_r1284737250
##########
processing/src/main/java/org/apache/druid/indexer/TaskLocation.java:
##########
@@ -30,21 +31,29 @@
public class TaskLocation
{
- private static final TaskLocation UNKNOWN = new TaskLocation(null, -1, -1);
+ private static final TaskLocation UNKNOWN = new TaskLocation(null, -1, -1, null);
@Nullable
private final String host;
private final int port;
private final int tlsPort;
+ @Nullable
+ private final String k8sPodName;
Review Comment:
Should we use a more generic field name here instead of named it after `k8s`? Maybe just name it `jobName`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add pod name to TaskLocation for easier observability and debugging. (druid)
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on code in PR #14758:
URL: https://github.com/apache/druid/pull/14758#discussion_r1284961786
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesPeonLifecycle.java:
##########
@@ -250,8 +250,10 @@ protected TaskLocation getTaskLocation()
podStatus.getPodIP(),
DruidK8sConstants.PORT,
DruidK8sConstants.TLS_PORT,
- Boolean.parseBoolean(pod.getMetadata().getAnnotations().getOrDefault(DruidK8sConstants.TLS_ENABLED, "false"))
+ Boolean.parseBoolean(pod.getMetadata().getAnnotations().getOrDefault(DruidK8sConstants.TLS_ENABLED, "false")),
+ pod.getMetadata() != null ? pod.getMetadata().getName() : ""
);
+ log.info(StringUtils.format("K8s task %s is running at location %s", taskId.getOriginalTaskId(), taskLocation));
Review Comment:
It is better not to use StringUtils.format in the log message directly as this will cause the string to be formatted even if the log level is set to a higher level like `warn`
```suggestion
log.info("K8s task %s is running at location %s", taskId.getOriginalTaskId(), taskLocation);
```
##########
processing/src/main/java/org/apache/druid/indexer/TaskLocation.java:
##########
@@ -30,21 +31,29 @@
public class TaskLocation
{
- private static final TaskLocation UNKNOWN = new TaskLocation(null, -1, -1);
+ private static final TaskLocation UNKNOWN = new TaskLocation(null, -1, -1, null);
@Nullable
private final String host;
private final int port;
private final int tlsPort;
+ @Nullable
+ private final String k8sPodName;
Review Comment:
I think `k8sJobName` will be a good name here as this is what it is called in the code in `K8sTaskId`
##########
processing/src/test/java/org/apache/druid/indexer/TaskLocationTest.java:
##########
@@ -54,6 +54,18 @@ public void testTlsForPeonJobs()
Assert.assertEquals(2, tls.getTlsPort());
}
+ @Test
+ public void testDefaultK8sJobName()
+ {
+ TaskLocation noK8sJobName = TaskLocation.create("foo", 1, 2, false);
+ Assert.assertNull(noK8sJobName.getK8sPodName());
+ noK8sJobName = TaskLocation.create("foo", 1, 2);
+ Assert.assertNull(noK8sJobName.getK8sPodName());
+
+ TaskLocation k8sJobName = TaskLocation.create("foo", 1, 2, false, "job-name");
+ Assert.assertEquals("job-name", k8sJobName.getK8sPodName());
Review Comment:
nit: it would be better to split this test into 2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add pod name to TaskLocation for easier observability and debugging. (druid)
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on code in PR #14758:
URL: https://github.com/apache/druid/pull/14758#discussion_r1284961786
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesPeonLifecycle.java:
##########
@@ -250,8 +250,10 @@ protected TaskLocation getTaskLocation()
podStatus.getPodIP(),
DruidK8sConstants.PORT,
DruidK8sConstants.TLS_PORT,
- Boolean.parseBoolean(pod.getMetadata().getAnnotations().getOrDefault(DruidK8sConstants.TLS_ENABLED, "false"))
+ Boolean.parseBoolean(pod.getMetadata().getAnnotations().getOrDefault(DruidK8sConstants.TLS_ENABLED, "false")),
+ pod.getMetadata() != null ? pod.getMetadata().getName() : ""
);
+ log.info(StringUtils.format("K8s task %s is running at location %s", taskId.getOriginalTaskId(), taskLocation));
Review Comment:
It is better not to use StringUtils.format in the log message directly as this will cause the string to be formatted even if the log level is set to a higher level like `warn`
```suggestion
log.info("K8s task %s is running at location %s", taskId.getOriginalTaskId(), taskLocation);
```
Also, would a better place for this log line be the `K8sTaskId` constructor? That way we are guaranteed that the log will be written once for any task with the original task id and the k8sJobName, even if getTaskLocation is never called.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add pod name to TaskLocation for easier observability and debugging. (druid)
Posted by "georgew5656 (via GitHub)" <gi...@apache.org>.
georgew5656 commented on code in PR #14758:
URL: https://github.com/apache/druid/pull/14758#discussion_r1285873512
##########
processing/src/test/java/org/apache/druid/indexer/TaskLocationTest.java:
##########
@@ -54,6 +54,18 @@ public void testTlsForPeonJobs()
Assert.assertEquals(2, tls.getTlsPort());
}
+ @Test
+ public void testDefaultK8sJobName()
+ {
+ TaskLocation noK8sJobName = TaskLocation.create("foo", 1, 2, false);
+ Assert.assertNull(noK8sJobName.getK8sPodName());
+ noK8sJobName = TaskLocation.create("foo", 1, 2);
+ Assert.assertNull(noK8sJobName.getK8sPodName());
+
+ TaskLocation k8sJobName = TaskLocation.create("foo", 1, 2, false, "job-name");
+ Assert.assertEquals("job-name", k8sJobName.getK8sPodName());
Review Comment:
k8sJobName != k8sPodName, in this case i think the pod name is more useful but we could add both if needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add pod name to TaskLocation for easier observability and debugging. (druid)
Posted by "georgew5656 (via GitHub)" <gi...@apache.org>.
georgew5656 commented on code in PR #14758:
URL: https://github.com/apache/druid/pull/14758#discussion_r1284785456
##########
processing/src/main/java/org/apache/druid/indexer/TaskLocation.java:
##########
@@ -30,21 +31,29 @@
public class TaskLocation
{
- private static final TaskLocation UNKNOWN = new TaskLocation(null, -1, -1);
+ private static final TaskLocation UNKNOWN = new TaskLocation(null, -1, -1, null);
@Nullable
private final String host;
private final int port;
private final int tlsPort;
+ @Nullable
+ private final String k8sPodName;
Review Comment:
hmm, is there another use case for using this field? it doesn't show up in the console if its not set so i figured its okay to leave it as more specific
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org