-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
enhancement(kubernetes source): Kubernetes partial message merge #2134
Conversation
2e33b06
to
9310817
Compare
Apparently, the |
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
…filters Signed-off-by: MOZGIII <mike-n@narod.ru>
99982c3
to
24fcf58
Compare
The kube test is sufficient, although I am curious if it's passing with Docker runtime and non Docker runtime. @MOZGIII Have you perhaps tested them on both? |
I wasn't able to successfully run the integration test locally. Docker tests that we have are hard to use for me, and it also got no success with my live k8s cluster. I have doubts about the test procedure I added - I suspect echoing 64k of data won't work if it's passed as an argument. So, I'd really like to test it. |
@LucioFranco @ktff mind helping or considering ways we can improve isolate these tests? |
@MOZGIII |
I tried this command: cargo test --package vector --features kubernetes-integration-tests --lib -- --nocapture sources::kubernetes::test::kube_partial But I get "no such file or dir" errors... It seems like tests (and vector) need access to the cluster filesystem. This doesn't work with Maybe I'm just doing something wrong. |
@MOZGIII I just tested locally with minikube with the kvm driver and it seems to work, the test ends up hanging looks like its just making a lot of 200ok requests. In the past I ran our tests via virtualbox and it worked fine. So not sure what is happening. |
This is an error sample. |
@MOZGIII where are you running this test and where does your kube config file live? Does kubectl work for you? If kubectl works, vector should too. Because I am 100% not running into that error I can actually reach kube. I just ran |
I have the Thanks for bringing this up the kubeconfig file location, I forgot this could be a problem! |
@MOZGIII oh that looks like something that is missing from kubers? |
@LucioFranco might be on their end, but idk yet. |
Checked out
I'm using a workaround, thx everyone for the help! |
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
I can't make the integration test pass, but I assume this is ok cause there's this comment: https://github.com/timberio/vector/blob/f22d088230024cce7493f0ac9258baadfd76b4ff/src/sources/kubernetes/test.rs#L78 @ktff what should I do? |
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
Yea, it happens. Two options:
|
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
I have troubles when running new Was able to test via Waiting on review! |
What kind of troubles? If it's relevant, could you post some last 100 lines of the log output here #1635, or open a new issue with it, so I can take a look. |
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.
As noted in the comment about CRI, the detection of partial message in case of a CRI runtime needs to be changed. This is made more complex by not knowing in what runtime we are until the first message. Check this function to see how we are detecting that. Maybe we could add a field to signal if we are in the Docker or CRI runtime.
@@ -38,6 +38,14 @@ of `kube-system`. Which is by default excluded, \ | |||
unless there are any non empty `include` options.\ | |||
""" | |||
|
|||
[sources.kubernetes.options.auto_partial_merge] | |||
type = "bool" | |||
common = true |
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.
Majority of the logs won't be separated, so I wouldn't consider it common.
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 how it is in docker
source, but I don't mind changing this.
@binarylogic what do you think?
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.
Agree that this should be false
. I don't think this is a common option that users should be changing.
.filter_map(move |event| now.filter(event)); | ||
|
||
let stream: Box<dyn Stream<Item = Event, Error = _> + Send> = if self.auto_partial_merge { | ||
let mut transform_merge_partial_events = transform_merge_partial_events(); |
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 approach to merging won't work with CRI runtimes. Implementations of it usually leave newline from the stdin stdout, and don't put it if the log was broken in multiple parts, instead they add a tag which we extract here to a field multiline_tag
.
This is evident if you test it against Kubernetes cluster with CRI runtime, for example microk8s.
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.
Good catch! Thanks for pointing this out. Indeed, we can't merge before this is corrected.
@MOZGIII could you re-request review once this is ready to go? I'm going to remove the review requests for now so we stop getting reminded. |
Closing this in favor of a k8s rework as part of the #2222. |
This PR adds support for partial events merging at kubernetes.
Closes #1718.
Ref fluent/fluent-bit#337.
To do: