search

fluxcd / flux

Successor: https://github.com/fluxcd/flux2
https://fluxcd.io
Apache License 2.0
6.9k stars 1.08k forks source link

WIP: handle noisy deprecation warnings #3555

Closed kingdonb closed 3 years ago

kingdonb commented 3 years ago

There is almost certainly more to do, this follows the discussion in #3554

We would like for Flux users to be able to get their deprecation warnings through Flux logs if it's the only way they have, however we would prefer that the GC sweep does not emit deprecation warnings for resource kinds that are deprecated but aren't even used on the cluster.

I believe this PR currently only suppresses all warnings, it is WIP.

kingdonb commented 3 years ago

I can confirm from testing (here) that this change does suppress all the warnings related to deprecations, though it seems a bit ham-handed, if we can assume the warnings are able to be received some other way, and they are just noise in the Flux logs, then this could be merged.

Given those constraints are not necessarily true, this is not merge-ready IMHO, however for users who are affected by this, if you want to squelch the messages and/or are unable to build this change for yourself, you can test it out now:

          image: docker.io/kingdonb/flux:suppress-noisy-deprecations-3d8b1524

I will bring this issue up for discussion at the Flux Dev Meeting later this week, meanwhile it will have to remain pending.

kingdonb commented 3 years ago

It sounds like we may not have actually been emitting these warnings during the main loop kubectl apply but only at the GC sweep. I will have to look at this again later on this week.

kingdonb commented 3 years ago

We addressed this at the meeting today, there is a slightly more surgical approach that we can take to make this better.

I will create a copy of the kubeconfig that we create in code for client-go and add the NoWarnings warning handler there. Then provide it to the dynamic config only, leaving the others in place.

The long and the short version is, this only affects client-go consumers which kubectl apply is not. So the warning messages that we care most about, the ones that belong to users' manifests that are emitted at apply time, are not suppressed by this change.

Making the update to only the single (dynamic client) that we are getting the spam from every sync cycle, should still resolve this, but leaves us open to receiving other warnings that might have been important. This is the implementation approach that was blessed in our discussion, I should be able to put that PR together soon and hopefully get a release published soon after.

If it can be submitted by an outside contributor, then I can review/approve and merge, but since I cannot approve my own PRs it will necessarily take just a bit longer unless there is someone else to submit the PR.

dimbleby commented 3 years ago

I've submitted #3558, I think that's what you're intending.

dimbleby commented 3 years ago

The warning messages that we care most about, the ones that belong to users' manifests that are emitted at apply time, are not suppressed by this change.

While this is true, those warnings are already suppressed - by virtue of the fact that flux simply doesn't log them. As I mentioned at https://github.com/fluxcd/flux/issues/3554#issuecomment-924366279, kubectl apply sends those warnings to stderr, and flux does not log that output.

I don't think that makes any difference at all to whether #3558 should be accepted. But if we really do care about warnings associated with user manifests, then there's a separate piece of work to be done for those.

kingdonb commented 3 years ago

Closing in favor of #3558