cmd/deploy log level in ig-k8s pod #2312
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghinks
requested review from
mqasimsarfraz,
alban and
mauriciovasquezbernal
as code owners
ghinks
force-pushed
the
ghinks/cmd-deploy-log-level
branch
from
7102625
to
be6ee6f
Compare
eiffel-fl
reviewed
Dec 19, 2023
eiffel-fl
reviewed
Dec 20, 2023
ghinks
force-pushed
the
ghinks/cmd-deploy-log-level
branch
2 times, most recently
from
d47821e
to
44c4705
Compare
eiffel-fl
reviewed
Dec 26, 2023
ghinks
force-pushed
the
ghinks/cmd-deploy-log-level
branch
2 times, most recently
from
5d3ad47
to
e4d7b60
Compare
eiffel-fl
reviewed
Dec 29, 2023
pkg/utils/gadgettracermanagerloglevel/gadgettracermanagerloglevel.go
Outdated
Show resolved
Hide resolved
eiffel-fl
reviewed
Jan 2, 2024
pkg/utils/gadgettracermanagerloglevel/gadgettracermanagerloglevel.go
Outdated
Show resolved
Hide resolved
eiffel-fl
reviewed
Jan 3, 2024
There was a problem hiding this comment.
Hi!
I tested your contribution and it looks fine:
$ ./kubectl-gadget deploy --image-pull-policy=Never --daemon-log-level warning
INFO[0000] Experimental features enabled
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
1/1 gadget pod(s) ready
Retrieving Gadget Catalog...
Inspektor Gadget successfully deployed
$ ../kubectl get pod -n gadget (remotes/ghinks/ghinks/cmd-deploy-log-level) %
NAME READY STATUS RESTARTS AGE
gadget-bnp89 1/1 Running 0 7s
$ ../kubectl logs -n gadget gadget-bnp89
time="2024-01-03T14:36:47Z" level=warning msg="failed to get cgroups info of container 0fa989494188d2b3fc4520b7c52579417fbcd54fab79768c5ab0c0446b303c15 from /proc/9419/cgroup: cgroup path not found in /proc/PID/cgroup"
time="2024-01-03T14:36:47Z" level=error msg="cgroup enricher: failed to get cgroup paths on container 0fa989494188d2b3fc4520b7c52579417fbcd54fab79768c5ab0c0446b303c15: cgroup path not found in /proc/PID/cgroup"
time="2024-01-03T14:36:47Z" level=warning msg="failed to get cgroups info of container 0fa989494188d2b3fc4520b7c52579417fbcd54fab79768c5ab0c0446b303c15 from /proc/9419/cgroup: cgroup path not found in /proc/PID/cgroup"I have some comments but nothing critical.
Also, I would like to get @flyth opinion as we have other loggers and I would like to understand how we can articulate this contribution with them.
I will enable the CI to run for your contribution.
You can also undraft this PR as I think we are near the truth.
Best regards.
EDIT: It seems you have to handle the conflict for go.mod to enable the CI.
Moreover, I do not think you actually need this replace.
cmd/kubectl-gadget/deploy.go
Outdated
Comment on lines
99
to
105
| strLevels := make([]string, len(log.AllLevels)) | ||
| for i, level := range log.AllLevels { | ||
| strLevels[i] = level.String() | ||
| } |
cmd/kubectl-gadget/deploy.go
Outdated
Comment on lines
492
to
499
| case "GADGET_TRACER_MANAGER_LOG_LEVEL": | ||
| gadgetContainer.Env[i].Value = daemonLogLevel |
There was a problem hiding this comment.
Before assigning here, I think we should test that the values belongs to strLevels.
You can take a look at slices.Contains().
|
First of all: Great contribution, thanks!
I think this'll fit in nicely with what we have. To give some context:
|
eiffel-fl
reviewed
Jan 4, 2024
cmd/kubectl-gadget/deploy.go
Outdated
Comment on lines
101
to
103
| strLevels = make([]string, len(log.AllLevels)) | ||
| for _, level := range log.AllLevels { | ||
| strLevels = append(strLevels, level.String()) |
There was a problem hiding this comment.
It is not possible to use copy() here?
Something like this:
Suggested change
| strLevels = make([]string, len(log.AllLevels)) | |
| for _, level := range log.AllLevels { | |
| strLevels = append(strLevels, level.String()) | |
| strLevels = make([]string, len(log.AllLevels)) | |
| copy(strLevels, log.AllLevels) |
There was a problem hiding this comment.
The logrus all levels is an int and has to have a conversion to a String type so a loop is required. But I'm happy to be corrected. Although I had lots of c/c++ experience the reason I'm doing this work is partially to improve my go.
There was a problem hiding this comment.
I tested it yesterday and I also got this issue.
And indeed, this does not make real sense to copy() int to a string buffer!
There was a problem hiding this comment.
thank you, I'm looking at the squash/rebase for the commit now.
There was a problem hiding this comment.
You are welcome!
Can you please just run again make lint?
For the rebase I think something like this may help you:
$ git rebase -i HEAD~16
# An editor open, replace almost all, except the first, pick by 's'. Close the editor once OK.
# The editor open again to let you write the commit message.In the meanwhile, I will test it again but everything should be OK.
eiffel-fl
reviewed
Jan 4, 2024
With |
eiffel-fl
reviewed
Jan 4, 2024
cmd/kubectl-gadget/deploy.go
Outdated
| @@ -19,7 +19,9 @@ import ( | |||
| _ "embed" | |||
| "encoding/json" | |||
| "fmt" | |||
| log "github.com/sirupsen/logrus" | |||
There was a problem hiding this comment.
Just run make lint to have this import set at the correct place.
There was a problem hiding this comment.
will do, sorry still testing cleanup
There was a problem hiding this comment.
No worries, this is not the end of the world to push a non-linted code!
eiffel-fl
reviewed
Jan 4, 2024
There was a problem hiding this comment.
I tested it and got this strange behavior:
Error: invalid log level "foobar", valid levels are: , , , , , , , panic, fatal, error, warning, info, debug, trace
The following patch solves this, otherwise we would double the slice size while keeping empty string at the beginning:
diff --git a/cmd/kubectl-gadget/deploy.go b/cmd/kubectl-gadget/deploy.go
index e618caaa..fdd5123f 100644
--- a/cmd/kubectl-gadget/deploy.go
+++ b/cmd/kubectl-gadget/deploy.go
@@ -100,8 +100,8 @@ var supportedHooks = []string{"auto", "crio", "podinformer", "nri", "fanotify",
func init() {
commonutils.AddRuntimesSocketPathFlags(deployCmd, &runtimesConfig)
strLevels = make([]string, len(log.AllLevels))
- for _, level := range log.AllLevels {
- strLevels = append(strLevels, level.String())
+ for i, level := range log.AllLevels {
+ strLevels[i] = level.String()
}
deployCmd.PersistentFlags().StringVarP(
&image,Regarding using copy(), this is easier to say than to actually do as golang will complain regarding types.
Otherwise, the remaining works perfectly:
$ ./kubectl-gadget deploy --image-pull-policy=Never --daemon-log-level warning
...
$ ../kubectl logs -n gadget gadget-h5z28
time="2024-01-04T13:56:53Z" level=warning msg="failed to get cgroups info of container 361433f5cc92af30518cc77575babd563cdad649493cc1379e01d47c02b284dc from /proc/14171/cgroup: cgroup path not found in /proc/PID/cgroup"
time="2024-01-04T13:56:53Z" level=error msg="cgroup enricher: failed to get cgroup paths on container 361433f5cc92af30518cc77575babd563cdad649493cc1379e01d47c02b284dc: cgroup path not found in /proc/PID/cgroup"
time="2024-01-04T13:56:53Z" level=warning msg="failed to get cgroups info of container 361433f5cc92af30518cc77575babd563cdad649493cc1379e01d47c02b284dc from /proc/14171/cgroup: cgroup path not found in /proc/PID/cgroup
The CI is OK, except a little lint issue.
Also, can you please rebase your history to only have one commit and detailing what this commit do in the commit message?
ghinks
changed the title
eiffel-fl
approved these changes
Jan 5, 2024
There was a problem hiding this comment.
Hi!
Thank you!
I tested it and everything is fine:
$ ./kubectl-gadget deploy --image-pull-policy=Never --daemon-log-level foobar
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...
Error: invalid log level "foobar", valid levels are: panic, fatal, error, warning, info, debug, trace
$ ./kubectl-gadget deploy --image-pull-policy=Never --daemon-log-level warning
...
$ ../kubectl logs -n gadget gadget-4l5wf
time="2024-01-05T12:31:35Z" level=warning msg="failed to get cgroups info of container 302ee8b43b673123555f69345c3d84abf694eb297313ed5c5e6ba3344c312625 from /proc/10599/cgroup: cgroup path not found in /proc/PID/cgroup"
time="2024-01-05T12:31:35Z" level=error msg="cgroup enricher: failed to get cgroup paths on container 302ee8b43b673123555f69345c3d84abf694eb297313ed5c5e6ba3344c312625: cgroup path not found in /proc/PID/cgroup"
time="2024-01-05T12:31:35Z" level=warning msg="failed to get cgroups info of container 302ee8b43b673123555f69345c3d84abf694eb297313ed5c5e6ba3344c312625 from /proc/10599/cgroup: cgroup path not found in /proc/PID/cgroup"I will wait for you to push the rebased commit and make lint it and I will merge it.
Best regards.
ghinks
force-pushed
the
ghinks/cmd-deploy-log-level
branch
from
39f6eda
to
5f5a76d
Compare
- makefile - deploy.go changes to pass log level Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
- remove debug line Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
add a comment for sed on macos so that folks understand that you need to give an extra argument for a backup file or empty string for none. Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
…tion\nfor sed in the chart version update for use\nof empty string for the interim edit in place\nfile. This will not leave any backup file Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
- Change the daemonLogLevel to use a string - add some logging into the entrypoint to test locally. Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
- fix space in makefile after -i - add default of info to the gadget values - add missing daemonLogLevels, that was missed off before Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
…emon-log-level. This is useful for debugging issues in the field. The
new command will set the environment variable **GADGET_TRACER_MANAGER_LOG_LEVEL** in the ig-k8s pod. The default value will be **info**.
Changes were made to the daemonset chart to add the new command line option and to set the environment variable in the pod.
Changes were made to the gadget container to set the log level based on the environment variable.
The log level is set in entrypoint.go, gadgettracermanager/main.go and cleanup.go.
To set the GADGET_TRACER_MANAGER_LOG_LEVEL to trace, run the following command:
```shell
./kubectl-gadget deploy --daemon-log-level=trace
```
The log level can be set to any of the following values from the logrus package:
- trace
- debug
- info
- warn
- error
- fatal
- panic
To validate this PR on a local image I used minikube.
- build the images locally **make gadget-container** ( required as daemonset chart is updated with new env var )
- add the image to the local k8s cluster, in my case minikube via "minikube cache add ghcr.io/inspektor-gadget/inspektor-gadget:ghinks/cmd-deploy-log-level"
- make kubectl-gadget
- deploy via './kubectl-gadget deploy --daemon-log-level=trace --image-pull-policy=Never'
- the deployment was validated via 'k describe pod -n "gadget" $mypod'
- undeploy via 'kubectl-gadget undeploy'
Testing on minikube required a local image as minikube would try to pull the image from the registry. The image was built locally and added to the minikube cache.
```shell
export MY_CONTAINER="ghcr.io/inspektor-gadget/inspektor-gadget:ghinks-cmd-deploy-log-level"
minikube cache delete $MY_CONTAINER && minikube image rm $MY_CONTAINER && minikube cache add $MY_CONTAINER
```
To view the logs from the pod:
```shell
export MY_POD=`kubectl get pods -o jsonpath='{.items[*].metadata.name}' -n gadget`
k logs -n gadget $MY_POD -f
```
Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
- add help on levels to the deploy command - add check on the tracermanager for invalid logrus log level Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
- add default log level message Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
…vel.go Co-authored-by: eiffel-fl <laniel_francis@privacyrequired.com> Signed-off-by: Glenn <ghinks@yahoo.com> Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
update error format to use ErrorF Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
…emon-log-level. This is useful for debugging issues in the field. The
new command will set the environment variable **GADGET_TRACER_MANAGER_LOG_LEVEL** in the ig-k8s pod. The default value will be **info**.
Changes were made to the daemonset chart to add the new command line option and to set the environment variable in the pod.
Changes were made to the gadget container to set the log level based on the environment variable.
The log level is set in entrypoint.go, gadgettracermanager/main.go and cleanup.go.
To set the GADGET_TRACER_MANAGER_LOG_LEVEL to trace, run the following command:
```shell
./kubectl-gadget deploy --daemon-log-level=trace
```
The log level can be set to any of the following values from the logrus package:
- trace
- debug
- info
- warn
- error
- fatal
- panic
To validate this PR on a local image I used minikube.
- build the images locally **make gadget-container** ( required as daemonset chart is updated with new env var )
- add the image to the local k8s cluster, in my case minikube via "minikube cache add ghcr.io/inspektor-gadget/inspektor-gadget:ghinks/cmd-deploy-log-level"
- make kubectl-gadget
- deploy via './kubectl-gadget deploy --daemon-log-level=trace --image-pull-policy=Never'
- the deployment was validated via 'k describe pod -n "gadget" $mypod'
- undeploy via 'kubectl-gadget undeploy'
Testing on minikube required a local image as minikube would try to pull the image from the registry. The image was built locally and added to the minikube cache.
```shell
export MY_CONTAINER="ghcr.io/inspektor-gadget/inspektor-gadget:ghinks-cmd-deploy-log-level"
minikube cache delete $MY_CONTAINER && minikube image rm $MY_CONTAINER && minikube cache add $MY_CONTAINER
```
To view the logs from the pod:
```shell
export MY_POD=`kubectl get pods -o jsonpath='{.items[*].metadata.name}' -n gadget`
k logs -n gadget $MY_POD -f
```
Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
- added trace level to clean up - ran make lint which resulted in a import order changes Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
- add an if clause for osx builds for the chart build Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
- use assign rather than append in str levels Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
Co-authored-by: eiffel-fl <laniel_francis@privacyrequired.com> Signed-off-by: Glenn <ghinks@yahoo.com> Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
ghinks
force-pushed
the
ghinks/cmd-deploy-log-level
branch
from
5f5a76d
to
4830632
Compare
alban
reviewed
Jan 8, 2024
- use sed based on the result of sed --version output Signed-off-by: Glenn Hinks <ghinks@yahoo.com>
eiffel-fl
reviewed
Jan 9, 2024
charts/Makefile
Outdated
|
|
||
| .DEFAULT_GOAL := help | ||
|
|
||
| clean: | ||
| rm -rf $(BUILD_DIR) | ||
|
|
||
| build: clean | ||
| # the --version option is only found in the GNU version of sed: | ||
| ifeq ($(shell sed --version >/dev/null 2>&1 ; echo $$?),0) | ||
| @echo Using GNU sed |
There was a problem hiding this comment.
This message will be printed twice.
You can just drop this if.
There was a problem hiding this comment.
I appear to have pushed up more than I intended and am removing that if statement.
There was a problem hiding this comment.
I have pushed again. Sincere thanks for taking the time to help me on this.
- remove duplicated if statements in makefile Signed-off-by: ghinks <ghinks@yahoo.com>
|
The CI explodes due to GitHub issues. diff --git a/gadget-container/cleanup/cleanup.go b/gadget-container/cleanup/cleanup.go
index 36fa8f93..8c52feeb 100644
--- a/gadget-container/cleanup/cleanup.go
+++ b/gadget-container/cleanup/cleanup.go
@@ -16,10 +16,11 @@ package main
import (
"encoding/json"
- "github.com/inspektor-gadget/inspektor-gadget/pkg/utils/gadgettracermanagerloglevel"
"os"
"path/filepath"
+ "github.com/inspektor-gadget/inspektor-gadget/pkg/utils/gadgettracermanagerloglevel"
+
nriv1 "github.com/containerd/nri/types/v1"
log "github.com/sirupsen/logrus"
diff --git a/gadget-container/entrypoint/entrypoint.go b/gadget-container/entrypoint/entrypoint.go
index 35bb1f36..2a808f38 100644
--- a/gadget-container/entrypoint/entrypoint.go
+++ b/gadget-container/entrypoint/entrypoint.go
@@ -19,18 +19,19 @@ import (
"encoding/json"
"errors"
"fmt"
- nriv1 "github.com/containerd/nri/types/v1"
- "github.com/inspektor-gadget/inspektor-gadget/pkg/oci"
- "github.com/inspektor-gadget/inspektor-gadget/pkg/utils/gadgettracermanagerloglevel"
- "github.com/inspektor-gadget/inspektor-gadget/pkg/utils/host"
- log "github.com/sirupsen/logrus"
- "golang.org/x/sys/unix"
"io/fs"
"os"
"path/filepath"
"regexp"
"strings"
"syscall"
+
+ nriv1 "github.com/containerd/nri/types/v1"
+ "github.com/inspektor-gadget/inspektor-gadget/pkg/oci"
+ "github.com/inspektor-gadget/inspektor-gadget/pkg/utils/gadgettracermanagerloglevel"
+ "github.com/inspektor-gadget/inspektor-gadget/pkg/utils/host"
+ log "github.com/sirupsen/logrus"
+ "golang.org/x/sys/unix"
)
const gadgetPullSecretPath = "/var/run/secrets/gadget/pull-secret/config.json"Can you please run |
- run linter for import issues Signed-off-by: ghinks <ghinks@yahoo.com>
eiffel-fl
approved these changes
Jan 10, 2024
There was a problem hiding this comment.
Thank you for the latest commit and for dealing with all my feedback!
This is a good contribution and it is really welcomed :D!
I tested it again and it works perfectly:
$ kubectl logs -n gadget gadget-lzjsc (remotes/ghinks/ghinks/cmd-deploy-log-level) %
time="2024-01-10T08:58:56Z" level=warning msg="failed to get cgroups info of container 015ef15f9bc38f91a54bef96bf3104c4fae39bef21597f9223890c2bde9dbbcd from /proc/5276/cgroup: cgroup path not found in /proc/PID/cgroup"
time="2024-01-10T08:58:56Z" level=error msg="cgroup enricher: failed to get cgroup paths on container 015ef15f9bc38f91a54bef96bf3104c4fae39bef21597f9223890c2bde9dbbcd: cgroup path not found in /proc/PID/cgroup"
time="2024-01-10T08:58:56Z" level=warning msg="failed to get cgroups info of container 015ef15f9bc38f91a54bef96bf3104c4fae39bef21597f9223890c2bde9dbbcd from /proc/5276/cgroup: cgroup path not found in /proc/PID/cgroup"I will squash and merge it!
|
Thank you for all the help @eiffel-fl. Having an open source booth at Kubecon did have a benefit for you folks. |
You are welcome and thank you for having the patience to handle all my comments! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Provide a way to set log level in ig-k8s pod issue 1869
Add a new command line option to ig-k8s to set the log level via --daemon-log-level. This is useful for debugging issues in the field. The new command will set the environment variable GADGET_TRACER_MANAGER_LOG_LEVEL in the ig-k8s pod. The default value will be info.
How to use
To set the GADGET_TRACER_MANAGER_LOG_LEVEL to trace, run the following command:
The log level can be set to any of the following values from the logrus package:
To validate this PR on a local image I used minikube.
Testing on minikube required a local image as minikube would try to pull the image from the registry. The image was built locally and added to the minikube cache.
To view the logs from the pod: