Keep docker & k8s pod annotations while they are needed #5084
Conversation
exekias
added
bug
in progress
exekias
changed the title
|
@dengqingpei1990 this PR should fix #4986, just in case you want to give this a try before we merge it. From my local tests, it seems to work nice |
There was a problem hiding this comment.
Few corner scenarios are breaking with this change. One, I have left comment on the method itself. Other is as follows:
Take this scenario. User deploys under namespace 1 a stateful set a pod that will come up as zk-1, zk-2, zk-3.
In metricbeat, we use IpPort index and PodName indexes. If a pod in a stateful set get restarted/recreated all together, they are going to come up with the same name and in most scenarios the same ip port combination.
When we look up pods in this change, we look at the deleted cache first and then goto the existing pod workloads. It should ideally be the other way around. If a stateful set pod is updated with new set of labels, the index keys will remain the same but the metadata would have changed.
Both scenarios should ideally be fixable by checking live pod metadata first and returning from that and then check dead pods.
| p.annotationCache.RLock() | ||
| meta, ok := p.annotationCache.annotations[arg] | ||
| var deleted bool | ||
| if ok { |
There was a problem hiding this comment.
For logs, we can update the pod labels using kubectl label po. The container name remains the same and the update will call onPodDelete && onPodAdd. So now the living container's metadata is in the deleted map. This portion of code will end up returning old metadata.
There was a problem hiding this comment.
Ideally we should first check metadata from live pods and return data for that.
There was a problem hiding this comment.
I've added a commit addressing this, basically, if an add method is called it will remove any deleted flag for all its indexes
|
good point, I've addressed the issue 👍 |
| w.containers[event.Actor.ID] = &Container{ | ||
| ID: event.Actor.ID, | ||
| Name: name, | ||
| Image: image, | ||
| Labels: event.Actor.Attributes, | ||
| } | ||
| w.Unlock() | ||
| } | ||
|
|
||
| // Delete | ||
| if event.Action == "die" || event.Action == "kill" { |
There was a problem hiding this comment.
If a container dies and gets started again it will still time out eventually and lose the metadata.
There was a problem hiding this comment.
I think that case is treated, as onPodAdd (also called in case of update) cleans any deleted flag: https://github.com/elastic/beats/pull/5084/files#diff-c211c9481fb346b82d8713e779e0be9fR141
There was a problem hiding this comment.
Sorry you are right, I was talking about the Kubernetes watcher, not Docker, will apply the same fix, thanks!
I'm also working on tests
There was a problem hiding this comment.
When I restart a container, the events are: kill -> die -> stop -> start -> restart. So the container will not be un-deleted in this case.
Looking at it, adding the metadata on "created" and removing on "die" does seem inconsistent to me.
There was a problem hiding this comment.
Let me do some tests, looks like it would be better to add it on start, isn't it?
There was a problem hiding this comment.
I think adding on start is the better approach.
exekias
force-pushed
the
fix-k8s-log-ending-lines
branch
from
162f58c to
b54187e
Compare
exekias
added
needs_backport
exekias
force-pushed
the
fix-k8s-log-ending-lines
branch
5 times, most recently
from
1ea2445 to
cc7260e
Compare
|
jenkins retest this |
| }, nil | ||
| } | ||
|
|
||
| // Container returns the running container with the given ID or nil if unknown | ||
| func (w *watcher) Container(ID string) *Container { | ||
| return w.containers[ID] | ||
| w.RLock() | ||
| container := w.containers[ID] |
There was a problem hiding this comment.
I'm aware the code was here before, but should we check here if ok if the ID actually exists?
There was a problem hiding this comment.
We get nil if the container is not there, and we treat that case
| var toDelete []string | ||
| timeout := time.Now().Add(-w.cleanupTimeout) | ||
| w.RLock() | ||
| for key, lastSeen := range w.deleted { |
There was a problem hiding this comment.
should we add some debug logging to this? Otherwise very hard to figure out what is still in memory and what was removed.
| // Check entries for timeout | ||
| var toDelete []string | ||
| timeout := time.Now().Add(-p.cleanupTimeout) | ||
| p.annotationCache.RLock() |
There was a problem hiding this comment.
Not for this PR, but the code looks so similar to the other cleanupWorker, I'm thinking if parts could be shared.
In some cases pod annotations are neede after the pod is deleted, for instance when filebeat is reading the log behind the container. This change makes sure we keep metadata after a pod is gone. By storing access times we ensure that it's available as long as it's being used
exekias
force-pushed
the
fix-k8s-log-ending-lines
branch
from
6357cec to
7d3314e
Compare
|
done, thanks for the review everyone! |
exekias
removed
the
needs_backport
In some cases pod annotations are neede after the pod is deleted, for instance when filebeat is reading the log behind the container. This change makes sure we keep metadata after a pod is gone. By storing access times we ensure that it's available as long as it's being used (cherry picked from commit a49463c)
In some cases pod annotations are neede after the pod is deleted, for instance when filebeat is reading the log behind the container. This change makes sure we keep metadata after a pod is gone. By storing access times we ensure that it's available as long as it's being used (cherry picked from commit a49463c)
elastic#5133) In some cases pod annotations are neede after the pod is deleted, for instance when filebeat is reading the log behind the container. This change makes sure we keep metadata after a pod is gone. By storing access times we ensure that it's available as long as it's being used (cherry picked from commit d24a88a)
In some cases pod annotations are neede after the container/pod is deleted, for
instance when filebeat is reading the log behind the container.
This change makes sure we keep metadata after a pod is gone. By storing
access times we ensure that it's available as long as it's being used
fixes #4986