-
Notifications
You must be signed in to change notification settings - Fork 418
Improve debugging experience for k0s inttests #5878
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
base: main
Are you sure you want to change the base?
Conversation
31e02c6
to
535b0c9
Compare
535b0c9
to
1d0a016
Compare
Implements an optional, heavier debug image with delve and strace, a way to replace the command in the BootlooseSuite for a debugging command and documents it. Signed-off-by: Juan-Luis de Sousa-Valadas Castaño <[email protected]>
1d0a016
to
d5eab45
Compare
Closing and reopening to retrigger CI as github is having problems |
# Determine the bootloose stamp dependency based on K0S_DEBUG | ||
# Default to .bootloose-alpine.stamp (the "otherwise" case from the request) | ||
BOOTLOOSE_STAMP_DEP := .bootloose-alpine.stamp | ||
ifeq ($(K0S_DEBUG_TESTS),true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should rename this variable to DEBUG, which is used to build a non stripped k0s binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. OTOH, we could have a richer syntax for automatically enabling delve, which would have a different syntax. We could then simply enable DEBUG, based on the presence/non-emptiness of the other variable.
The PR is marked as stale since no activity has been recorded in 30 days |
done | ||
|
||
RUN if [ "$INCLUDE_DEBUG_TOOLS" = "true" ]; then \ | ||
apk add --no-cache strace \ | ||
&& go install github.com/go-delve/delve/cmd/dlv@latest; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also just take delve from the Alpine repos: apk add --repository http://dl-cdn.alpinelinux.org/alpine/edge/community delve
. In that case, we could even spare all the base image juggling.
It's possible to attach a debugger to a smoke test but it requires some modifications: | ||
|
||
1. k0s must be compiled with symbols: `DEBUG=true make k0s` | ||
2. The source code must be present in `go/src/github.com/k0sproject/` and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's achieved by this second point? I was able debug stuff without doing this IIRC.
ln -s <K0s source dir>/build/cache/go /run/k0s-build/go | ||
``` | ||
|
||
3. Modify the inttest to include to run the command that you want to run: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider to support this without having to actively edit the code? Say, have a Make variable K0S_INTTEST_DEBUG_PORT=4000
which automatically selects the debug image and enables the debugger. We could also add something like "K0S_INTTEST_DEBUG=controller1:4000,controller2:5000" or sth. along those lines.
### Debugging smoke tests | ||
|
||
Every smoke test can be executed independently, the complete list of smoke tests is | ||
available in `inttest/Makefile.variables`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
available in `inttest/Makefile.variables`` | |
available in `inttest/Makefile.variables`. |
Fun fact: You can print them to stdout via ./vars.sh FROM=inttest smoketests
as well.
s.Require().NoError(s.RunWorkers()) | ||
``` | ||
|
||
4. Launch the tests with `K0S_DEBUG_TEST=true` so that it uses the correct image: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4. Launch the tests with `K0S_DEBUG_TEST=true` so that it uses the correct image: | |
4. Launch the tests with `K0S_DEBUG_TESTS=true` so that it uses the correct image: |
BOOTLOOSE_STAMP_DEP := .bootloose-alpine.stamp | ||
ifeq ($(K0S_DEBUG_TESTS),true) | ||
BOOTLOOSE_STAMP_DEP := .bootloose-debug.stamp | ||
BOOTLOOSE_IMAGE ?= bootloose-debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest a lower case name to indicate this is not something that should come via the env.
$(smoketests): K0S_PATH ?= $(realpath ../k0s) | ||
$(smoketests): K0S_IMAGES_BUNDLE ?= $(realpath ../airgap-image-bundle-linux-$(ARCH).tar) | ||
$(smoketests): .bootloose-alpine.stamp | ||
$(smoketests): $(BOOTLOOSE_STAMP_DEP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd ditch that variable in favor of
$(smoketests): $(BOOTLOOSE_STAMP_DEP) | |
$(smoketests): .$(BOOTLOOSE_IMAGE).stamp |
--build-arg GOLANG_IMAGE=$(golang_buildimage) \ | ||
--build-arg ALPINE_VERSION=$(alpine_patch_version) \ | ||
--build-arg ETCD_VERSION=$(etcd_version) \ | ||
--build-arg HELM_VERSION=$(helm_version) \ | ||
-t bootloose-alpine \ | ||
--build-arg TARGETARCH=$(ARCH) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is breaking make check-bootloose-alpine-buildx
. You can't include TARGETARCH here, and you need to include BOOTLOOSE_BASE.
@@ -1,13 +1,13 @@ | |||
ARG ALPINE_VERSION | |||
ARG GOLANG_IMAGE | |||
ARG BOOTLOOSE_BASE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just BASE_IMAGE?
@@ -50,22 +50,27 @@ RUN curl --proto '=https' --tlsv1.2 -L https://get.helm.sh/helm-v$HELM_VERSION-l | |||
# Install etcd for smoke tests with external etcd | |||
# No arm or riscv64 binaries available (check-externaletcd won't work on ARMv7 or RISC-V) | |||
RUN if [ "$TARGETARCH" != arm ] && [ "$TARGETARCH" != riscv64 ]; then \ | |||
curl --proto '=https' --tlsv1.2 -L https://github.com/etcd-io/etcd/releases/download/v$ETCD_VERSION/etcd-v$ETCD_VERSION-linux-$TARGETARCH.tar.gz \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated whitespace change here and in the following lines.
Description
Attaching a debugger or running the inttests with strace is super annoying and every time I need to do it I end up having to make a custom image and modifying the Bootloose suite or launching myself the k0s command. We should have a standardized way of doing this where we don't need to think and it's just a command to get that done.
This PR implements an optional, heavier debug image with delve and strace, a way to replace the command in the BootlooseSuite for a debugging command and documents it.
Type of change
How Has This Been Tested?
Checklist