feat(k8s): make hot reloading work for remote clusters

[edvald]

This amends the hot reloading flow for K8s containers to use port-forwarding instead of finding the hostname via ingress (which wasn't going to work in all cases anyway), which makes it work securely across the wire.

I also happened upon issues with the "hot reload scheduler" which wasn't handling promise errors correctly. I made a HotReloadTask instead, which serves the same purpose - which was to queue up and deduplicate rapid successions of sync attempts - but captures and handles errors properly (and does away with superfluous code along the way).

[edvald]

Oh and I should add - I started this because I was working on providing hot-reloading to user-defined Helm charts. The changes turned out to be necessary for that, but I split this part out of the PR because it's useful by itself.

[thsig]

This moving of the hot reload operation to HotReloadTask is a good idea.

The original thought was: Since a hot reload operation doesn't depend on any other tasks, and since it should be executed as soon as possible (regardless of whether capacity was available in TaskGraph), that it made sense to execute it outside the task system.

However, in hindsight, this approach (i.e. wrapping hot-reloading in a task) is simpler / "less special", and fits better into the general infrastructure.

[edvald]

@drubin Updated with changes related to your comments. Please take a look.

[drubin]

I tested this with the gatsby example. And it just stays stuck in hot reloading

garden-local dev --hot-reload=website -l silly

⠼ tasks                     → Processing task hot-reload.website.462f95f0-02e9-11e9-a702-f5de2e984789
⠹ website                   → Hot reloading...
    Waiting for services website
    Waiting for services website
    Waiting for services website
    Waiting for services website
    Waiting for services website
    Waiting for services website
    Waiting for services website
    Waiting for services website
    Waiting for services website
    Waiting for services website
    Waiting for services website

Can't really figure out what its doing because it just hangs, the same applies to the hot reload example

[edvald]

I made a delightfully dumb mistake there 🤦‍♂️ Updated commit, and tested again, works for me. Might be worth a double-check when you have a moment.

[drubin]

The performance is about 3x worse than the nodeport version currently on master which is already noticeable.

120ms is on the border of it being too slow 400ms is simply not ok, and this will be much worse for remote clusters.

Current master (I tested it once but its always been roughly 100s - 150ms, and I have tested this a lot)

✔ node-service → Hot reloading... → Done (took 145 ms)

This branch

✔ node-service              → Hot reloading... → Done (took 351 ms)
✔ node-service              → Hot reloading... → Done (took 420 ms)

I think the finding an open port and port-forwarding can't happen on every change we need to do them when we deploy the service.

@edvald I think if we move the port-forwarding to a single tunnel and stream the changes down that we could get this down to acceptable limits? What do you think?

[edvald]

Managing a single tunnel might be tricky, but worth exploring. I can take a look at that, but would prefer to defer it to another PR. This PR also addresses potential bugs when working locally, and I would be okay with an increased response time while we dig into it.

[edvald]

I just took a stab at persisting tunnels, and noticed it'll be much cleaner to implement once I'm done with my Helm branch, because it changes the hotReload plugin action to work at a service level, as opposed to per-module. We can either hold off on this, or make a separate PR if/when we find something that improves the performance measurably.

[edvald]

Re-visiting this. @drubin would you be okay with me splitting the re-used tunnels into a separate issue? Otherwise this needs to wait for another week or so, until my WIP Helm changes are merged.

[eysi09]

@drubin, regarding your requested changes, it appears @edvald already addressed them. I've also tested this on my end and everything seems to work fine, apart from the perf regression.

The perf regressions themselves are addressed in #458 which is also a part of the 0.9 milestone. With that in mind, should we merge this one?

Untimely.

Performance concerns to be addressed in separate issue #458, which will be done in the same release.