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

Consider filtering out finalizer warnings in controller logs #11065

Closed
sbueringer opened this issue Aug 19, 2024 · 8 comments · Fixed by #11242
Closed

Consider filtering out finalizer warnings in controller logs #11065

sbueringer opened this issue Aug 19, 2024 · 8 comments · Fixed by #11242
Labels
area/logging Issues or PRs related to logging kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@sbueringer
Copy link
Member

sbueringer commented Aug 19, 2024

Context:

The warnings we get today in our controllers look like this:

{"ts":1724055396150.5093,"logger":"KubeAPIWarningLogger","caller":"log/warning_handler.go:65","msg":"metadata.finalizers: "addons.cluster.x-k8s.io": prefer a domain-qualified finalizer name to avoid accidental conflicts with other finalizer writers","v":0}

As we are aware of the "problem" and it will take us a while to migrate away from our current finalizers, I think we should consider to filter out the warnings in our controller logs.

Today logging the warnings produces a lot of spam and nobody that is running Cluster API can do anything about it.

@sbueringer sbueringer added the area/logging Issues or PRs related to logging label Aug 19, 2024
@k8s-ci-robot k8s-ci-robot added needs-priority Indicates an issue lacks a `priority/foo` label and requires one. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 19, 2024
@sbueringer
Copy link
Member Author

/cc @dlipovetsky (just fyi)
/cc @chrischdi @fabriziopandini

@sbueringer sbueringer changed the title Consider filtering out warnings in controller-logs Consider filtering out warnings in controller logs Aug 19, 2024
@sbueringer sbueringer changed the title Consider filtering out warnings in controller logs Consider filtering out finalizer warnings in controller logs Aug 19, 2024
@sbueringer sbueringer added kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Aug 19, 2024
@k8s-ci-robot k8s-ci-robot removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates an issue lacks a `priority/foo` label and requires one. labels Aug 19, 2024
@Karthik-K-N
Copy link
Contributor

@sbueringer whats expected from this issue, If you can provide some info, I can try working on it.

@fabriziopandini fabriziopandini added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Sep 11, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Sep 11, 2024
@fabriziopandini
Copy link
Member

+1 to implement this while we figure it out the transition to fully compliant finalizers tracked in #10914

@sbueringer
Copy link
Member Author

@Karthik-K-N Sorry just noticed that this might have a big overlap with this: #11149 (comment)

Let's see how the other PR goes first. Potentially we can just use the same implementation

@Karthik-K-N
Copy link
Contributor

Karthik-K-N commented Sep 11, 2024

@Karthik-K-N Sorry just noticed that this might have a big overlap with this: #11149 (comment)
Let's see how the other PR goes first. Potentially we can just use the same implementation

Sure, will wait Thank you.

@dlipovetsky
Copy link
Contributor

I missed the tag earlier.

I think this is separate from the change in clusterctl (#11149), but the implementation should be similar.

We may want to hide specific warnings (e.g. only warnings related to Cluster API finalizers) in controllers and clusterctl. In that case, we'll need to implement a warning handler, and we can use that in both places. Is that something we want? If so, I can file a PR for that handler.

I have a draft PR for hiding all warnings in clusterctl (#11173).

@sbueringer
Copy link
Member Author

We may want to hide specific warnings (e.g. only warnings related to Cluster API finalizers) in controllers and clusterctl. In that case, we'll need to implement a warning handler, and we can use that in both places. Is that something we want? If so, I can file a PR for that handler.

Yeah I was thinking that the PR you wanted to open might lead to that, but I was waiting to see how that goes :)

@sbueringer
Copy link
Member Author

sbueringer commented Sep 11, 2024

I'm not sure if we should just ignore all warnings in clusterctl. Mostly because I don't know what other warnings there might be, so I thought it's maybe better to only hide/ignore what we know is absolutely safe to ignore (the finalizer warning)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/logging Issues or PRs related to logging kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
5 participants