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

Design proposal for new Debug Events #3122

Merged
merged 4 commits into from
Jan 27, 2020

Conversation

briandealwis
Copy link
Member

@briandealwis briandealwis commented Oct 24, 2019

Description

Design proposal for new Debug Events to be raised by skaffold debug.

Issue: #2211

User facing changes

n/a: no such events are currently being emitted, and existing events are unchanged.

Reviewer Notes

  • The code flow looks good.
  • Unit test added.
  • User facing changes look good.

@briandealwis
Copy link
Member Author

"event": {
"debugEvent": {
"status": "Started",
"podName": "npm",

Choose a reason for hiding this comment

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

What is the best way to map a debugEvent to a portEvent? Is it by comparing podName and containerName?

Copy link
Member Author

Choose a reason for hiding this comment

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

And namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

The debug-events will be raised before the port-events since debug-events will be raised when the container is started, and that's when the port-forward will be initiated.

@codecov
Copy link

codecov bot commented Oct 24, 2019

Codecov Report

Merging #3122 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted Files Coverage Δ
pkg/skaffold/jib/jib_init.go 89.55% <0%> (+0.66%) ⬆️

Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

this looks pretty good to me. one question I have is, will this surface any changes to the internal state tracked by skaffold during the run? i.e. the payload the you get back from hitting the /v1/state endpoint.

docs/design_proposals/debug-events.md Outdated Show resolved Hide resolved
docs/design_proposals/debug-events.md Outdated Show resolved Hide resolved
docs/design_proposals/debug-events.md Outdated Show resolved Hide resolved
docs/design_proposals/debug-events.md Outdated Show resolved Hide resolved
docs/design_proposals/debug-events.md Outdated Show resolved Hide resolved
require a `kubectl exec` to launch a process in the remote container.
- Port-forwards may be dropped and re-established

There may also be a desire for the IDEs to selectively initiate port-forwards on an as-neeeded
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 actually pretty huge. so we could just send a debug payload to skaffold, and request to open up a debug session on a port at any point during the dev session right? in theory, this would allow debugging during a normal skaffold dev loop wouldn't it? or is it required that we modify the container podspecs beforehand

Copy link
Member Author

Choose a reason for hiding this comment

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

We'd still need to have modified the pod specs, though that could be done on-the-fly.

@briandealwis
Copy link
Member Author

Added example of state addition too.

- Identify the corresponding artifact from a port-forward event by correlating the
remote port with the debug port from the previously-raised debug event
- Establish new debug launch configurations with the forwarded port,
and the local source location, and any other necessary information

Choose a reason for hiding this comment

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

As the local source location is needed by both IJ and VSCode extensions, can we have a way to store this info in the skaffold.yaml configuration?

Choose a reason for hiding this comment

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

With this, we don't need to create a 'debug' node in the vscode launch configuration.

Copy link
Member Author

Choose a reason for hiding this comment

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

The local source root is unfortunately not really clear — certainly the debug portion has no more information than the skaffold.yaml's artifact context directory. Consider a source layout like:

path/to/my/artifact/
├── Dockerfile
└── some
    └── deep
        └── directory
            └── RandomPython.py
            └── RandomPython.py
            └── RandomPython.py
            └── RandomPython.py
└── another
    └── deep
        └── directory
            └── RandomPython.py
            └── RandomPython.py
            └── RandomPython.py
            └── RandomPython.py

where the Dockerfile is:

COPY some/deep /random/place/in/image
COPY another/deep/directory /another/place/in/image
WORKDIR /a/different/location

I suspect we'll find it worthwhile to develop a little Go utility to guess at possible source roots, (if something like this doesn't already exist?).

@tejal29
Copy link
Member

tejal29 commented Jan 27, 2020

DD looks good to me. As we discussed the "optional the container image's remote root for source file resolving" is the only matter of concern. If IDEs could resolve it, it would be great!

#LGTM

@tejal29 tejal29 merged commit 3214e7e into GoogleContainerTools:master Jan 27, 2020
@briandealwis briandealwis mentioned this pull request Jan 29, 2020
10 tasks
@briandealwis briandealwis mentioned this pull request Feb 5, 2020
7 tasks
@briandealwis briandealwis deleted the debugeventsproposal branch April 3, 2020 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants