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

LOG-1286: Pick daemon names when configuring addLogSource field #1018

Merged
merged 1 commit into from
Jul 28, 2021

Conversation

k-keiichi-rh
Copy link
Contributor

@k-keiichi-rh k-keiichi-rh commented May 11, 2021

Description

The current syslog forwarding can not report who outputted logs in journal log when configuring addLogSource field.
To identify the daemon name, the "systemd.u.SYSLOG_IDENTIFIER" in journal log record is what information we want.
So fluentd can pick their daemon names from journal log records.

This PR enhances the addLogSource field in Log Forwarding API by splitting infrastructure stream into journal log and pod log and injecting daemon names into the journal log stream.
So this change is limited to the addLogSource field related code.

This PR picks the originating process name and id(known as the "pid") from the record and injects them into the header field.
There are three type messages in the journal log and the process information are injected like the following:

  1. System messages stored in the journal log
    "Tag" and "AppName" fields in CLF are ignored for the system messages when enabling the "AddLogSource" field.
<158>May 18 00:21:27 worker3.cluster.sub.nec.test crio[1738]: time="2021-05-18 00:21:25.958832751Z" level=info msg="About to del CNI network multus
-cni-network (type=multus)"
  1. Kernel messages stored in the journal
    Any daemon information are not injected to the kernel messages.
<158>May 18 00:21:27 worker3.cluster.sub.nec.test kernel: device vethc2d8643c left promiscuous mode
  1. System pod messages not stored in the journal
    Any daemon information are not injected to the pod messages.
    The "fluentd:" is overwritten by specifying "Tag" and "AppName" fields.
<158>May 18 00:23:00 worker4.cluster.sub.nec.test fluentd: namespace_name=openshift-image-registry, container_name=node-ca, pod_name=node-ca-q5m
n5, message=image-registry.openshift-image-registry.svc:5000

/cc @vimalk78 @alanconway

Links

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2021
@k-keiichi-rh
Copy link
Contributor Author

/test e2e-operator

1 similar comment
@k-keiichi-rh
Copy link
Contributor Author

/test e2e-operator

@alanconway
Copy link
Contributor

/approve
Looks good, will lete @vimalk78 give the final LGTM as he's the syslog output expert.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2021
@k-keiichi-rh
Copy link
Contributor Author

@alanconway Thank you for your review. I have an issue for e2e testing in forward_to_syslog_test.go. I will fix it and remove "WIP:".

@k-keiichi-rh
Copy link
Contributor Author

/test lint

@k-keiichi-rh k-keiichi-rh force-pushed the fixsyslog branch 2 times, most recently from 505cc2b to 7814b0e Compare May 18, 2021 01:15
@k-keiichi-rh k-keiichi-rh changed the title WIP: LOG-1286: Pick daemon names when configuring addLogSource field LOG-1286: Pick daemon names when configuring addLogSource field May 18, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 18, 2021
@k-keiichi-rh
Copy link
Contributor Author

/test e2e-operator

1 similar comment
@k-keiichi-rh
Copy link
Contributor Author

/test e2e-operator

Comment on lines 693 to 698
<match journal.** system.var.log**>
@type copy
{{include .StoreTemplate . "pick_daemon_name" | indent 4}}
</match>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont catch using the syslog store template here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infrastructure pod logs and journal logs are sent under "infrastructure".
But the infrastructure pod logs doesn't have $.systemd.u.SYSLOG_IDENTIFIER.
So I splitted "infrastructure" stream into the pod logs and the journal logs and then injected the daemon name into the journal logs only.
This syslog store template is for the journal logs. I will add some comments to explain this.

Copy link
Contributor

@vimalk78 vimalk78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add an e2e/functional test to highlight the use of the changes

@k-keiichi-rh
Copy link
Contributor Author

I will add a e2d testcase for this PR.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2021
@k-keiichi-rh k-keiichi-rh changed the title LOG-1286: Pick daemon names when configuring addLogSource field WIP: LOG-1286: Pick daemon names when configuring addLogSource field Jul 3, 2021
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 3, 2021
@alanconway
Copy link
Contributor

@k-keiichi-rh What's the status of this PR? Is it still WIP or is it ready to consider for merge?

@k-keiichi-rh
Copy link
Contributor Author

@k-keiichi-rh What's the status of this PR? Is it still WIP or is it ready to consider for merge?

I believe that the function is ready to consider for merge.
But the e2e testcase for the function doesn't work well. Now I am trying to fix it.
After finishing the fix, I will remove "WIP".

@k-keiichi-rh k-keiichi-rh force-pushed the fixsyslog branch 2 times, most recently from 1825a66 to b2ff008 Compare July 8, 2021 22:30
@openshift-ci openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2021
@k-keiichi-rh k-keiichi-rh reopened this Jul 17, 2021
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2021
@k-keiichi-rh
Copy link
Contributor Author

/test e2e-gcp

1 similar comment
@k-keiichi-rh
Copy link
Contributor Author

/test e2e-gcp

@alanconway
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 19, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alanconway, k-keiichi-rh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@alanconway
Copy link
Contributor

Hi @k-keiichi-rh, you need to add "Bug 1891886:" in front of the title of this PR so that the CI bot will associate it with the BZ.

@k-keiichi-rh k-keiichi-rh changed the title LOG-1286: Pick daemon names when configuring addLogSource field Bug 1891886: LOG-1286: Pick daemon names when configuring addLogSource field Jul 26, 2021
@openshift-ci openshift-ci bot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Jul 26, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 26, 2021

@k-keiichi-rh: This pull request references Bugzilla bug 1891886, which is invalid:

  • expected the bug to be open, but it isn't
  • expected the bug to target the "4.8.0" release, but it targets "4.7.0" instead
  • expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is CLOSED (ERRATA) instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1891886: LOG-1286: Pick daemon names when configuring addLogSource field

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jul 26, 2021
@k-keiichi-rh
Copy link
Contributor Author

k-keiichi-rh commented Jul 26, 2021

@alanconway Hi. Thank you for the comment. I am a little confused.
This PR is targeted to the master branch, which is for CLO5.2 now I think. So I filed a Jira ticket for this PR.
Do I need to backport this patch to CLO5.1, CLO5.0 and CLO4.7?

@jcantrill jcantrill changed the title Bug 1891886: LOG-1286: Pick daemon names when configuring addLogSource field LOG-1286: Pick daemon names when configuring addLogSource field Jul 27, 2021
@openshift-ci openshift-ci bot removed bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jul 27, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 27, 2021

@k-keiichi-rh: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

LOG-1286: Pick daemon names when configuring addLogSource field

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jcantrill
Copy link
Contributor

/refresh
/bugzilla refresh

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 27, 2021

@jcantrill: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

/refresh
/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jcantrill
Copy link
Contributor

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2021
@jcantrill
Copy link
Contributor

@alanconway Hi. Thank you for the comment. I am a little confused.
This PR is targeted to the master branch, which is for CLO5.2 now I think. So I filed a Jira ticket for this PR.
Do I need to backport this patch to CLO5.1, CLO5.0 and CLO4.7?

Is this a bug? If so, how critical is it that it gets into 5.0 or even 4.6? Can we simply backport to 5.1 and ask user's to upgrade? We would prefer to backport as few releases as possible if user's are capable of working around the issue. If not, the BZ should be for logging 4.6 and we should have JIRA issues for the 5.x code changes

@k-keiichi-rh
Copy link
Contributor Author

Is this a bug? If so, how critical is it that it gets into 5.0 or even 4.6? Can we simply backport to 5.1 and ask user's to upgrade? We would prefer to backport as few releases as possible if user's are capable of working around the issue. If not, the BZ should be for logging 4.6 and we should have JIRA issues for the 5.x code changes

Thank you for the clarification. This is a feature request to 5.2, not a bug. So we don't need to backport to previous versions.

@openshift-merge-robot openshift-merge-robot merged commit e598ed3 into openshift:master Jul 28, 2021
pmoogi-redhat pushed a commit to pmoogi-redhat/cluster-logging-operator that referenced this pull request Apr 26, 2022
 LOG-1286: Pick daemon names when configuring addLogSource field
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants