Wire up debug events

[briandealwis]

Relates to #3122 (#3564, #3609)
Fixes #2211

Description

This PR wires up the new DebuggingContainerEvent (#3609) to be fired by Skaffold when observing a debuggable container starting or terminating. These debugging containers are exposed via the Skaffold state object. With this PR, the IDEs can monitor for events to populate UIs.

User facing changes

New events and addition to the Skaffold state.

There are no changes to the log. Although some logging may be useful for users seeking to configure their debuggers manually, I figured the port-forwarding messages would usually be sufficient.

`/v1/events` stream of `skaffold debug` within `examples/jib`

In this example, we do a skaffold debug, and then kill the deployed pod. The deployment starts a new pod. We get a terminated event for the container for the killed pod.

{
  "result": {
    "timestamp": "2020-02-05T03:27:30.114354Z",
    "event": {
      "debuggingContainerEvent": {
        "status": "Started",
        "podName": "web-f6d56bcc5-6csgs",
        "containerName": "web",
        "namespace": "default",
        "artifact": "skaffold-jib",
        "runtime": "jvm",
        "debugPorts": {
          "jdwp": 5005
        }
      }
    },
    "entry": "Debuggable container started pod/web-f6d56bcc5-6csgs:web (default)"
  }
}

The `/v1/state` listing debugging containers

{
  "buildState": {
    "artifacts": {
      "skaffold-jib": "Complete"
    }
  },
  "deployState": {
    "status": "Complete"
  },
  "forwardedPorts": {
    "5005": {
      "localPort": 5005,
      "remotePort": 5005,
      "podName": "web-f6d56bcc5-6csgs",
      "containerName": "web",
      "namespace": "default",
      "portName": "jdwp",
      "resourceType": "pod",
      "resourceName": "web-f6d56bcc5-6csgs",
      "address": "127.0.0.1"
    },
    "8080": {
      "localPort": 8080,
      "remotePort": 8080,
      "namespace": "default",
      "resourceType": "service",
      "resourceName": "web",
      "address": "127.0.0.1"
    },
    "8081": {
      "localPort": 8081,
      "remotePort": 8080,
      "podName": "web-f6d56bcc5-6csgs",
      "containerName": "web",
      "namespace": "default",
      "resourceType": "pod",
      "resourceName": "web-f6d56bcc5-6csgs",
      "address": "127.0.0.1"
    }
  },
  "statusCheckState": {
    "status": "Not Started"
  },
  "fileSyncState": {
    "status": "Not Started"
  },
  "debuggingContainers": [
    {
      "status": "Started",
      "podName": "web-f6d56bcc5-6csgs",
      "containerName": "web",
      "namespace": "default",
      "artifact": "skaffold-jib",
      "runtime": "jvm",
      "debugPorts": {
        "jdwp": 5005
      }
    }
  ]
}

Next PRs.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes unit tests
• Mentions any output changes.
• Adds documentation as needed: user docs, YAML reference, CLI reference. already done
• Adds integration tests if needed.

Reviewer Notes

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

Release Notes

- New events emitted when debuggable containers are started and terminated

[codecov]

Codecov Report

Merging #3645 into master will increase coverage by 0.08%.
The diff coverage is 67.39%.

Impacted Files	Coverage Δ
...skaffold/kubernetes/debugging/container_manager.go	48.33% <ø> (+1.55%)	⬆️
cmd/skaffold/app/cmd/flags.go	100% <ø> (ø)	⬆️
pkg/skaffold/deploy/helm.go	77.18% <67.39%> (+2.52%)	⬆️
pkg/skaffold/sync/sync.go	72.57% <0%> (+0.31%)	⬆️

[briandealwis]

I introduced this function to test whether Skaffold was launched as skaffold debug. This works for debug, but it doesn't seem a great pattern — for example, debug effectively invokes dev under the hood.

[balopat]

nit: we already have a runContext.DevMode that serves a similar purpose. Can we make this similar? I don't have a preference towards either, I just don't want inconsistency.

[briandealwis]

What I observed when prototyping this functionality that I couldn't rely on the pod status. Rather I had to check on pod MODIFIED and DELETED events and then look at and track the individual container status (see below in checkPod()). I would get spurious debug events otherwise.

[balopat]

LGTM, tried it and works

[balopat]

Hmm...the new test just flaked:

time="2020-02-27T20:10:21Z" level=info msg="Ran in 9.814053538s"
--- FAIL: TestDebugEventsRPC (13.69s)
    rpc_test.go:376: waiting for connection...
    rpc_test.go:376: waiting for connection...
    debug_test.go:126: int differ (-got, +want):   int(
        - 	0,
        + 	1,
          )

I'm worried about the flake of TestDebugEventsRPC

[balopat]

we need to figure out what broke with --status-check=true that just got merged to be the default.

[briandealwis]

So the cause is that I deviate from port-forwarding's watch event filtering, which looks at ADDED and MODIFIED events, whereas the container manager only looks at MODIFIED and DELETED watch events. Prior to the status checks, ADDED events seemed spurious as they had no ContainerStatus items — the real changes came through MODIFIED events.

Removing this watch event filtering and using only the container status makes everything work again.

[briandealwis]

PTAL @balopat