-
Notifications
You must be signed in to change notification settings - Fork 101
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
new pod logstream not getting created with mounted-file option #289
Comments
@Cryptophobia Can you please help with the issue , seems like the new container information is not updated in "fluent.conf" until the fluent is refreshed manually. |
@vkadi , can you share the configMap or CRD fluentdconfig that you are using for the above testing container? Are you using type Also it is possible, when your configuration changes the fluentd container needs to be restarted as gracefulReload may not be enough. Let me know if you can provide the detail above and I can test with a similar configmap or CRD. |
@Cryptophobia Yes, I am using the "mounted-file" and this is how my configmap looks for the testing pod -
|
@Cryptophobia Note that fluentd logs does show the new pod such as: |
Thank you @sabdalla80 , that description helps a lot! I think in this case maybe we are not doing it correctly. Since the config file has not change, we only force fluentd reload when config hashes change. We need to force gracefulReload on fluentd even when configmap does not change but the mountedFile pod is restarted/replaced. This sounds like the issue here. This also explains why KFO sometimes does not work with job type pods. |
I think this is related to the issues in #253 |
@Cryptophobia Thank you for the response and feedback. It sounds like others having same issue, I know this issue is currently impacting me and I can use a quick fix. Do you know if you have an ETA for the fix? I would be glad to assist as I already went down this path of learning the insides of the code and got hung up on how to detect pod changes (I am not 100% sure, but it sounds like it's done in KubeInformer). Please let me know as I can help with testing the fix as well since I already have an environment with this issue ready for testing. |
@sabdalla80 , yes I think we need to find some place in @sabdalla80 Before we start looking at code fix can you verify if this is not a regression for me? There are a couple lines of code were deleted that were thought to be useless. Can you run the The line deleted in question is this commit: https://github.com/vmware/kube-fluentd-operator/pull/227/files |
@Cryptophobia I tried it against 1.15.0 and 1.13.0, it has the same issue. |
I just looked at the code and I have an idea. We can use an hasher to get a unique has for the nsConfig struct each time RunOnce() loop runs. This will avoid us having to reload each time new pods restart or using kubeInformer at all. We just build namespace representation using our current way. If the podlist slice representation in nsConfig.MiniContainers changes then the hash value of the whole nsConfig struct will change and we can issue reload. We can use this for our hasher: EDIT: The true problem stems from the fact that mount-file names change based on the container names, but forcing more reloads will make KFO more resilient and this is the best way to solve the problem I think. |
@sabdalla80, thank you for testing. After I do some local testing, I will build a new test image today. |
This would be spectacular. @Cryptophobia If you can share any code/branch you have started, I can contribute to it if it helps to bring to completion. |
@sabdalla80 , When I do a simple test with 3 pods in a deployment. I cannot reproduce this. You are saying the mountedfile:Process() is not being called. Can you be more specific and in which part of the code? |
@Cryptophobia Are you able to see main control loop re-run when you restart/replace a pod in the deployment? Does it re-generate fluentd.conf file? |
No, the main control loop does not rerun. But I see fluentd picks up new pods and new container names when the containers restarts/scale-up or whatever. This seems like a race-condition with logstream for CW? |
@sabdalla80 , do you think if we force fluentd to reload this will be fixed? This seems like CW logstreams expect to find new names of pods and it is not being forced because of race condition? |
@Cryptophobia In intial deployment things work fine, the control loop generates fluentd.conf and proper CM files for a service. On pod restart, even though fluentd indeed sees the new pod, the fluend.conf and CM files are not re-generated, they become stale and don't point correctly to the new pods. For example: this is what's generated in fluent.conf file in deployment for mounted-file pod. When mounted-file pod restarts, this file isn't changed.
|
@sabdalla80 , thank you for this. I think I understand the problem. The CW plugin requires that the container name be known ahead of time so even though we make correct fluentd configs from reloader when fluentd does not restart then the CW plugin configuration is not the correct one loaded. I am building a container for you to test this theory. The fix is basically to reload anytime we have new pods. I think this idea will make KFO more robust anyways. We will see in practice if this is desired. |
It would be awesome. You are spot on in your description. |
@sabdalla80 , I still can't produce a valid test with the similar preprocessing block as your config. Can you post the fluentd-config you are using for the namespace and the deployment yaml that you are deploying for this mount-file test? |
Can you try testing with this image when you have some time?
EDIT: Image is still being pushed up, give it 2 mins. |
@sabdalla80 , I am able to reproduce the preprocessing block now. I see when I scale up from 3 to 6 pods, the preprocessing still stay to 3 blocks... |
with I did see reloader log this event this time around on mounted-file pod restart. But, I did not see fluentd.conf file update. Are you not running the whole chain of events such as generator::renderToDisk in your new changes?
|
I think I solved it @sabdalla80 ! By making a function with a channel in the Run() loop (moving it out of RunOnce()) so that it does the check now and does not rely on the KubeInformer update channels, but does it outside of the informers. This was to save replicating the code across all of the different type of informers. It's a hacky solution, but if it works, it will solve all the preprocessing race conditions problems we may have in future too. 😄 Please test when you can:
Here was the solution:
|
@sabdalla80 I will wait for your test with container before I make PR:
|
@Cryptophobia I am still seeing the same behavior. It seems as if In contrast to |
@Cryptophobia Great news! I have verified that |
@sabdalla80, Great news! Tomorrow, I will make and publish a new minor release |
@sabdalla80 , apologies to bother you today. I had a realization this morning that we do not need to compare hashes of all pods and store those at all. We can just rely on config hashes changing per namespace, since preProcessors are part of renderToDisk function the config for each namespace will include the preprocessing directives. I removed a bunch of useless code I had and now it is a much leaner solution (no more hashing structures of pods that may not be related to any configs). Can you please just verify that it all works when you scale up and down with configmaps type? I have verified crd type works again. Would appreciate a second pair of eyes on this before making the PR and release.
|
Certainly! I Will do shortly and will report back.
…________________________________
From: AMO ❤️☕ ***@***.***>
Sent: Wednesday, November 24, 2021 1:01:34 PM
To: vmware/kube-fluentd-operator ***@***.***>
Cc: Sam A. ***@***.***>; Mention ***@***.***>
Subject: Re: [vmware/kube-fluentd-operator] new pod logstream not getting created with mounted-file option (Issue #289)
@sabdalla80<https://github.com/sabdalla80> , apologies to bother you today. I had a realization this morning that we do not need to compare hashes of all pods and store those at all. We can just rely on config hashes changing per namespace, since preProcessors are part of renderToDisk function the config for each namespace will include the preprocessing directives.
I removed a bunch of useless code I had and now it is a much leaner solution (no more hashing structures of pods that may not be related to any configs).
Can you please just verify that it all works when you scale up and down with configmaps type? I have verified crd type works again. Would appreciate a second pair of eyes on this before making the PR and release.
vmwaresaas.jfrog.io/vdp-public/upstream/kube-fluentd-operator:sabdalla80-test-4
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#289 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAUNLXHHKQDDYDV7NFGJTXTUNUR75ANCNFSM5ICHBFKA>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
- add kube pod informer for pods add/rm in namespace we monitor, rerun preprocess configs - fixes vmware#289 - fixes vmware#253 - fixes issue with respecting `c.cfg.AnnotConfigmapName` on configmap namespaces discovery - introduces `hashstructure` package for smarter hashing of changing objects - make smarter hashing for `allConfigsHash` metahashing (without `reflect.DeepEqual()`) Signed-off-by: Anton Ouzounov <[email protected]>
@Cryptophobia I like the cleanup - looks good. |
New |
Hey @sabdalla80, Unfortunately, this needs to be reverted for now. We noticed that too many gracefulReload calls are causing the fluentd plugins to break and fluentd gets stuck. Fluentd sits in a bad state so in very huge clusters with lots of configs this is a problem. I think fluentd just gets in a bad state if too many gracefulReloads come in all at once. I have no other explanation. Which means we need to be way smarter when we decide to reload. We need to be even more deliberate… We can’t just reload all the time when new pods come in and out… we will need to somehow keep track if those pods are in the preprocessing directives, if they are, then reload… I will most likely need to modify preprocessor function to return actual pod names and container ids of the pods that are in the preprocessing config. That way the controller will track them. Then when pods come in only reload if that pod matches the previous config's container ids. I will go back to the drawing board and look at other ways of fixing this. |
@Cryptophobia Sorry about the late response, this got lost in my email box somehow. I would like to collaborate with you on the fix as needed. Getting this right is critical to us. Besides the increase in reloads, how did you find out that fluentd was getting hung? Ideally, on the update front, we don't want to reload everytime a pod changes, we just want to reload only if a mounted_file pod has been updated. |
Log volumes got really low (see picture above) and when inspecting the fluentd containers, we saw that we don't get normal debug/info logs from fluentd but rather it was stuck after the gracefulReload API call. Upon further inspection of the logs in the fluentd container, it looks like fluentd is taking a lot of processing power to release the buffers/load/unload plugins and then becomes stuck in that data pipeline flush code (suspecting this is the case). Fluentd cannot keep up with all of the API calls to reload when using too many plugins and too many configs. It could also be that it tries to flush too many file buffers from our fluentd configuration files per namespaces... (another possibility).
Yes, this is exactly the point. We need a better solution that only reloads when mount_file pods are updated. Only call the update event in the pod reloader when a container that has mount_file directive (in preprocessors) to regenerate configs. This will lower the number of reload calls to the absolute requirement to make it all work without DOS on fluentd pod's API. |
@Cryptophobia I am going to start working on it today. I am going to figure out how to reload only when mounted_file pod restarted. If you already thought about how the code may look like, please share. I will also need your help when fix is in so you can run the same tests you ran to ensure fluentd is not getting hung with too many reload calls. |
@Cryptophobia Can you provide more input on the behavior you saw, I was not able to reproduce. I deployed a mounted-file pod and a standard pod (not mounted-file), and I observed the standard pod does not actually execute graceful reload calls (but it does go thru the On a side note: On ConfigMap changes or Pod changes(now), |
@Cryptophobia I made some changes so that we only run controller and reload when pod changes is associated with namespace/config that has mounted-file type. I am doing this in kube_informer and not in controller to avoid the I appreciate your help on this to help unblock me as I am not able to proceed forward until I verify there are no issues with v16.2 |
@Cryptophobia Can you help reviewing the above changes? |
Current solution in 1.16.5 is not affordable for big clusters. |
Hi All |
) Signed-off-by: huskykurt <[email protected]>
Signed-off-by: huskykurt <[email protected]>
…d-file is created or deleted (#347) * Reload configuration only if changed pod with mounted labels (#289) * add test for util/Match function (#289) Signed-off-by: huskykurt <[email protected]>
I am seeing the issue with CW logstreams not getting created when using the "mounted-file" to ingest the logs from file in the container.
I picked the sample config from examples folder and able to reproduce it , below is the demo config what I have used.
apiVersion: apps/v1$((var++)) to file"; sleep 2; [[ $ (($var % 100)) == 0 ]] && :> /var/log/welcome.log ;done > /var/log/welcome.log
kind: Deployment
metadata:
name: nginx-deployment
namespace: default
spec:
selector:
matchLabels:
app: nginx
replicas: 1
template:
metadata:
labels:
app: nginx
spec:
containers:
- image: ubuntu
name: test-container
command:
- bash
- -c
- while true; do echo
date -R
[INFO] "Random welcome numbervolumeMounts:
- mountPath: /var/log
name: logs
- mountPath: /host-root
name: root
volumes:
- name: logs
emptyDir: {}
- name: root
hostPath:
path: /
Sometime when we refresh the fluent then we will see the logstreams but its not consistent either. Anyone using the "mounted-file" option heavily and saw this issue. Any guidance will be a great help around this area.
Note : Using the latest KFO version (1.16.1)
The text was updated successfully, but these errors were encountered: