Skip to content
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

Add support for log tags in newest CRI spec #8265

Merged
merged 3 commits into from
Sep 10, 2018

Conversation

exekias
Copy link
Contributor

@exekias exekias commented Sep 7, 2018

CRI spec has changed to include tags (to tell if the line is full or
partial):

2016-10-06T00:17:09.669794202Z stdout F The content of the log entry 1
2016-10-06T00:17:09.669794202Z stdout P First line of log entry 2
2016-10-06T00:17:09.669794202Z stdout P Second line of the log entry 2
2016-10-06T00:17:10.113242941Z stderr F Last line of the log entry 2

In order to support both the previous and this new format, this PR introduces a new flag you can pass to the docker input (cri.parse_flags):

filebeat.inputs:
  - type: docker
    ...
    cri.parse_flags: true

cri.parse_flags is disabled by default, we should flip this to enabled by default in 7.0.

For more details you can check:
https://github.com/kubernetes/community/blob/master/contributors/design-proposals/node/kubelet-cri-logging.md

Fixes #8257

CRI spec has changed to include tags (to tell if the line is full or
partial):

```
2016-10-06T00:17:09.669794202Z stdout F The content of the log entry 1
2016-10-06T00:17:09.669794202Z stdout P First line of log entry 2
2016-10-06T00:17:09.669794202Z stdout P Second line of the log entry 2
2016-10-06T00:17:10.113242941Z stderr F Last line of the log entry 2
```

I still have to determine how this affects the older format, obviously
this change is breaking it, but to be seen if CRI is still supporting
it.

For more details you can check:
https://github.com/kubernetes/community/blob/master/contributors/design-proposals/node/kubelet-cri-logging.md
@exekias exekias added review Filebeat Filebeat needs_backport PR is waiting to be backported to other branches. containers Related to containers use case in progress Pull request is currently in progress. labels Sep 7, 2018
@kvch kvch self-requested a review September 7, 2018 13:30
@ruflin
Copy link
Member

ruflin commented Sep 7, 2018

Could we make it a config option on which version should be used? Like this we could keep 3 as default in 6.x and switch to 4 in 7.0.

@exekias
Copy link
Contributor Author

exekias commented Sep 7, 2018

That makes sense, I will add a parameter, where did you get these numbers from?

@ruflin
Copy link
Member

ruflin commented Sep 7, 2018

https://github.com/elastic/beats/pull/8265/files#diff-d53e98303996a81ae177747740c9f9deR63 (I know, there is more to it then just the split :-)

@exekias
Copy link
Contributor Author

exekias commented Sep 7, 2018

Aah got it, thanks! Yes, I was thinking of cri.parse_tags: true|false, wdyt?

@ruflin
Copy link
Member

ruflin commented Sep 7, 2018

I wonder if users are aware of CRI versions? Will a user know that tags got added to the log? What config will he look for?

+1 on the propose config as I don't have a better suggestion.

One thing we could do later is have a dynamic implementation, meaning that we implement it in a way that with the first log message we detect, for example by checking if there is a valid tag, if it is the old or new CRI.

Setting `cri.parse_flags` will configure the input to parse CRI flags
(for partial lines)
@exekias
Copy link
Contributor Author

exekias commented Sep 10, 2018

I have requested more info about the change here: containerd/cri#830, the format is not really backward compatible, as you cannot tell if the log really started with P ... or that P is a flag in our settings. I guess we will need to stick to the cri.pàrse_tags setting. Probably the old format is deprecated anyway, I'll add a new issue to flip this flag to true in 7.0 to #6106 once merged

@exekias exekias merged commit 0a20f58 into elastic:master Sep 10, 2018
@exekias exekias added v6.4.1 and removed needs_backport PR is waiting to be backported to other branches. labels Sep 10, 2018
@exekias exekias added the v6.5.0 label Sep 10, 2018
exekias added a commit to exekias/beats that referenced this pull request Sep 10, 2018
* Add support for log tags in newest CRI spec

CRI spec has changed to include tags (to tell if the line is full or
partial):

```
2016-10-06T00:17:09.669794202Z stdout F The content of the log entry 1
2016-10-06T00:17:09.669794202Z stdout P First line of log entry 2
2016-10-06T00:17:09.669794202Z stdout P Second line of the log entry 2
2016-10-06T00:17:10.113242941Z stderr F Last line of the log entry 2
```

I still have to determine how this affects the older format, obviously
this change is breaking it, but to be seen if CRI is still supporting
it.

For more details you can check:
https://github.com/kubernetes/community/blob/master/contributors/design-proposals/node/kubelet-cri-logging.md

(cherry picked from commit 0a20f58)
exekias added a commit to exekias/beats that referenced this pull request Sep 10, 2018
* Add support for log tags in newest CRI spec

CRI spec has changed to include tags (to tell if the line is full or
partial):

```
2016-10-06T00:17:09.669794202Z stdout F The content of the log entry 1
2016-10-06T00:17:09.669794202Z stdout P First line of log entry 2
2016-10-06T00:17:09.669794202Z stdout P Second line of the log entry 2
2016-10-06T00:17:10.113242941Z stderr F Last line of the log entry 2
```

I still have to determine how this affects the older format, obviously
this change is breaking it, but to be seen if CRI is still supporting
it.

For more details you can check:
https://github.com/kubernetes/community/blob/master/contributors/design-proposals/node/kubelet-cri-logging.md

(cherry picked from commit 0a20f58)
exekias added a commit that referenced this pull request Sep 11, 2018
* Add support for log tags in newest CRI spec

CRI spec has changed to include tags (to tell if the line is full or
partial):

```
2016-10-06T00:17:09.669794202Z stdout F The content of the log entry 1
2016-10-06T00:17:09.669794202Z stdout P First line of log entry 2
2016-10-06T00:17:09.669794202Z stdout P Second line of the log entry 2
2016-10-06T00:17:10.113242941Z stderr F Last line of the log entry 2
```

I still have to determine how this affects the older format, obviously
this change is breaking it, but to be seen if CRI is still supporting
it.

For more details you can check:
https://github.com/kubernetes/community/blob/master/contributors/design-proposals/node/kubelet-cri-logging.md

(cherry picked from commit 0a20f58)
exekias added a commit that referenced this pull request Sep 11, 2018
* Add support for log tags in newest CRI spec

CRI spec has changed to include tags (to tell if the line is full or
partial):

```
2016-10-06T00:17:09.669794202Z stdout F The content of the log entry 1
2016-10-06T00:17:09.669794202Z stdout P First line of log entry 2
2016-10-06T00:17:09.669794202Z stdout P Second line of the log entry 2
2016-10-06T00:17:10.113242941Z stderr F Last line of the log entry 2
```

I still have to determine how this affects the older format, obviously
this change is breaking it, but to be seen if CRI is still supporting
it.

For more details you can check:
https://github.com/kubernetes/community/blob/master/contributors/design-proposals/node/kubelet-cri-logging.md

(cherry picked from commit 0a20f58)
@exekias
Copy link
Contributor Author

exekias commented Sep 11, 2018

I've pusehd a snapshot for 6.4.1 with this fix to: exekias/filebeat:6.4.1-snapshot

leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
)

* Add support for log tags in newest CRI spec

CRI spec has changed to include tags (to tell if the line is full or
partial):

```
2016-10-06T00:17:09.669794202Z stdout F The content of the log entry 1
2016-10-06T00:17:09.669794202Z stdout P First line of log entry 2
2016-10-06T00:17:09.669794202Z stdout P Second line of the log entry 2
2016-10-06T00:17:10.113242941Z stderr F Last line of the log entry 2
```

I still have to determine how this affects the older format, obviously
this change is breaking it, but to be seen if CRI is still supporting
it.

For more details you can check:
https://github.com/kubernetes/community/blob/master/contributors/design-proposals/node/kubelet-cri-logging.md

(cherry picked from commit c06e91b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
containers Related to containers use case Filebeat Filebeat in progress Pull request is currently in progress. review v6.4.1 v6.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants