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 no Cilium output if operator was not detected #2635

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

marseel
Copy link
Contributor

@marseel marseel commented Jun 26, 2024

Fixes regression from #1776

@marseel marseel requested a review from a team as a code owner June 26, 2024 10:34
@marseel marseel requested a review from asauber June 26, 2024 10:34
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

we chatted about this a bit offline. ideally --cilium-namespace should set namespace for both cilium-agent and cilium-operator unless --cilium-operator-namespace is also specified. could we do something like this:

% git diff
diff --git a/cli/sysdump.go b/cli/sysdump.go
index 2069e548..de6fc8d3 100644
--- a/cli/sysdump.go
+++ b/cli/sysdump.go
@@ -34,8 +34,12 @@ func newCmdSysdump(hooks sysdump.Hooks) *cobra.Command {
                        if sysdumpOptions.CiliumNamespace == "" && cmd.Flags().Changed("namespace") {
                                sysdumpOptions.CiliumNamespace = namespace
                        }
-                       if sysdumpOptions.CiliumOperatorNamespace == "" && cmd.Flags().Changed("namespace") {
-                               sysdumpOptions.CiliumOperatorNamespace = namespace
+                       if sysdumpOptions.CiliumOperatorNamespace == "" {
+                               if cmd.Flags().Changed("namespace") {
+                                       sysdumpOptions.CiliumOperatorNamespace = namespace
+                               } else {
+                                       sysdumpOptions.CiliumOperatorNamespace = sysdumpOptions.CiliumNamespace
+                               }
                        }
                        // Honor --helm-release-name global flag in case it is set and --cilium-helm-release-name is not set
                        if sysdumpOptions.CiliumHelmReleaseName == "" && cmd.Flags().Changed("helm-release-name") {

@michi-covalent michi-covalent added the priority/release-blocker This issue will prevent the release of the next version of Cilium. label Jun 26, 2024
@marseel marseel force-pushed the fix_missing_operator branch from 2b764a4 to e940151 Compare June 27, 2024 07:24
@marseel marseel force-pushed the fix_missing_operator branch from e940151 to c602cc9 Compare June 27, 2024 07:26
marseel and others added 2 commits June 27, 2024 10:35
@marseel
Copy link
Contributor Author

marseel commented Jun 27, 2024

Hey @michi-covalent
I thought about it a bit more with a fresh brain.
I've added your snippet as a separate commit, but I still believe that having "&& -> ||" change makes sense as is orthogonal to your proposed change.
If user does not specify a namespace at all and we fail to detect only one of agent/operator namespaces (for example if someone deletes operator deployment) we might still want to run agent tasks.

I've also opened #2645 which seems like a reasonable next-step to improve the sysdump experience for users. Maybe good-first-time issue :) ?

@marseel marseel requested a review from michi-covalent June 27, 2024 08:41
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

🚀

@michi-covalent michi-covalent merged commit 92440dd into cilium:main Jun 27, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/release-blocker This issue will prevent the release of the next version of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants