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

sysdump: Fix empty namespace error #1291

Merged
merged 2 commits into from
Dec 12, 2022

Conversation

pchaigno
Copy link
Member

See commits for details.

I'll note that if we implemented #1083 and failed CI on any warning, this sort of regression wouldn't happen.

Fixes: #1289
Fixes: #1132
cc @tommyp1ckles

@pchaigno pchaigno added kind/bug Something isn't working area/sysdump labels Dec 12, 2022
@pchaigno pchaigno requested a review from a team as a code owner December 12, 2022 10:08
@pchaigno pchaigno requested a review from squeed December 12, 2022 10:08
@pchaigno pchaigno temporarily deployed to ci December 12, 2022 10:08 — with GitHub Actions Inactive
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

✔️ 👍 🚢

@pchaigno pchaigno force-pushed the pr/pchaigno/fix-sysdump-empty-namespace branch from 6c0d2d8 to 3b21172 Compare December 12, 2022 10:14
@pchaigno pchaigno temporarily deployed to ci December 12, 2022 10:14 — with GitHub Actions Inactive
Copy link
Contributor

@squeed squeed left a comment

Choose a reason for hiding this comment

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

Looks like this tickled an unimplemented stub method in the unit tests. Otherwise looks good.

Commit e8cfcc7 added an autodetection feature for the Cilium
namespace, but because of a Golang error, never actually wrote it to the
Collector's options. The 'o' variable is never used after being written
to. This commit fixes it.

Fixes: e8cfcc7 ("sysdump: auto detect namespace from list of defaults.")
Signed-off-by: Paul Chaignon <[email protected]>
Commit e8cfcc7 ("sysdump: auto detect namespace from list of
defaults.") implemented autodetection for the cilium-agent namespace but
not for the cilium-operator namespace. It however cleared both
--cilium-operator-namespace and --cilium-namespace's default values,
thus causing collection of the cilium-operator deployment to fail all
the time.

This commit fixes it by implementing namespace detection for
cilium-operator.

Fixes: e8cfcc7 ("sysdump: auto detect namespace from list of defaults.")
Signed-off-by: Paul Chaignon <[email protected]>
@pchaigno pchaigno force-pushed the pr/pchaigno/fix-sysdump-empty-namespace branch from 3b21172 to 36b7aa8 Compare December 12, 2022 12:06
@pchaigno pchaigno temporarily deployed to ci December 12, 2022 12:06 — with GitHub Actions Inactive
@pchaigno pchaigno requested a review from squeed December 12, 2022 12:06
@tklauser tklauser merged commit 0894e70 into master Dec 12, 2022
@tklauser tklauser deleted the pr/pchaigno/fix-sysdump-empty-namespace branch December 12, 2022 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sysdump kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing resources in sysdump: an empty namespace may not be set when a resource name is provided
5 participants