fix: exception when retrieving logs from helm resource #1086
Conversation
10ko
force-pushed
the
fix-unhandled-exception-viewing-logs
branch
from
641c18c
to
f6ef5dc
Compare
| * NOTE: selector: {} matches all the pods for the resource, when using the k8s APIs. | ||
| */ | ||
| export function getSelectorFromResource(resource: KubernetesWorkload) { | ||
| let selector = {} |
There was a problem hiding this comment.
I guess my only concern would be the case where the function returns {}. Wondering if no pods are a better default than all the pods. Might also be useful to add a log.debug statement for that case, regardless of how we handle it.
There was a problem hiding this comment.
On point. I am open to both tbh. I am not extremely familiar with selectors and I don't know what's the "expected" default.
There was a problem hiding this comment.
Updated the function to return undefined instead.
Thanks @eysi09
10ko
force-pushed
the
fix-unhandled-exception-viewing-logs
branch
from
f6ef5dc
to
e00693d
Compare
| * NOTE: { selector: undefined } matches no pods for the resource, when using the k8s APIs. | ||
| */ | ||
| export function getSelectorFromResource(resource: KubernetesWorkload) { | ||
| const logger = getLogger() |
There was a problem hiding this comment.
This is not a good idea, we should pass a LogEntry instance to the function.
There was a problem hiding this comment.
I am going to remove this since we'll throw further down.
I am wondering why this is a bad idea thou?
There was a problem hiding this comment.
It's because this completely ignores any nesting and structure in the logs. With garden dev, for example, this would print the log line at the bottom of the terminal and not next to the relevant task log.
| } | ||
| // No selector found, return. | ||
| logger.debug(`No selector found for ${resource.metadata.name}: returning empty selector.`) | ||
| return |
There was a problem hiding this comment.
Hmm, if we return undefined here, how does that play with the getPods? Shouldn't we rather just throw? We don't want to get all of the Pods from a namespace if we're missing a selector.
There was a problem hiding this comment.
So it looks like the k8s javascript library has a class V1LabelSelector which is used in various resources and matches everything if it's empty but nothing if it's undefined.
The function listNamespacedPod(...) called by getPods(...) has a parameter called labelSelector which is, of course, not of type V1LabelSelector and defaults to "all the pods".
I thought the getPods would follow the same behaviour, hence my decision of either returning empty object or undefined.
I'd also rather throw.
10ko
force-pushed
the
fix-unhandled-exception-viewing-logs
branch
from
b7da6ff
to
1faa48d
Compare
|
@edvald, is this one ready? |
| app: resource.metadata.labels.app, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Sorry, could have thought of this earlier, but... Wouldn't it make sense to just use the exact labels from the Pod template (spec.template.metadata.labels)? And maybe leave the last Helm-specific clause as a 3rd fallback? Because if the Pod spec explicitly specifies labels, those will work fine as a selector.
10ko
force-pushed
the
fix-unhandled-exception-viewing-logs
branch
from
1faa48d
to
5e65447
Compare
| return resource.spec.selector.matchLabels | ||
| } | ||
| // We check if the pod template has labels | ||
| if (resource.spec.template.metadata.labels) { |
There was a problem hiding this comment.
This may error if one of the variables doesn't exist. Actually the same applies above as well, even though it's less likely to occur IRL.
10ko
force-pushed
the
fix-unhandled-exception-viewing-logs
branch
from
5e65447
to
5aa4e95
Compare
|
Really looking forward to this. |
What this PR does / why we need it:
Whenever we retrieve logs with
garden logswe assume the resource has aselector. This is not always true when we deal with Helm charts. This PR introduce a new function to determine a selector in case of an Helm chart.Please @edvald or @eysi09, do a sanity check to see if it makes sense. I did some tests with
stable/mysqland some other charts and it "looks" like it's working but yeah, maybe you have some specific scenarios/charts in mind.Which issue(s) this PR fixes:
Fixes #1068
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Well, it fixes an exception.