kubectl-gadget: Add --deploy flag to all commands #2196

burak-ok · 2023-11-03T16:46:21Z

If the flag is set Inspektor Gadget is deployed before running the specified command. After the command finished its execution, Inspektor Gadget will be undeployed

./kubectl-gadget trace tcp --deploy
deploy true
Creating Namespace/gadget...
Creating ServiceAccount/gadget...
Creating ClusterRole/gadget-cluster-role...
Creating ClusterRoleBinding/gadget-cluster-role-binding...
Creating Role/gadget-role...
Creating RoleBinding/gadget-role-binding...
Creating DaemonSet/gadget...
Creating CustomResourceDefinition/traces.gadget.kinvolk.io...
Waiting for gadget pod(s) to be ready...
0/1 gadget pod(s) ready
0/1 gadget pod(s) ready
1/1 gadget pod(s) ready
Retrieving Gadget Catalog...
Inspektor Gadget successfully deployed
K8S.NODE             K8S.NAMESPACE        K8S.POD              K8S.CONTAINER       T PID        COMM       IP SRC                       DST                      
minikube-docker      default              test-pod             test-pod            C 2077488    wget       4  p/default/test-pod:57674  r/216.58.212.163:80      
minikube-docker      default              test-pod             test-pod            X 2077488    wget       4  p/default/test-pod:57674  r/216.58.212.163:80      
minikube-docker      default              test-pod             test-pod            C 2077488    wget       4  p/default/test-pod:59654  r/142.250.185.227:80     
minikube-docker      default              test-pod             test-pod            X 2077488    wget       4  p/default/test-pod:59654  r/142.250.185.227:80     
^CRemoving traces...
Removing CRD...
Removing cluster role binding...
Removing cluster role...
Removing namespace...
Waiting for namespace to be removed...
Inspektor Gadget successfully removed

eiffel-fl

I tested it and it does the trick:

$ ./kubectl-gadget trace exec -A --deploy --image-pull-policy=Never
Creating Namespace/gadget...
Creating ServiceAccount/gadget...
Creating ClusterRole/gadget-cluster-role...
Creating ClusterRoleBinding/gadget-cluster-role-binding...
Creating Role/gadget-role...
Creating RoleBinding/gadget-role-binding...
Creating DaemonSet/gadget...
Creating CustomResourceDefinition/traces.gadget.kinvolk.io...
Waiting for gadget pod(s) to be ready...
0/1 gadget pod(s) ready
0/1 gadget pod(s) ready
1/1 gadget pod(s) ready
Retrieving Gadget Catalog...
Inspektor Gadget successfully deployed
K8S.NODE            K8S.NAMESPACE       K8S.POD             K8S.CONTAINER      PID       PPID      COMM      RET ARGS                    
minikube-docker     gadget              gadget-bkr98        gadget             78578     78569     gadgettr… 0   /bin/gadgettracermanage…
minikube-docker     gadget              gadget-bkr98        gadget             78600     78587     gadgettr… 0   /bin/gadgettracermanage…
minikube-docker     kube-system         kube-proxy-h58jx    kube-proxy         78749     46045     ip6tables 0   /usr/sbin/ip6tables -w …
minikube-docker     kube-system         kube-proxy-h58jx    kube-proxy         78750     46045     iptables  0   /usr/sbin/iptables -w 5…
minikube-docker     gadget              gadget-bkr98        gadget             78776     78767     gadgettr… 0   /bin/gadgettracermanage…
minikube-docker     gadget              gadget-bkr98        gadget             78799     78786     gadgettr… 0   /bin/gadgettracermanage…
^CRemoving traces...
Removing CRD...
Removing cluster role binding...
Removing cluster role...
Removing namespace...
Waiting for namespace to be removed...
Inspektor Gadget successfully removed

Where should the logs of deploy and undeploy go

I think the current behavior is OK, as the deployment can take a bit of time, this is better to inform user about it.
Otherwise, if people do not agree, we can make it printed only if --verbose is given.

Doesn't make sense for all commands, yet the flags are registered for all commands

Any way to filter it out for these commands?

cmd/kubectl-gadget/main.go

alban · 2023-11-08T11:05:45Z

Where should the logs of deploy and undeploy go

Could this go to stderr? In this way, stdout could be parsed correctly when using options such as -o json or -o yaml.

burak-ok · 2023-11-27T16:21:08Z

Doesn't make sense for all commands, yet the flags are registered for all commands

Any way to filter it out for these commands?

Does anyone have an idea for this?

eiffel-fl · 2023-11-28T08:17:01Z

Doesn't make sense for all commands, yet the flags are registered for all commands

Any way to filter it out for these commands?

Does anyone have an idea for this?

Isn't it possible to remove the flag for these commands?

burak-ok · 2023-11-28T13:52:47Z

Isn't it possible to remove the flag for these commands?

I don't think so that removing them is possible after one added the flag.

I also can not add them one by one for the Commands from the Registry. AddCommandsFromRegistry utilizes the catalog and we might not have it yet (without deploying first). So the deploying itself has to be done before the AddCommandsFromRegistry step

eiffel-fl

I will test but it looks good, I just have one comment:

eiffel-fl · 2023-11-29T10:38:01Z

cmd/kubectl-gadget/deploy.go

@@ -19,6 +19,7 @@ import (
 	_ "embed"


If you really want to make it settable, you need to add a flag.

I don't quite understand your comment. For what do I need a flag here in this file?

If you mean --deploy that is in the main.go file.
Or are you suggesting that the output file stdout/stderr should be settable by the user?

Or are you suggesting that the output file stdout/stderr should be settable by the user?

This!

For this PR my goal was to leave the deploy command the same as before. So its output is going to stdout. When using the --deploy flag though the output of deploy and undeploy is redirected to stderr so the user can easily filter out these parts if they are only interested in the gadget output.

Maybe we could add a redirect to another out stream feature for all gadgets in another PR?

Maybe we could add a redirect to another out stream feature for all gadgets in another PR?

Let's keep what we have now and see if we need to add a dedicated flag in the future.

eiffel-fl

I tested it again and it works fine:

$ ./kubectl-gadget trace exec -A --deploy --image-pull-policy=Never
Creating Namespace/gadget...
Creating ServiceAccount/gadget...
Creating ClusterRole/gadget-cluster-role...
Creating ClusterRoleBinding/gadget-cluster-role-binding...
Creating DaemonSet/gadget...
Creating CustomResourceDefinition/traces.gadget.kinvolk.io...
Waiting for gadget pod(s) to be ready...
1/1 gadget pod(s) ready
Retrieving Gadget Catalog...
Inspektor Gadget successfully deployed
K8S.NODE            K8S.NAMESPACE       K8S.POD             K8S.CONTAINER      PID       PPID      COMM      RET ARGS                    
minikube-docker     gadget              gadget-drzxk        gadget             93407     93397     gadgettr… 0   /bin/gadgettracermanage…
minikube-docker     gadget              gadget-drzxk        gadget             93434     93419     gadgettr… 0   /bin/gadgettracermanage…
^CRemoving traces...
Removing CRD...
Removing cluster role binding...
Removing cluster role...
Removing namespace...
Waiting for namespace to be removed...
Inspektor Gadget successfully removed

mauriciovasquezbernal · 2023-11-29T17:01:58Z

Doesn't make sense for all commands, yet the flags are registered for all commands

Any way to filter it out for these commands?

Does anyone have an idea for this?

I think the following can make the trick: (after calling rootCmd.PersistentFlags().BoolVar(&deploy, "deploy")...

	for _, c := range rootCmd.Commands() {
		switch c.Name() {
		case "deploy", "version", "undeploy":
			c.InheritedFlags().MarkHidden("deploy")
		}
	}

burak-ok · 2023-11-29T17:08:16Z

Doesn't make sense for all commands, yet the flags are registered for all commands

Any way to filter it out for these commands?

Does anyone have an idea for this?

I think the following can make the trick: (after calling rootCmd.PersistentFlags().BoolVar(&deploy, "deploy")...
	for _, c := range rootCmd.Commands() {
		switch c.Name() {
		case "deploy", "version", "undeploy":
			c.InheritedFlags().MarkHidden("deploy")
		}
	}

I guess hiding is better than leaving it there. Even if it still can be set.

EDIT: all other flags that got added through addGeneralDeployFlags(rootCmd) are still visible. I'll think about if that can be circumvented too in any other way. For now I just mark them hidden here aswell

burak-ok · 2023-11-29T17:21:49Z

I think the following can make the trick: (after calling rootCmd.PersistentFlags().BoolVar(&deploy, "deploy")...
	for _, c := range rootCmd.Commands() {
		switch c.Name() {
		case "deploy", "version", "undeploy":
			c.InheritedFlags().MarkHidden("deploy")
		}
	}
I guess hiding is better than leaving it there. Even if it still can be set.

EDIT: all other flags that got added through addGeneralDeployFlags(rootCmd) are still visible. I'll think about if that can be circumvented too in any other way. For now I just mark them hidden here aswell

Just tested it. The logic here seems to be that if we find any command registered that is called deploy, undeploy or version it will hide the flag for ALL commands :/

mauriciovasquezbernal · 2023-11-29T18:24:51Z

Just tested it. The logic here seems to be that if we find any command registered that is called deploy, undeploy or version it will hide the flag for ALL commands :/

I only tested the happy case (that --deploy was removed from deploy)

I think something based on https://stackoverflow.com/questions/46591225/how-to-mark-some-global-persistent-flags-as-hidden-for-some-cobra-commands should work:

	for _, c := range rootCmd.Commands() {
		switch c.Name() {
		case "deploy", "version", "undeploy":
			origHelp := c.HelpFunc()
			c.SetHelpFunc(func(cmd *cobra.Command, args []string) {
				c.InheritedFlags().MarkHidden("deploy")
				origHelp(cmd, args)
			})
		}
	}

$ ./kubectl-gadget trace exec -h | grep deploy
INFO[0000] Experimental features enabled                
      --deploy                         Deploy the gadget to the cluster before running the gadget and undeploy it afterwards
      --image string                   container image (default "192.168.1.150:5000/gadget:deploy_undeploy_with_cmd")
$ ./kubectl-gadget deploy -h | grep deploy
INFO[0000] Experimental features enabled                
  kubectl-gadget deploy [flags]
  -h, --help                           help for deploy
      --image string                   container image (default "192.168.1.150:5000/gadget:deploy_undeploy_with_cmd")
      --timeout duration               timeout for deployment (default 2m0s)

mauriciovasquezbernal

The implementation looks mostly good. My main concern is that lot of flags are added to all gadgets. I think spf13/cobra#1778 could help with that, but I'm still wondering about possible collisions, for instance, note the following flags:

./kubectl-gadget trace exec -h
...
  -d, --debug                          show extra debug information
...
  -q, --quiet                          supress information messages
...
  -v, --verbose                        Print debug information
...

Perhaps we shouldn't be exposing too many flags to the commands, only the most relevant ones like --image and maybe --image-pull-policy? I know it'd be nice to be able to control all those options from different commands --deploy, but at the same time pollutes the commands with a lot of non relevant flags.

cmd/kubectl-gadget/main.go

mauriciovasquezbernal · 2023-11-29T16:12:52Z

cmd/kubectl-gadget/main.go

+	if deploy {
+		err := runUndeploy(os.Stderr)
+		if err != nil {
+			log.Errorf("undeploying Inspektor Gadget: %v\n", err)
+			os.Exit(1)
+		}
+	}


Put this on a defer() on L110? If something goes wrong on L122, we don't want that message to be lost because it's exiting here.

The output still behaves like before: There is no output of the err returned by rootCmd.Execute. But I could add that

Problem with defer is that it will not run when we are exiting with os.Exit(). See Line 133:

if err != nil { os.Exit(1) }

Perhaps we can refactor main() not to call os.Exit() in different places? -> https://stackoverflow.com/a/27629493, however it's not related to this PR, so I'm fine with the current solution.

cmd/kubectl-gadget/main.go

burak-ok · 2023-11-30T11:15:00Z

Perhaps we shouldn't be exposing too many flags to the commands, only the most relevant ones like --image and maybe --image-pull-policy? I know it'd be nice to be able to control all those options from different commands --deploy, but at the same time pollutes the commands with a lot of non relevant flags.

One could argue that --hook-mode, --node-selector and maybe even --experimental (ok, we get that through the env variable too) can be usefull too.

We should make a list what might be the most used/useful ones

mauriciovasquezbernal · 2023-11-30T14:26:59Z

One could argue that --hook-mode, --node-selector and maybe even --experimental (ok, we get that through the env variable too) can be usefull too.

We should make a list what might be the most used/useful ones

I see the --deploy flag like an "express" way to use Inspektor Gadget, IMO we can just leave it to be deployed with the default configuration, if the user wants to customize it, then they should use kubectl gadget deploy. I'd say the only important flag is --image others can be part of deploy command only.

--node-selector

Good call, in this case we should use the already existing --node flag on different gadgets.

github-actions · 2024-03-07T01:40:22Z

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

Signed-off-by: Burak Ok <burakok@microsoft.com>

If the flag is set Inspektor Gadget is deployed before running the specified command. After the command finished its execution, Inspektor Gadget will be undeployed Signed-off-by: Burak Ok <burakok@microsoft.com>

github-actions · 2024-05-27T01:48:29Z

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

burak-ok · 2024-06-07T10:18:36Z

Any new reviews or comments?

burak-ok linked an issue Nov 3, 2023 that may be closed by this pull request

Provide a way to run a gadget without having to manually deploy Inspektor Gadget #2153

Open

burak-ok force-pushed the deploy_undeploy_with_cmd branch from e243b98 to 57a3131 Compare November 6, 2023 14:05

eiffel-fl reviewed Nov 8, 2023

View reviewed changes

cmd/kubectl-gadget/main.go Outdated Show resolved Hide resolved

burak-ok force-pushed the deploy_undeploy_with_cmd branch from 57a3131 to f946139 Compare November 8, 2023 16:08

burak-ok marked this pull request as ready for review November 8, 2023 16:09

burak-ok requested review from alban and mauriciovasquezbernal as code owners November 8, 2023 16:09

burak-ok requested a review from flyth November 8, 2023 16:09

burak-ok force-pushed the deploy_undeploy_with_cmd branch from f946139 to 42d8b69 Compare November 27, 2023 16:20

burak-ok force-pushed the deploy_undeploy_with_cmd branch from 42d8b69 to b8f409e Compare November 28, 2023 13:49

eiffel-fl reviewed Nov 29, 2023

View reviewed changes

eiffel-fl approved these changes Nov 29, 2023

View reviewed changes

mauriciovasquezbernal reviewed Nov 29, 2023

View reviewed changes

burak-ok force-pushed the deploy_undeploy_with_cmd branch from b8f409e to 4de0035 Compare November 30, 2023 11:13

github-actions bot added the lifecycle/stale Marked to be closed in next 14 days because of inactivity. label Mar 7, 2024

mauriciovasquezbernal removed the lifecycle/stale Marked to be closed in next 14 days because of inactivity. label Mar 11, 2024

burak-ok added 3 commits March 21, 2024 15:32

kubectl-gadget: Allowed setting output file for (un)deploy

065e8e4

Signed-off-by: Burak Ok <burakok@microsoft.com>

kubectl-gadget/deploy: Generalized deploy flags

fd3003c

Signed-off-by: Burak Ok <burakok@microsoft.com>

kubectl-gadget: Add --deploy flag to all commands

d2fbcb7

If the flag is set Inspektor Gadget is deployed before running the specified command. After the command finished its execution, Inspektor Gadget will be undeployed Signed-off-by: Burak Ok <burakok@microsoft.com>

burak-ok force-pushed the deploy_undeploy_with_cmd branch from 4de0035 to d2fbcb7 Compare March 22, 2024 13:42

github-actions bot added the lifecycle/stale Marked to be closed in next 14 days because of inactivity. label May 27, 2024

github-actions bot removed the lifecycle/stale Marked to be closed in next 14 days because of inactivity. label Jun 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubectl-gadget: Add --deploy flag to all commands #2196

kubectl-gadget: Add --deploy flag to all commands #2196

burak-ok commented Nov 3, 2023 •

edited

eiffel-fl left a comment •

edited

alban commented Nov 8, 2023

burak-ok commented Nov 27, 2023

eiffel-fl commented Nov 28, 2023

burak-ok commented Nov 28, 2023

eiffel-fl left a comment

eiffel-fl Nov 29, 2023

burak-ok Nov 29, 2023

eiffel-fl Nov 29, 2023

burak-ok Nov 29, 2023

eiffel-fl Nov 29, 2023

eiffel-fl left a comment •

edited

mauriciovasquezbernal commented Nov 29, 2023 •

edited

burak-ok commented Nov 29, 2023 •

edited

burak-ok commented Nov 29, 2023

mauriciovasquezbernal commented Nov 29, 2023

mauriciovasquezbernal left a comment

mauriciovasquezbernal Nov 29, 2023

burak-ok Nov 30, 2023

mauriciovasquezbernal Nov 30, 2023

burak-ok commented Nov 30, 2023

mauriciovasquezbernal commented Nov 30, 2023

github-actions bot commented Mar 7, 2024

github-actions bot commented May 27, 2024

burak-ok commented Jun 7, 2024 •

edited

kubectl-gadget: Add --deploy flag to all commands #2196

Are you sure you want to change the base?

kubectl-gadget: Add --deploy flag to all commands #2196

Conversation

burak-ok commented Nov 3, 2023 • edited

eiffel-fl left a comment • edited

Choose a reason for hiding this comment

alban commented Nov 8, 2023

burak-ok commented Nov 27, 2023

eiffel-fl commented Nov 28, 2023

burak-ok commented Nov 28, 2023

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eiffel-fl left a comment • edited

Choose a reason for hiding this comment

mauriciovasquezbernal commented Nov 29, 2023 • edited

burak-ok commented Nov 29, 2023 • edited

burak-ok commented Nov 29, 2023

mauriciovasquezbernal commented Nov 29, 2023

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burak-ok commented Nov 30, 2023

mauriciovasquezbernal commented Nov 30, 2023

github-actions bot commented Mar 7, 2024

github-actions bot commented May 27, 2024

burak-ok commented Jun 7, 2024 • edited

burak-ok commented Nov 3, 2023 •

edited

eiffel-fl left a comment •

edited

eiffel-fl left a comment •

edited

mauriciovasquezbernal commented Nov 29, 2023 •

edited

burak-ok commented Nov 29, 2023 •

edited

burak-ok commented Jun 7, 2024 •

edited