Port forward pods automatically during skaffold dev
#945
Conversation
Skaffold dev will now automatically port-forward any pods that expose container ports. Ports are assigned on a first-come-first-serve basis, and that container will hold the lock on that port for the remainder of the dev cycle. Updates to a container will trigger an update which replaces the forwarded port with the newest version. Scaling up a deployment will continue to port-forward the latest pod. However, scaling down a deployment has the potential to kill the port-forwarded pod without replacement - since we do not keep track of candidates that are "waiting" on that port.
r2d4
force-pushed
the
port-forward-pods
branch
from
6c51ac9 to
2873a1c
Compare
Codecov Report
@@ Coverage Diff @@
## master #945 +/- ##
==========================================
- Coverage 43.28% 43.14% -0.15%
==========================================
Files 63 65 +2
Lines 2629 2726 +97
==========================================
+ Hits 1138 1176 +38
- Misses 1372 1429 +57
- Partials 119 121 +2
Continue to review full report at Codecov.
|
|
Fixes #94 |
| func (*kubectlForwader) Forward(pfe *portForwardEntry) error { | ||
| logrus.Debugf("Port forwarding %s", pfe) | ||
| portNumber := fmt.Sprintf("%d", pfe.port) | ||
| cmd := exec.Command("kubectl", "port-forward", fmt.Sprintf("pod/%s", pfe.podName), portNumber, portNumber) |
There was a problem hiding this comment.
Nit: this will probably fail on Windows, right? That's probably fine though.
There was a problem hiding this comment.
No, this should work on windows as well.
| pfe.cmd = cmd | ||
|
|
||
| buf := &bytes.Buffer{} | ||
| cmd.Stdout = buf |
There was a problem hiding this comment.
nit: would it make sense to separate these buffers to make the error easier to read later?
|
|
||
| // Key is an identifer for the lock on a port during the skaffold dev cycle. | ||
| func (p *portForwardEntry) key() string { | ||
| return fmt.Sprintf("%s-%d", p.containerName, p.port) |
There was a problem hiding this comment.
Should we also include namespace and podName here to make it fully keyed?
There was a problem hiding this comment.
We want to consider all pods created by a workloads object to be the same - so if a deployment creates three replicas of "pod": "pod-a", "pod-b", "pod-c", they should be considered the same key.
Namespace might be fine, but I can't think of a use case. The case would be where I deploy the same container to two different namespaces. The current code would port-forward the latest deployed container, while the proposed namespace key wouldn't.
|
|
||
| // PortForwarder is responsible for selecting pods satisfying a certain condition and port-forwarding the exposed | ||
| // container ports within those pods. It also tracks and manages the port-forward connections. | ||
| type PortForwarder struct { |
There was a problem hiding this comment.
nit: I would rename this to ExclusivePortForwarder (or similar) and rename the Forwarder interface to PortForwarder.
| containerName: c.Name, | ||
| port: port.ContainerPort, | ||
| } | ||
| v, ok := p.forwardedPods.Load(entry.key()) |
There was a problem hiding this comment.
nit: maybe ok -> isPodForwarded
| if !ok { | ||
| continue | ||
| } | ||
| if p.podSelector.Select(pod) && pod.Status.Phase == v1.PodRunning && pod.DeletionTimestamp == nil { |
There was a problem hiding this comment.
can we extract the event handling logic and have it covered by unit test? I feel like it's an important piece to protect especially if we want to merge it later with the logging podwatcher.
There was a problem hiding this comment.
|
tested it it on @ahmetb's microservices example, and it's running nicely 👍 Maybe we could add a doc to list the ports as well, it was a bit hard to find what service was mapped where before I found this - but this works only for unix: |
|
What does it do in case of port conflicts? We deliberately kept the port numbers different so we can develop locally easier –but we actually have plans to standardize port numbers. |
|
I just tested this out and it is failing when the pods expose a privileged port (ie 80). We need to be able to detect that and map to a higher port maybe by adding a constant (80->10080). |
|
In this PR the first one wins. See https://github.com/GoogleContainerTools/skaffold/pull/945/files#diff-f0665c32a151485f611d6517eb543409R175 for conflict "resolution" with known ports. |
|
I think if you print something on the screen in the format: then you can randomly choose from available ports, and I'll just paste this to my terminal where I'm launching my app from. |
|
Added #969 for follow-up on the issues for more complex cases. We should document the caveats and then merge. |
Skaffold dev will now automatically port-forward any pods that expose
container ports.
Ports are assigned on a first-come-first-serve basis,
and that container will hold the lock on that port for the remainder of the dev
cycle.
Updates to a container will trigger an update which replaces the
forwarded port with the newest version.
Scaling up a deployment will continue to port-forward the latest pod.
However, scaling down a deployment has the potential to kill the
port-forwarded pod without replacement - since we do not keep track of
candidates that are "waiting" on that port.