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

Add Tilt configuration to debug using Delve #3189

Merged
merged 4 commits into from
Jan 22, 2021

Conversation

zubron
Copy link
Contributor

@zubron zubron commented Dec 15, 2020

This change adds support to run the Velero process in Tilt using
Delve.
This does not include support for debugging the Velero process in the
restic pods, just in the Velero deployment.

For an optimal debugging experience, this change also introduces a new
flag (DEBUG) to the hack/build.sh script to enable a "debug" build
of the Velero binary. This flag, if enabled, will build the binary
without optimisations and inlining. Disabling optimisations and inlining
is recommended by Delve.

Two configuration options have been added to the Tilt settings. The
first, enable_debug, is to control whether debugging should be
enabled. If enabled, the process will be started by Delve, and the Delve
server port (2345) will be forwarded to the local machine.
The second option, debug_continue_on_start, is to control whether the
process should "continue" when started by Delve or should be paused.
By default, debugging is disabled, and if in debug mode, the process
will continue.

Signed-off-by: Bridget McErlean bmcerlean@vmware.com

@zubron zubron added Enhancement/Dev Internal or Developer-focused Enhancement to Velero kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes labels Dec 15, 2020
@@ -51,6 +51,11 @@ if [[ -z "${GIT_TREE_STATE}" ]]; then
exit 1
fi

