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

Pause debug pod watchers before next iteration deploy #5932

Merged

Conversation

tejal29
Copy link
Contributor

@tejal29 tejal29 commented Jun 3, 2021

Fixes #5914

See #5914 (comment) for more details on why we need to stop the debug manager between deploys.

Changes in this PR

  1. Add a Watcher Interface for PortforwardManager and DebugManager with Start and Stop methods.
  2. Call Stop for DebugManager similar to what we do for PortforwardManager.
  3. Remove close from DebugManager.Stop since the doc mention close should only be called by sender. Changed the DebugManager.Stop to stop the pod watchers.

Verified locally, we don't see an DebugContainerEvent in StatusCheck phase.

@tejal29 tejal29 requested a review from a team as a code owner June 3, 2021 00:27
@tejal29 tejal29 requested a review from briandealwis June 3, 2021 00:27
@google-cla
Copy link

google-cla bot commented Jun 3, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Jun 3, 2021
@tejal29 tejal29 force-pushed the pause_debug_pod_watchers branch from 2042917 to 4cb40a9 Compare June 3, 2021 00:30
@google-cla
Copy link

google-cla bot commented Jun 3, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@tejal29
Copy link
Contributor Author

tejal29 commented Jun 3, 2021

@googlebot I fixed it

@google-cla
Copy link

google-cla bot commented Jun 3, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@tejal29 tejal29 added cla: yes and removed cla: no labels Jun 3, 2021
@tejal29 tejal29 changed the title Pause debug pod watchers before next iteration Pause debug pod watchers before next iteration deploy Jun 3, 2021
@codecov
Copy link

codecov bot commented Jun 3, 2021

Codecov Report

Merging #5932 (6a7abbc) into master (fafdcd7) will decrease coverage by 0.02%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5932      +/-   ##
==========================================
- Coverage   70.67%   70.65%   -0.03%     
==========================================
  Files         454      454              
  Lines       17395    17399       +4     
==========================================
- Hits        12294    12293       -1     
- Misses       4195     4197       +2     
- Partials      906      909       +3     
Impacted Files Coverage Δ
...skaffold/kubernetes/debugging/container_manager.go 50.98% <33.33%> (+0.98%) ⬆️
pkg/skaffold/runner/v1/dev.go 73.11% <60.00%> (+0.12%) ⬆️
...affold/kubernetes/portforward/forwarder_manager.go 50.00% <100.00%> (+1.11%) ⬆️
pkg/skaffold/util/tar.go 50.66% <0.00%> (-5.34%) ⬇️
pkg/skaffold/docker/image.go 78.34% <0.00%> (-1.39%) ⬇️
pkg/skaffold/event/v2/event.go 74.03% <0.00%> (+0.12%) ⬆️
pkg/skaffold/docker/parse.go 87.14% <0.00%> (+0.95%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fafdcd7...6a7abbc. Read the comment docs.

@briandealwis
Copy link
Member

You need to drop a7ce90d — sorry, it was a test :-(

@tejal29 tejal29 force-pushed the pause_debug_pod_watchers branch from 4cb40a9 to dc24c78 Compare June 3, 2021 04:57
@tejal29
Copy link
Contributor Author

tejal29 commented Jun 3, 2021

You need to drop a7ce90d — sorry, it was a test :-(

No worries.

@tejal29 tejal29 enabled auto-merge (squash) June 3, 2021 04:59
@tejal29 tejal29 merged commit 55418f6 into GoogleContainerTools:master Jun 3, 2021
@briandealwis
Copy link
Member

I think one consequence of this change is that all debug sessions will be torn down and need to be re-established. I’m not sure CC-IJ does this at the moment — @etanshaul?

Maybe we need these managers to have a Pause() and Resume () that can be a bit more intelligent at resolving the impact of pod changes.

@etanshaul
Copy link
Contributor

I’m not sure CC-IJ does this at the moment — @etanshaul?

IntelliJ has no iterative debugging at the moment (unless the user goes out of their way to turn on watching via env vars - but this isn't yet an officially supported path).

@briandealwis
Copy link
Member

And CC-IJ doesn't reconnect on a dropped connection.

@tejal29 tejal29 deleted the pause_debug_pod_watchers branch July 21, 2021 17:39
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.

Node JS (Hello World) iterative debugging fails on the first attempt in iterative development
3 participants