[Merged by Bors] - Add pod enrichment controller for injecting node addresses#36
[Merged by Bors] - Add pod enrichment controller for injecting node addresses#36
Conversation
|
|
||
| The hostname or IP address of the `Node` that the `Pod` is scheduled to run on. | ||
| Compared to `Pod.status.nodeIP`, this can also (but doesn't have to) be a hostname, and prefers | ||
| externally routable addresses. |
There was a problem hiding this comment.
What do you think about having to separate labels enrichment.stackable.tech/node-ip and enrichment.stackable.tech/node-hostname? This way the product operator or even the product could decides what he prefers or needs.
There was a problem hiding this comment.
We might add a separate node-ip at some point if something requires that specifically, but part of the point here is to abstract away the fact that not all clus†ers have meaningful (resolvable) hostnames for nodes.
| .into_iter() | ||
| .filter(move |pod| { | ||
| pod.spec.as_ref().and_then(|s| s.node_name.as_deref()) | ||
| == node.metadata.name.as_deref() |
There was a problem hiding this comment.
| == node.metadata.name.as_deref() | |
| == Some(&node.name()) |
feel free to ignore
needs ResourceExt import
There was a problem hiding this comment.
Myeah, ResourceExt needs a bit of a revamp at some point: kube-rs/kube#634
soenkeliebau
left a comment
soenkeliebau
left a comment
There was a problem hiding this comment.
Overall looks good to me, just one question (haven't tested it yet, sorry).
Would this annotate all pods that are out there? I didn't find anywhere that this is restricted to something carrying a specific Stackable label or similar?
| pod_restart_controller, | ||
| pod_enrichment_controller | ||
| ); | ||
| futures::future::select( |
There was a problem hiding this comment.
Would using select_all instead of chaining selects have the same effect here?
There was a problem hiding this comment.
Yes, but select_all boxes all the futures, effectively making each of them a black box for the optimizer.
I'm not sure how big the impact of that is here specifically, but…
Yes, on the assumption that the annotation is namespaced anyway, so it shouldn't clash and anything that doesn't care can just ignore the annotation. |
Ok, fine for me. Is there anything to consider around this in an RBAC enabled environment? Would the operator keep throwing errors when trying to annotate pods it doesn't have the right to change or something like that? |
|
That's true, that'd happen for pods that it has permission to read but not patch… |
We need to at some point look into having a stackableconfig configmap I think. We've had a couple of things come up that would be useful to configure centrally over the past few weeks. This is another one of those, being able to configure labelselector to match pods that should get annotated would give us a mechanism to improve this, where needed. |
|
In that case I'd rather just hard-code a labelselector that triggers this.. |
|
Added a filter to only enrich pods with the label |
|
bors r+ |
|
Pull request successfully merged into main. Build succeeded: |
bors
Bot
changed the title
3 participants
Description
Fixes #35
Review Checklist
Once the review is done, comment
bors r+(orbors merge) to merge. Further information