GCFLAGS=""
if [[ ${DEBUG:-} = "1" ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a better way to make a boolean flag in bash?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an option:

if [[ -n "${DEBUG-}" ]]; then

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe there is, but my bash knowledge is admittedly incomplete and sporadic.

@carlisia
Copy link
Contributor

Tip for anyone trying to test this:

Be sure to copy all or the bits in the examples folder that contain these changes.

If you use vscode, use this configuration for the debugger:

{
    "version": "0.2.0",
    "configurations": [
        {
            "name": "Delv",
            "type": "go",
            "request": "attach",
            "mode": "remote",
            "cwd": "${workspaceFolder}",
            "remotePath": "",
            "port": 2345,
            "host": "127.0.0.1",
            "showLog": true,
            "trace": "verbose"
        }
    ]
}

It worked perfectly for me!

Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

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

It works perfectly! 🎉

Some minor stuff.

Tiltfile Outdated
}

# global settings
settings.update(read_json(
"tilt-resources/tilt-settings.json",
default = {},
default={},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a best practices for starlark that motivated the changes to remove spaces? When I look at the docs, the examples contain spaces: https://docs.bazel.build/versions/master/skylark/language.html. If not, there is no reason for this and the other similar changes, please revert and make the additions consistent with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just did a "reformat" in my editor and it applied the PEP8 standard to remove the spaces around keyword args. Starlark prefers spaces around the = in keyword args so I will revert this! I'll keep some of the other formatting changes though (e.g. removing spaces at ends of lines).

Tiltfile Outdated
# Support live reloading with Tilt
RUN wget --output-document /restart.sh --quiet https://raw.githubusercontent.com/windmilleng/rerun-process-wrapper/master/restart.sh && \
wget --output-document /start.sh --quiet https://raw.githubusercontent.com/windmilleng/rerun-process-wrapper/master/start.sh && \
chmod +x /start.sh && chmod +x /restart.sh
"""

additional_docker_helper_commands = """
# Install delve to allow debugging
RUN go get github.com/go-delve/delve/cmd/dlv
RUN which dlv
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the which?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, this is leftover from checking the path for the dlv binary :) Will remove!

@@ -51,6 +51,11 @@ if [[ -z "${GIT_TREE_STATE}" ]]; then
exit 1
fi

GCFLAGS=""
if [[ ${DEBUG:-} = "1" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an option:

if [[ -n "${DEBUG-}" ]]; then

Tiltfile Outdated
)

local_resource(
"velero_local_binary",
cmd = 'cd ' + '.' + ';mkdir -p _tiltbuild/local;PKG=. BIN=velero GOOS=' + local_goos + ' GOARCH=amd64 GIT_SHA=' + git_sha + ' VERSION=main GIT_TREE_STATE=dirty OUTPUT_DIR=_tiltbuild/local ./hack/build.sh',
deps = ["internal", "pkg/cmd"],
cmd='cd ' + '.' + ';mkdir -p _tiltbuild/local;PKG=. BIN=velero GOOS=' + local_goos + ' GOARCH=amd64 GIT_SHA=' + git_sha + ' VERSION=main GIT_TREE_STATE=dirty OUTPUT_DIR=_tiltbuild/local ./hack/build.sh',
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add the debug config to the local binary too so we can debug the CLI.

By enabling debug mode, the Velero executable will be built in debug mode (using the flags `-gcflags="-N -l"` which disables optimizations and inlining), and the process will be started in the Velero deployment using [`dlv exec`](https://github.com/go-delve/delve/blob/master/Documentation/usage/dlv_exec.md).

The debug server will accept connections on port 2345 and Tilt is configured to forward this port to the local machine.
To connect to the session, you can use the Delve CLI locally by running `dlv connect 127.0.0.1:2345`. See the [Delve CLI documentation](https://github.com/go-delve/delve/tree/master/Documentation/cli) for more guidance on how to use Delve.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's good to add a tiny note to run this after starting up Tilt.

carlisia
carlisia previously approved these changes Dec 18, 2020
Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

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

This is awesome! 🎉

Copy link
Contributor

@jenting jenting left a comment

Choose a reason for hiding this comment

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

I have only one concern, will this enhancement release at 1.5.x?
If not, then I think we don't need to update doc v1.5/title.md.

Copy link
Contributor

@nrb nrb left a comment

Choose a reason for hiding this comment

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

I agree with Jenting, we likely don't need the v1.5 doc unless we're planning on backporting this.

Other than that this looks super useful and I just have a couple questions that I don't consider blockers.

"enable_debug": False,
"debug_continue_on_start": True, # Continue the velero process by default when in debug mode
"create_backup_locations": False,
"setup-minio": False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to standardize these so that setup-minio becomes setup_minio?

@@ -51,6 +51,11 @@ if [[ -z "${GIT_TREE_STATE}" ]]; then
exit 1
fi

GCFLAGS=""
if [[ ${DEBUG:-} = "1" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe there is, but my bash knowledge is admittedly incomplete and sporadic.

@@ -141,6 +148,26 @@ to learn what field/value pairs are required for your provider's credentials. Th

There is a sample credentials file properly formatted for a MinIO storage credentials in `velero/tilt-resources/examples/cloud`.

### Configure debugging with Delve
If you would like to debug the Velero process, you can enable debug mode by setting the field `enable_debug` to `true` in your `tilt-resources/tile-settings.json` file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this flag also enable debug logging, or is that too much noise do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could, but this is also controlled in the deployment yaml file that is used by Tilt. Perhaps another option would be to instead have a "log_level" option in the tilt settings so that it can be managed in the same file, but can be configured independently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Putting a separate logging flag in works for me. I'd hazard to say debug logging should be on by default in Tilt setups, but I also think that discussion might be better suited elsewhere.

@zubron
Copy link
Contributor Author

zubron commented Jan 20, 2021

@nrb @jenting I included the docs in the 1.5 branch as that serves as our "latest" docs and we have already included the existing Tilt instructions there (which were added post 1.5). Without this, it would require navigating to the "main" version of our docs. You're right though that this isn't backported to the 1.5 branch. It's a bit awkward but it made sense to me that the latest version of our developer documentation be in the default "latest" view.

@nrb
Copy link
Contributor

nrb commented Jan 21, 2021

It's a bit awkward but it made sense to me that the latest version of our developer documentation be in the default "latest" view.

That's a good point. If you're developing on Velero, you'd like want this documentation front and center, and not have to look at a different branch of the docs to get started. That's a bit of a disconnect I don't know how to bridge at the moment. At the very least, we'd have to have a link to the main branch of docs.

Given that point, I think it would be fine to leave it in the v1.5 docs for the time being.

@nrb
Copy link
Contributor

nrb commented Jan 21, 2021

@zubron I'm good with this as is if you'd like to resolve the Tiltfile conflict

This change adds support to run the Velero process in Tilt using
[Delve](https://github.com/go-delve/delve).
This does not include support for debugging the Velero process in the
restic pods, just in the Velero deployment.

For an optimal debugging experience, this change also introduces a new
flag (`DEBUG`) to the `hack/build.sh` script to enable a "debug" build
of the Velero binary. This flag, if enabled, will build the binary
without optimisations and inlining. Disabling optimisations and inlining
is recommended by Delve.

Two configuration options have been added to the Tilt settings. The
first, `enable_debug`, is to control whether debugging should be
enabled. If enabled, the process will be started by Delve, and the Delve
server port (2345) will be forwarded to the local machine.
The second option, `debug_continue_on_start`, is to control whether the
process should "continue" when started by Delve or should be paused.
By default, debugging is disabled, and if in debug mode, the process
will continue.

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
Starlark prefers spaces around `=` in keyword arguments:
https://docs.bazel.build/versions/master/skylark/bzl-style.html#keyword-arguments

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
@jenting jenting merged commit a42284e into vmware-tanzu:main Jan 22, 2021
dharmab pushed a commit to dharmab/velero that referenced this pull request May 25, 2021
* Add Tilt configuration to debug using Delve

This change adds support to run the Velero process in Tilt using
[Delve](https://github.com/go-delve/delve).
This does not include support for debugging the Velero process in the
restic pods, just in the Velero deployment.

For an optimal debugging experience, this change also introduces a new
flag (`DEBUG`) to the `hack/build.sh` script to enable a "debug" build
of the Velero binary. This flag, if enabled, will build the binary
without optimisations and inlining. Disabling optimisations and inlining
is recommended by Delve.

Two configuration options have been added to the Tilt settings. The
first, `enable_debug`, is to control whether debugging should be
enabled. If enabled, the process will be started by Delve, and the Delve
server port (2345) will be forwarded to the local machine.
The second option, `debug_continue_on_start`, is to control whether the
process should "continue" when started by Delve or should be paused.
By default, debugging is disabled, and if in debug mode, the process
will continue.

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

* Add spaces around keyword args

Starlark prefers spaces around `=` in keyword arguments:
https://docs.bazel.build/versions/master/skylark/bzl-style.html#keyword-arguments

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

* Remove unnecessary command from Dockerfile

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

* Add note to connect after Tilt is running

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
ywk253100 pushed a commit to ywk253100/velero that referenced this pull request Jun 29, 2021
* Add Tilt configuration to debug using Delve

This change adds support to run the Velero process in Tilt using
[Delve](https://github.com/go-delve/delve).
This does not include support for debugging the Velero process in the
restic pods, just in the Velero deployment.

For an optimal debugging experience, this change also introduces a new
flag (`DEBUG`) to the `hack/build.sh` script to enable a "debug" build
of the Velero binary. This flag, if enabled, will build the binary
without optimisations and inlining. Disabling optimisations and inlining
is recommended by Delve.

Two configuration options have been added to the Tilt settings. The
first, `enable_debug`, is to control whether debugging should be
enabled. If enabled, the process will be started by Delve, and the Delve
server port (2345) will be forwarded to the local machine.
The second option, `debug_continue_on_start`, is to control whether the
process should "continue" when started by Delve or should be paused.
By default, debugging is disabled, and if in debug mode, the process
will continue.

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

* Add spaces around keyword args

Starlark prefers spaces around `=` in keyword arguments:
https://docs.bazel.build/versions/master/skylark/bzl-style.html#keyword-arguments

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

* Remove unnecessary command from Dockerfile

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

* Add note to connect after Tilt is running

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
gyaozhou pushed a commit to gyaozhou/velero-read that referenced this pull request May 14, 2022
* Add Tilt configuration to debug using Delve

This change adds support to run the Velero process in Tilt using
[Delve](https://github.com/go-delve/delve).
This does not include support for debugging the Velero process in the
restic pods, just in the Velero deployment.

For an optimal debugging experience, this change also introduces a new
flag (`DEBUG`) to the `hack/build.sh` script to enable a "debug" build
of the Velero binary. This flag, if enabled, will build the binary
without optimisations and inlining. Disabling optimisations and inlining
is recommended by Delve.

Two configuration options have been added to the Tilt settings. The
first, `enable_debug`, is to control whether debugging should be
enabled. If enabled, the process will be started by Delve, and the Delve
server port (2345) will be forwarded to the local machine.
The second option, `debug_continue_on_start`, is to control whether the
process should "continue" when started by Delve or should be paused.
By default, debugging is disabled, and if in debug mode, the process
will continue.

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

* Add spaces around keyword args

Starlark prefers spaces around `=` in keyword arguments:
https://docs.bazel.build/versions/master/skylark/bzl-style.html#keyword-arguments

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

* Remove unnecessary command from Dockerfile

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

* Add note to connect after Tilt is running

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement/Dev Internal or Developer-focused Enhancement to Velero kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants