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.

Sign up for GitHub

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

Jump to bottom

Catch nil ptr exceptions with enabled Forwarder and empty Collection #2313

Merged
merged 4 commits into from Feb 21, 2024
Merged

Catch nil ptr exceptions with enabled Forwarder and empty Collection #2313

merged 4 commits into from Feb 21, 2024

Conversation

andreaskaris
Copy link
Contributor

@andreaskaris andreaskaris commented Jan 18, 2024 •
edited by jcantrill

When the Forwarder is enabled, make sure that
clusterLogging.Spec.Collection is non nil to avoid nil pointer exceptions from occurring.

Fixes #2312
Fixes #2314
Fixes #2315

Description

Links

@andreaskaris andreaskaris changed the title Catch nil ptr exceptions with enabled Forwarder and empty Collection WIP: Catch nil ptr exceptions with enabled Forwarder and empty Collection Jan 18, 2024
@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 Jan 18, 2024
@jcantrill
Copy link
Contributor

/test functional-target

@andreaskaris andreaskaris force-pushed the fix-nil-pointer-exception branch 3 times, most recently from 894ba99 to fc17860 Compare January 18, 2024 17:02
jcantrill
jcantrill reviewed Jan 18, 2024
View reviewed changes
apis/logging/v1/cluster_log_forwarder.go Outdated Show resolved Hide resolved
apis/logging/v1/cluster_log_forwarder.go Outdated Show resolved Hide resolved
apis/logging/v1/cluster_log_forwarder.go Outdated Show resolved Hide resolved
apis/logging/v1/conditions.go Outdated Show resolved Hide resolved
apis/logging/v1/conditions.go Outdated Show resolved Hide resolved
controllers/forwarding/forwarding_controller.go Outdated Show resolved Hide resolved
internal/k8s/loader/load.go Show resolved Hide resolved
@jcantrill
Copy link
Contributor

/approve

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreaskaris, 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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2024
@andreaskaris andreaskaris force-pushed the fix-nil-pointer-exception branch 2 times, most recently from b66b10a to 4fa1c60 Compare January 19, 2024 13:40
jcantrill
jcantrill reviewed Jan 19, 2024
View reviewed changes
apis/logging/v1/clusterlogforwarder_types_test.go Outdated Show resolved Hide resolved
apis/logging/v1/clusterlogforwarder_types_test.go Outdated Show resolved Hide resolved
internal/k8s/loader/load.go Show resolved Hide resolved
@andreaskaris andreaskaris force-pushed the fix-nil-pointer-exception branch 2 times, most recently from 2eef62c to a9b7349 Compare January 21, 2024 15:51
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2024
@andreaskaris
Copy link
Contributor Author

note: I'm still working on this, I got pulled into some partner cases and will resume work as soon as I get some time

@jasaltvik jasaltvik mentioned this pull request Feb 7, 2024
Closed
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2024
@andreaskaris andreaskaris force-pushed the fix-nil-pointer-exception branch 3 times, most recently from 74cb313 to e17704f Compare February 9, 2024 17:14
@andreaskaris andreaskaris changed the title WIP: Catch nil ptr exceptions with enabled Forwarder and empty Collection Catch nil ptr exceptions with enabled Forwarder and empty Collection Feb 9, 2024
@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 Feb 9, 2024
@andreaskaris
Copy link
Contributor Author

@jcantrill wdyt now? thanks!

@andreaskaris andreaskaris marked this pull request as draft February 19, 2024 16:15
@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 Feb 19, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 19, 2024
@andreaskaris andreaskaris marked this pull request as ready for review February 19, 2024 17:12
@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 Feb 19, 2024
@andreaskaris
Fix 'make run' after refactor to operator-sdk
56d46f5 
Signed-off-by: Andreas Karis <ak.karis@gmail.com>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 19, 2024
@jcantrill
Copy link
Contributor

@andreaskaris please squash otherwise lgtm

@andreaskaris
Catch nil ptr exceptions with enabled Forwarder and empty Collection
c11c9ef 
When the Forwarder is enabled, make sure that
clusterLogging.Spec.Collection is non nil to avoid nil pointer
exceptions from occurring.

Signed-off-by: Andreas Karis <ak.karis@gmail.com>
@andreaskaris
Synchronize ClusterLogForwarder Status conditions, don't replace them
d1af0bb 
Replacing the CLF Status field causes the resourceVersion to be updated
constantly due to changing timestamps in Status conditions. This in
turn leads to an infinite reconciliation loop. Instead, synchronize all
status conditions so that timestamps remain unchanged if conditions are
not changed.

Signed-off-by: Andreas Karis <ak.karis@gmail.com>
@andreaskaris
Update ClusterLogForwarder status when ClusterLogging becomes valid
61c1e1a 
After a valid ClusterLogForwarder and an invalid ClusterLogging resource
are created, the ClusterLogForwarder's status will show that the CLF is
not available. When the ClusterLogging CR is then changes to a valid
configuration, the CLF's status should reflect that everything is o.k.
now.

Signed-off-by: Andreas Karis <ak.karis@gmail.com>
@andreaskaris
Copy link
Contributor Author

@andreaskaris please squash otherwise lgtm

Done, I moved the tests to their respective commits

@andreaskaris
Copy link
Contributor Author

/retest

@andreaskaris
Copy link
Contributor Author

/test e2e-ocp-target-minus-one

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 21, 2024

@andreaskaris: all tests passed!

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
Copy link
Contributor

/lgtm

@jcantrill
Copy link
Contributor

Any idea if this issue effects earlier releases?

Thank you for your cotribution

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2024
@andreaskaris
Copy link
Contributor Author

Yeah, I think that the nil pointer exception at least bit me on an earlier one like 5.8. I have a low prio task open to verify logging configuration for a partner, as soon as I get back to this, I'd be able to test

Also, if you look here: #2312 (comment)
There's someone else who complained about the nil pointer exception, for 5.8

@openshift-merge-bot openshift-merge-bot bot merged commit 96a97e5 into openshift:master Feb 21, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cahartma cahartma Awaiting requested review from cahartma

@syedriko syedriko Awaiting requested review from syedriko

@alanconway alanconway Awaiting requested review from alanconway

@jcantrill jcantrill Awaiting requested review from jcantrill

Assignees

@jcantrill jcantrill

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. release/5.9
Projects
None yet
Milestone
No milestone
3 participants
@andreaskaris @jcantrill @openshift-merge-robot