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

🌱 Tiltfile: Enable debugging with delve #5485

Merged
merged 1 commit into from
Oct 26, 2021

Conversation

randomvariable
Copy link
Member

@randomvariable randomvariable commented Oct 23, 2021

Signed-off-by: Naadir Jeewa [email protected]

What this PR does / why we need it:

After chatting with @sbueringer , been thinking of ways of making using debuggers easier.
This enables adding the following in tilt-settings.json, for example, to enable a remote debugger to be attached to the core controller on 127.0.0.1:30000 when using Tilt:

  "debug": {
    "core": {
      "race_detector": true,
      "port": 30000,
      "profiler_port": 40000,
      "metrics_port": 40001
    },
    "kubeadm-bootstrap": {
      "port": 30001
    },
    "kubeadm-control-plane": {
      "port": 30002
    },
    "docker": {
      "port": 30003
    },
    "aws": {
      "port": 30004
    }
  },

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 23, 2021
@randomvariable randomvariable force-pushed the tilt-debug branch 2 times, most recently from 13e1fbc to ff58d0b Compare October 23, 2021 01:58
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 23, 2021
@randomvariable randomvariable changed the title [wip] Tiltfile: Enable debugging with delve Tiltfile: Enable debugging with delve Oct 23, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 23, 2021
@randomvariable randomvariable changed the title Tiltfile: Enable debugging with delve 🌱 Tiltfile: Enable debugging with delve Oct 23, 2021
@randomvariable randomvariable force-pushed the tilt-debug branch 2 times, most recently from 1563a94 to b8d2ba1 Compare October 23, 2021 02:01
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

That will be super useful! Thanks for driving this!

docs/book/src/developer/tilt.md Outdated Show resolved Hide resolved
docs/book/src/developer/tilt.md Show resolved Hide resolved
@randomvariable
Copy link
Member Author

/hold

Would like someone on MacOS to test. This works on Linux, but on MacOS we have the complexity of Docker Machine. However, given that this is using port forwarding via the API server, it shouldn't require additional networking hacks.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 25, 2021
@sbueringer
Copy link
Member

/hold

Would like someone on MacOS to test. This works on Linux, but on MacOS we have the complexity of Docker Machine. However, given that this is using port forwarding via the API server, it shouldn't require additional networking hacks.

I'll test it

docs/book/src/developer/tilt.md Show resolved Hide resolved
Tiltfile Outdated Show resolved Hide resolved
Tiltfile Outdated Show resolved Hide resolved
@sbueringer
Copy link
Member

sbueringer commented Oct 25, 2021

@randomvariable Looks great and works on MacOS, thx!!

A few comments, mostly nits, but imho it would be great to default to using dlv with --continue.

@randomvariable randomvariable force-pushed the tilt-debug branch 3 times, most recently from a25506b to 6a9184f Compare October 25, 2021 15:28
@randomvariable
Copy link
Member Author

/unhold

Have addressed the comments.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 25, 2021
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

lgtm pending one question about go version bump

Makefile Outdated Show resolved Hide resolved
This enables adding the following in tilt-settings.json, for example, to enable a remote debugger to be attached to the core controller on 127.0.0.1:30000 when using Tilt:

```
  "debug": {
    "core": {
      "race_detector": true,
      "port": 30000,
      "profiler_port": 40000,
      "metrics_port": 40001
    },
    "kubeadm-bootstrap": {
      "port": 30001
    },
    "kubeadm-control-plane": {
      "port": 30002
    },
    "docker": {
      "port": 30003
    },
    "aws": {
      "port": 30004
    }
  },
```

Signed-off-by: Naadir Jeewa <[email protected]>
@fabriziopandini
Copy link
Member

/lgtm
over to @vincepri @CecileRobertMichon (mostly because we add yq to our toolset, but this seems straight forward IMO)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2021
@sbueringer
Copy link
Member

Very nice!
/lgtm

@randomvariable
Copy link
Member Author

randomvariable commented Oct 25, 2021

over to @vincepri @CecileRobertMichon (mostly because we add yq to our toolset, but this seems straight forward IMO)

I'm intending (in another PR) to extend the usage of yq to get rid of editing manifests in place whenever we do e2es as well.

Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

Thank you @randomvariable, this is great!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2021
@k8s-ci-robot k8s-ci-robot merged commit ee811e0 into kubernetes-sigs:main Oct 26, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.1 milestone Oct 26, 2021
@randomvariable randomvariable deleted the tilt-debug branch October 26, 2021 14:48
k8s_yaml(blob(yaml))

manager_name = p.get("manager_name")
if manager_name:
Copy link
Contributor

@mboersma mboersma Oct 26, 2021

Choose a reason for hiding this comment

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

Since this if line was removed, I get this stack trace when running tilt up:

local: ./hack/tools/bin/kustomize build ../cluster-api-provider-azure/config/default | ./hack/tools/bin/envsubst 
Traceback (most recent call last):
  /Users/matt/projects/cluster-api/Tiltfile:342:17: in <toplevel>
  /Users/matt/projects/cluster-api/Tiltfile:309:24: in enable_providers
  /Users/matt/projects/cluster-api/Tiltfile:288:21: in enable_provider
  <builtin>: in k8s_resource
Error: k8s_resource: for parameter "workload": Value should be convertible to string, but is type NoneType

My debugging configuration was happy before, and it's happy when I restore that line. Should I make a PR to restore the if check?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants