-
Notifications
You must be signed in to change notification settings - Fork 273
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: exception when retrieving logs from helm resource #1086
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the function to return undefined instead.
Thanks @eysi09
f6ef5dc
to
e00693d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a good idea, we should pass a LogEntry
instance to the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
b7da6ff
to
1faa48d
Compare
@edvald, is this one ready? |
app: resource.metadata.labels.app, | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
5e65447
to
5aa4e95
Compare
Really looking forward to this. |
What this PR does / why we need it:
Whenever we retrieve logs with
garden logs
we 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/mysql
and 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.