LOG-5186: Refactor input filter for includes and excludes #2404

cahartma · 2024-03-13T22:21:47Z

Description

Some local tests are still failing so doubtful this will pass all of CI yet, but need to get this moving, so here goes. Please review and comment!

/cc @Clee2691 @vparfonov
/assign @jcantrill

Refactor the custom application include/exclude to change from:

    inputs:
    - application:
        namespaces:
        - app-ns    
        excludeNamespaces:
        - test*
        containers:
          include:
          - nginx     
          exclude:
          - logger

to

    inputs:
    - application:
      - includes:
        - namespace: app-ns
          container: "*"
        - container: nginx
          namespace: "*"
      - excludes:
        - namespace: "test*"
          container: "*"
        - container: logger
          namespace: "*"

Acceptance Criteria:

Verify when an include/exclude entry only defines "namespace" that it applies to all containers (e.g. *)
Verify when an include/exclude entry only defines "container" that it applies to all namespaces (e.g. *)
Verify multiple includes combinations results in logs included from each of the matches
Verify multiple excludes combinations results in logs excluded from each of the matches
Verify only includes that are not in the excludes list are matched. (i.e. excludes take precedence and are run first)
Verify the CEE documentation is updated to reflect the changes
Verify the enhancement documentation is updated to reflect the changes
Verify "namespaces" field as marked as deprecated
Verify the entries of the "namespaces" field are added to the list of "includes" in the generated configuration
Verify related/linked bugs are resolved

TODO:

create upstream docs
update CEE slides

Links

openshift-ci-robot · 2024-03-13T22:21:50Z

@cahartma: This pull request references LOG-5186 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.8.0" version, but no target version was set.

In response to this:

Description

Some local tests are still failing so doubtful this will pass all of CI yet, but need to get this moving, so here goes. Please review and comment!

/cc @Clee2691 @vparfonov
/assign @jcantrill

Refactor the custom application include/exclude to change from:
   inputs:
   - application:
       namespaces:
       - app-ns    
       excludeNamespaces:
       - test*
       containers:
         include:
         - nginx     
         exclude:
         - logger          
to
   inputs:
   - application:
     - includes:
       - namespace: app-ns
         container: "*"
       - container: nginx
         namespace: "*"
     - excludes:
       - namespace: "test*"
         container: "*"
       - container: logger
         namespace: "*"        
Acceptance Criteria:

Verify when an include/exclude entry only defines "namespace" that it applies to all containers (e.g. *)

Verify when an include/exclude entry only defines "container" that it applies to all namespaces (e.g. *)

Verify multiple includes combinations results in logs included from each of the matches

Verify multiple excludes combinations results in logs excluded from each of the matches

Verify only includes that are not in the excludes list are matched. (i.e. excludes take precedence and are run first)

Verify the CEE documentation is updated to reflect the changes

Verify the enhancement documentation is updated to reflect the changes

Verify "namespaces" field as marked as deprecated

Verify the entries of the "namespaces" field are added to the list of "includes" in the generated configuration

Verify related/linked bugs are resolved

TODO:

create upstream docs

update CEE slides

Links

https://issues.redhat.com/browse/LOG-5186

Related:

https://issues.redhat.com/browse/LOG-5167

https://issues.redhat.com/browse/LOG-5162

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 openshift-eng/jira-lifecycle-plugin repository.

cahartma · 2024-03-14T12:30:23Z

/retest

cahartma · 2024-03-14T15:35:41Z

/retest

cahartma · 2024-03-14T17:37:26Z

/test lint

jcantrill

/hold

api/logging/v1/input_types.go

jcantrill · 2024-03-14T18:35:42Z

internal/generator/vector/conf/complex.toml

@@ -306,6 +306,7 @@ max_read_bytes = 3145728
 glob_minimum_cooldown_ms = 15000
 auto_partial_merge = true
 include_paths_glob_patterns = ["/var/log/pods/test-ns_*/*/*.log"]
+exclude_paths_glob_patterns = ["/var/log/pods/*/*/*.gz", "/var/log/pods/*/*/*.tmp", "/var/log/pods/default_*/*/*.log", "/var/log/pods/kube*_*/*/*.log", "/var/log/pods/openshift*_*/*/*.log"]


