Skip to content

fix(customresourcestate): log when configured CRD is not installed in cluster #2903

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

Open
maksimp13 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(customresourcestate): log when configured CRD is not installed in cluster #2903

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/customresourcestate/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,8 @@ func FromConfig(decoder ConfigDecoder, discovererInstance *discovery.CRDiscovere
resolvedSet /* GVKPs */, err := discovererInstance.ResolveGVKToGVKPs(schema.GroupVersionKind(resource.GroupVersionKind))
if err != nil {
klog.ErrorS(err, "failed to resolve GVK", "gvk", resource.GroupVersionKind)
} else if len(resolvedSet) == 0 {
Copy link
Copy Markdown
Member

@nmn3m nmn3m Mar 20, 2026

Choose a reason for hiding this comment

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

The change looks good to me — adding visibility for silently skipped CRDs is a nice improvement.

One note: this PR adds a log line, but issue #2354 reports that /metrics goes down entirely when non-existent CRDs are listed. A log doesn't fix that behavior, so Fixes #2354 might be inaccurate.
Consider changing it to Ref #2354 unless the endpoint issue has already been resolved separately.

/cc @rexagod

Copy link
Copy Markdown
Author

@maksimp13 maksimp13 Mar 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm.. I wasn't able to reproduce the /metrics crash locally - it seems it was already fixed in a previous versions. The endpoint returns 200 OK even when a configured CRD is not installed in the cluster.

Updated Fixes #2354 → Ref #2354.

klog.InfoS("CRD for resource is not installed in the cluster, skipping", "gvk", resource.GroupVersionKind)
Copy link
Copy Markdown
Member

@mrueg mrueg Mar 20, 2026

Choose a reason for hiding this comment

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

How often does this log? Only at the start up of ksm or regularly?

Copy link
Copy Markdown
Author

@maksimp13 maksimp13 Mar 20, 2026

Choose a reason for hiding this comment

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

The log fires once per CRD informer event (add/update/delete), not on every poll tick. generateMetrics() only runs when WasUpdated == true - set by the informer on any CRD change, reset after each run. So it runs once at startup, then only if the cluster CRD state changes.

Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

len(resolvedSet) == 0 can also happen when the configured G/V/K doesn’t match what’s in discovery (e.g., wrong version or kind), not only when the CRD is missing. Consider rewording this log to “no matching CRD found for configured GVK, skipping” (or similar) so it stays accurate.

Suggested change
klog.InfoS("CRD for resource is not installed in the cluster, skipping", "gvk", resource.GroupVersionKind)
klog.InfoS("no matching CRD found for configured GVK, skipping", "gvk", resource.GroupVersionKind)

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

@mrueg mrueg Apr 18, 2026

Choose a reason for hiding this comment

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

@maksimp13 can you address this comment?

}
for _, resolved /* GVKP */ := range resolvedSet {
// Set their G** attributes to various resolutions of the GVK.
Expand Down