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.

Sign up for GitHub

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

Jump to bottom

Makefile: new target kind-debug to debug cilium operator & agent in kind cluster #23898

Merged
merged 14 commits into from Mar 1, 2023
Merged

Makefile: new target kind-debug to debug cilium operator & agent in kind cluster #23898

merged 14 commits into from Mar 1, 2023

Conversation

mhofstetter
Copy link
Member

@mhofstetter mhofstetter commented Feb 21, 2023 •
edited

A new make target kind-debug has been added. In addition to make kind-debug-agent it adds the possibility to debug the cilium operator beside the agents.

It follows the same ideas which has been introduced for the cilium agent in #21108

To allow easy remote-debugging of the operator, the following points has been addressed:

  • operator gets pinned to kind-worker with nodeSelector
  • operator debug port gets exposed on every kind node (for the sake of completeness) (controlplane: 2350x, worker: 2351x)(possible due to podAntiAffinity -> only one operator pod per node anyway)
  • operator HA mode isn't addressed (operator doesn't get deployed this way anyway)
  • Use of the delve flag --continue to remove the need to attach a debugger to all instances. (deactivate by setting environment variable DEBUG_HOLD=true make kind-debug)

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 21, 2023
@mhofstetter mhofstetter added the release-note/misc This PR makes changes that have no direct user impact. label Feb 21, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 21, 2023
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/kind-debug-operator branch from 793021a to 0d7b5e9 Compare February 21, 2023 11:38
@mhofstetter mhofstetter marked this pull request as ready for review February 21, 2023 12:04
@mhofstetter mhofstetter requested review from a team as code owners February 21, 2023 12:04
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/kind-debug-operator branch from 0d7b5e9 to b755893 Compare February 21, 2023 17:39
@mhofstetter
Copy link
Member Author

/test

@mhofstetter mhofstetter force-pushed the pr/mhofstetter/kind-debug-operator branch from b755893 to c82cb75 Compare February 21, 2023 17:47
@mhofstetter
Copy link
Member Author

mhofstetter commented Feb 21, 2023 •
edited

runtime container images were outdated

/test

joestringer
joestringer reviewed Feb 21, 2023
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Nice work, thanks! I have a couple of questions about the details in the operator images where ideally we could reduce the amount of duplicate logic. Then there's one more discussion point I'm not sure about the resolution yet but where it may help to have additional input from the developer community.

images/operator/Dockerfile Outdated Show resolved Hide resolved
images/operator/Dockerfile Outdated Show resolved Hide resolved
images/scripts/debug-wrapper.sh Outdated Show resolved Hide resolved
@mhofstetter
Copy link
Member Author

mhofstetter commented Feb 22, 2023 •
edited

ci-verifier required but still not triggered automatically in case of missing datapath changes

/ci-verifier

@mhofstetter
Copy link
Member Author

/test-1.16-4.19

@mhofstetter mhofstetter force-pushed the pr/mhofstetter/kind-debug-operator branch from 4f80fbf to 03ce875 Compare February 23, 2023 11:06
@mhofstetter
Copy link
Member Author

rebased to master to fix conflicts with updated go image

joestringer
joestringer requested changes Feb 23, 2023
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Just some minor nits but the latest version looks good overall. I'm just not 100% sure about how to ensure these changes end up in the canonical builder images and how we can ensure all dependent images are synced up properly.

(I also didn't manually test this myself)

images/builder/debug-wrapper.go Outdated Show resolved Hide resolved
images/builder/debug-wrapper.go Outdated Show resolved Hide resolved
images/operator/Dockerfile Outdated Show resolved Hide resolved
images/operator/Dockerfile Outdated Show resolved Hide resolved
aanm
aanm reviewed Feb 24, 2023
View reviewed changes
images/operator/Dockerfile Outdated Show resolved Hide resolved
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/kind-debug-operator branch from 03ce875 to a68d32d Compare February 27, 2023 07:19
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/kind-debug-operator branch from a68d32d to f9de268 Compare February 27, 2023 07:50
@mhofstetter
.nvim: Add cilium operator debug target
f75c455 
This commit adds the debug target for debugging the cilium operator
within a k8s kind cluster.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
.vscode: Add cilium operator debug target
def4386 
This commit adds the debug target for debugging the cilium operator
within a k8s kind cluster.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
images: configurable debug port in debug-wrapper.sh
41588a8 
This commit adds the possibility to configure the debug port as
environment variable DEBUG_PORT. It still defaults to 2345.

This adds the possibility to use the debug-wrapper for the cilium
operator which needs to expose the debugger on a different port than the
agent.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
Makefile: add make target kind-debug
8d9e9f7 
This commit adds a new make target kind-debug which builds & deploys the
cilium agent & operator in a local kind cluster for debugging with
delve.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
Makefile: silence output of creating .dockerignore files
34e6103 
Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
contrib: Expose cilium operator debug port in kind setup
b806fe3 
With this commit, the operator debug port gets exposed on every kind
node. In addition, the operator pod gets pinned to node kind-worker (via
a nodeSelector on the hostname) which eases debuggability. This is
possible due to the default podAntiAffinity, which ensures, that only
one operator pod is running on one k8s node.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
images: docker target debug for cilium operator
866d6fe 
This commit introduces a new docker target debug which provides the
possibility to build a cilium operator image which includes the
debug-wrapper.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
images: optionally continue delve debugger at startup via flag --cont…
3d54637 
…inue

By passing the environment variable DEBUG_CONTINUE=true to the make
target, the devle flag --continue is set. This way, the cilium agent
& operator don't wait for an attached debugger.

Without this change, it might gets a tedious action to attach the
debugger to all pods.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
images: install delve without CGO
7e3f66b 
Delve should be go installed with CGO=0 in the cilium builder image.
This enables to use the binary in images based upon scratch (e.g. cilium
operator)

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
images: build debug-wrapper binary in cilium builder image
282b639 
the debug wrapper go binary should be built and put into the builder
image alongside installing the delve binary.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
images: remove debug-wrapper based on bash
aacf3c3 
the debug-wrapper based on bash has been replaced with a statically
linked go binary which is also usable in images not having bash on its
PATH.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/kind-debug-operator branch from f9de268 to 4c6f2a4 Compare February 27, 2023 08:04
@mhofstetter
images: using go binary of debug-wrapper in agent & operator images
757e024 
By replacing the bash-based debug-wrapper with a statically linked go
binary - the debug image of the operator can base upon the release stage
which doesn't include bash (scratch).

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
images: continue delve debugger by default at startup
7cdc47b 
By default, the debugger continues at startup. If this is undesired, the
environment variable DEBUG_HOLD=true can be set to build the image with
the delve flag --continue=false.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/kind-debug-operator branch from 4c6f2a4 to 7cdc47b Compare February 27, 2023 08:05
@mhofstetter mhofstetter temporarily deployed to release-base-images February 27, 2023 13:43 — with GitHub Actions Inactive
@mhofstetter
images: update cilium-{runtime,builder}
16e4759 
Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter mhofstetter requested a review from a team as a code owner February 27, 2023 14:06
@mhofstetter
Copy link
Member Author

/test

joestringer
joestringer approved these changes Feb 27, 2023
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@mhofstetter
Copy link
Member Author

mhofstetter commented Feb 28, 2023 •
edited

not automatically triggered / reported

/ci-verifier

tklauser
tklauser approved these changes Feb 28, 2023
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Thanks!

images/operator/Dockerfile
# Typical image bulids will stop above at the 'release' target, but
# developers follow this Dockerfile to the end. Starting from a release
# image, install delve debugger and wrap the cilium-operator binary calls
# with a script that automatically provisions the debugger on a
Copy link
Member

Choose a reason for hiding this comment

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

Super nit:

Suggested change
# with a script that automatically provisions the debugger on a
# with the statically linked debug-wrapper binary that automatically provisions the debugger on a

Only worth addressing in case there are other change requests.

@mhofstetter mhofstetter removed the request for review from jibi March 1, 2023 16:33
@joestringer joestringer merged commit f8af281 into cilium:master Mar 1, 2023
1 check failed
@mhofstetter mhofstetter deleted the pr/mhofstetter/kind-debug-operator branch March 1, 2023 18:31
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm left review comments

@tklauser tklauser tklauser approved these changes

@joestringer joestringer joestringer approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mhofstetter @tklauser @joestringer @aanm