@Clee2691 noted this is unnecessary given the include will never match the exclude which was a change I made for the original implementation. If it is more challenging in the implementation, I can be swayed to leave as-is

I'd prefer to keep it there for all. I wrote it to handle "*" but think it makes more sense to have it present.

internal/generator/vector/input/viaq_test.go

internal/validations/clusterlogforwarder/validate_clusterlogforwarder_service_account.go

cahartma · 2024-03-15T11:56:06Z

/retest

cahartma · 2024-03-15T14:02:47Z

/retest

openshift-ci · 2024-03-15T18:30:49Z

@cahartma: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-ocp-target-minus-two	`178ae8f`	link	false	`/test e2e-ocp-target-minus-two`
ci/prow/functional	`178ae8f`	link	false	`/test functional`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

jcantrill · 2024-03-15T19:20:35Z

/hold cancel
/approve
/lgtm

openshift-ci · 2024-03-15T19:21:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cahartma, jcantrill

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:

~~OWNERS~~ [cahartma,jcantrill]

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

openshift-ci bot assigned jcantrill Mar 13, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 13, 2024

openshift-ci bot requested review from Clee2691 and vparfonov March 13, 2024 22:21

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 13, 2024

cahartma force-pushed the LOG-5186-improved-input-filter-api branch from 1c1fc54 to ec423a9 Compare March 14, 2024 13:41

cahartma force-pushed the LOG-5186-improved-input-filter-api branch from ec423a9 to 1d0efce Compare March 14, 2024 16:08

jcantrill added the release/5.9 label Mar 14, 2024

jcantrill reviewed Mar 14, 2024

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 14, 2024

cahartma force-pushed the LOG-5186-improved-input-filter-api branch from 1d0efce to 89c4989 Compare March 14, 2024 19:18

cahartma force-pushed the LOG-5186-improved-input-filter-api branch from 89c4989 to 21fb917 Compare March 15, 2024 16:43

Refactor input filter for includes and excludes

178ae8f

cahartma force-pushed the LOG-5186-improved-input-filter-api branch from 21fb917 to 178ae8f Compare March 15, 2024 16:44

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 15, 2024

openshift-merge-bot bot merged commit 3b403a3 into openshift:master Mar 15, 2024
8 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LOG-5186: Refactor input filter for includes and excludes #2404

LOG-5186: Refactor input filter for includes and excludes #2404

cahartma commented Mar 13, 2024

openshift-ci-robot commented Mar 13, 2024 •

edited by openshift-ci bot

Description

Acceptance Criteria:

TODO:

Links

cahartma commented Mar 14, 2024

cahartma commented Mar 14, 2024

cahartma commented Mar 14, 2024

jcantrill left a comment

jcantrill Mar 14, 2024

cahartma Mar 14, 2024

cahartma commented Mar 15, 2024

cahartma commented Mar 15, 2024

openshift-ci bot commented Mar 15, 2024

jcantrill commented Mar 15, 2024

openshift-ci bot commented Mar 15, 2024

LOG-5186: Refactor input filter for includes and excludes #2404

LOG-5186: Refactor input filter for includes and excludes #2404

Conversation

cahartma commented Mar 13, 2024

Description

Acceptance Criteria:

TODO:

Links

openshift-ci-robot commented Mar 13, 2024 • edited by openshift-ci bot

Description

Acceptance Criteria:

TODO:

Links

cahartma commented Mar 14, 2024

cahartma commented Mar 14, 2024

cahartma commented Mar 14, 2024

jcantrill left a comment

Choose a reason for hiding this comment

jcantrill Mar 14, 2024

Choose a reason for hiding this comment

cahartma Mar 14, 2024

Choose a reason for hiding this comment

cahartma commented Mar 15, 2024

cahartma commented Mar 15, 2024

openshift-ci bot commented Mar 15, 2024

jcantrill commented Mar 15, 2024

openshift-ci bot commented Mar 15, 2024

openshift-ci-robot commented Mar 13, 2024 •

edited by openshift-ci bot