Transfer control of dev loop from file watcher to dev listener #2354

nkubala · 2019-06-26T19:07:21Z

This PR changes the way the dev loop control flow is handled in the codebase.

Previously, the file watcher was responsible for triggering the actions that skaffold dev would perform: when a file change was detected (whether through a polling, fsnotify, or manual mechanism), the file watcher itself was responsible for performing the builds, syncs, and deploys, and the actual "dev loop" served only as a wrapper around the file watcher.

In this new design, the file watcher (renamed to File Monitor and referred to as such from now on) now only serves as a component that the dev loop uses to compute filesystem changes when necessary. The dev loop now contains a Listener, which is responsible for listening to the same trigger that was previously triggering the old file watcher. When a trigger is received, it first asks the file monitor to compute the changes on the local filesystem, and then conditionally performs the actions in the dev loop based on the changes it finds. This trigger is also moved out to a component stored on the runner, rather than being embedded in the file watcher (or in the listener).

The changeSet struct is moved from a dev-scoped variable to a struct on the runner, and is closed over in the callbacks that are registered to the file monitor before the listener is started. This struct still serves the same purpose: the track the "actions" that the dev loop needs to perform on each cycle. These actions are semantically computed by the file monitor when it determines the changes to the local filesystem since the last cycle of the dev loop. (This is not a new concept to this PR, just a clarification for better understanding).

This is the first step toward introducing component-specific triggers in the dev loop (e.g. manual build, manual deploy, manual sync), as a way to give users more control over what parts of skaffold dev happen when.

Summary of changes:

Watcher -> Monitor (package watch moved to package filemon, and trigger-related code moved to package trigger)
decouple the "file monitoring" component (previously Watcher, now Monitor) from the "trigger" component that actually kicks off the dev cycle (Trigger)
move changeSet from "dev"-scoped variable to struct on Runner: this struct is closed over in the callbacks that are registered on the file watcher
Run() method on the file monitor is now called by the dev listener whenever a trigger is received
move onChange() callback from file watcher to dev listener: this is now called when a trigger is received by the listener, after it calls r.Monitor.Run(...)

balopat

Started looking at it. Some initial feedback for some nits and questions.
I have to digest the debounce logic again - but otherwise it actually looks good to me!

pkg/skaffold/filemon/monitor_test.go

balopat · 2019-06-26T22:40:21Z

pkg/skaffold/filemon/monitor_test.go

@@ -26,20 +26,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )

-func TestWatchWithPollTrigger(t *testing.T) {


TestWatchWithPollTrigger and TestWatchWithNotifyTrigger should find their places in the new design too somehow. Maybe under the new trigger package?

I added these as integration tests.

pkg/skaffold/runner/runner_test.go

pkg/skaffold/runner/dev.go

codecov · 2019-06-27T22:43:31Z

Codecov Report

❗ No coverage uploaded for pull request base (master@7fe4c6f). Click here to learn what that means.
The diff coverage is 65.03%.

Impacted Files	Coverage Δ
pkg/skaffold/filemon/changes.go	`79.62% <ø> (ø)`
pkg/skaffold/runner/diagnose.go	`13.04% <0%> (ø)`
pkg/skaffold/runner/listen.go	`0% <0%> (ø)`
pkg/skaffold/sync/sync.go	`82.71% <100%> (ø)`
pkg/skaffold/runner/changeset.go	`100% <100%> (ø)`
pkg/skaffold/runner/runner.go	`71.31% <100%> (ø)`
pkg/skaffold/runner/logger.go	`100% <100%> (ø)`
pkg/skaffold/trigger/triggers.go	`29.12% <21.42%> (ø)`
pkg/skaffold/runner/dev.go	`64.1% <79.24%> (ø)`
pkg/skaffold/filemon/monitor.go	`81.25% <81.25%> (ø)`

dgageot

This is an initial feedback. I like where it's going.

pkg/skaffold/runner/runner.go

pkg/skaffold/runner/dev.go

dgageot · 2019-06-28T15:33:26Z

pkg/skaffold/runner/dev.go

+	return r.changeSet.needsReload || len(r.changeSet.needsRebuild) > 0 || r.changeSet.needsRedeploy
+}
+
+func (r *SkaffoldRunner) processArtifacts() error {


I'm not sure extracting this piece of code makes it easier to read. It seems disconnected to the rest of the code now. If you still prefer to extract it, at least maybe change the name.

the main reason I did this is because it's also being used in the tests, and it felt better than having duplicated code. I'll leave it moved out for now, but I'll put a TODO to try and move it into the callback registered with the monitor. I changed the name to separateBuildAndSync(), suggestions welcome though (I don't like this one either)

dgageot · 2019-06-28T15:35:39Z

pkg/skaffold/runner/dev.go

@@ -150,5 +166,5 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
 		return errors.Wrap(err, "starting forwarder manager")
 	}

-	return r.Watcher.Run(ctx, out, onChange)
+	return r.Listen(ctx, out, onChange)


Could the function be called WaitForChanges or WaitForTrigger or something that better tels what it's doing?

yep good idea. I changed this to WatchForChanges and the previous method with that name to LogWatchToUser, WDYT?

dgageot · 2019-06-28T15:36:45Z

pkg/skaffold/runner/dev.go

@@ -42,43 +61,40 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
 	defer forwarderManager.Stop()

 	// Create watcher and register artifacts to build current state of files.
-	changed := changes{}


I liked when this state was local to this function. This way, it was clear that nobody else could change its state.

changeSet is a better name though.

I see your point, it's much easier to handle the tests when the changeSet is embedded on the runner though. plus, it's not really true that "nobody can change its state": the file monitor does exactly this, albeit through the callbacks that we register before we start the dev loop.

if you feel strongly about it, I can change it back.

pkg/skaffold/trigger/triggers.go

dgageot · 2019-06-28T15:42:07Z

pkg/skaffold/runner/runner_test.go

@@ -159,6 +213,38 @@ func createRunner(t *testutil.T, testBench *TestBench) *SkaffoldRunner {
 	runner.Syncer = testBench
 	runner.Tester = testBench
 	runner.Deployer = testBench
+	runner.Listener = testBench
+
+	testBench.devLoop = func() error {


It feels super strange that we have to reimplement that part of the runner. It doesn't feel like we're testing the actual code. (Maybe it wasn't the case before either)

yeah, this was the case before as well, TestBench is a big mock. it is very hard to see what is the actual part of production code that is tested.

I've just checked and it looks like TestBench is implementing Builder, Deployer, Tester... but didn't implement anything from the SkaffoldRunner. It's basically a list of mocks that record the interactions of a SkaffoldRunner with the other components.
That doesn't seem to be the case anymore.

yeah it's weird. I can't call runner.Dev() or runner.WatchForChanges() in the tests because those will run forever; so this was my way of "simulating" the dev loop by "mapping" out the general flow of it.

I'm not sure I totally follow what you're saying though....it sounds like you're proposing that we rewrite the TestBench to mock out the methods defined on the SkaffoldRunner interface? I don't totally disagree with you (I don't really like the way this test is structure to begin with) but it would be a pretty big change. what I did here was the most minimal way I found to retrofit the old tests to the new structure.

What I'm saying is that runner_test.go tries to test the code in runner.go, so, the SkaffoldRunner. If feels weird to reimplement the devloop part in tests because then, what are we testing?
The idea of this test was to mock the Builder, the Deployer, the Tester..., group then under the name TestBench and see how SkaffoldRunner interacts with them.

ok I see what you mean. I took a stab at moving the dev loop out to a method on the SkaffoldRunner, and then calling that from the tests, with a tiny wrapper that calls the file monitor. this is the same thing the actual Listener does whenever it receives a trigger, so it should be actually testing the correct thing now. have a look at the latest commit and tell me what you think

dgageot

LGTM with just one nit

pkg/skaffold/runner/dev_test.go

dgageot · 2019-07-02T11:46:09Z

I think it would make sense to merge just after the next release to be able to safely test the impact of those changes.

…e tests

nkubala requested review from balopat, dgageot, priyawadhwa and tejal29 as code owners June 26, 2019 19:07

googlebot added the cla: yes label Jun 26, 2019

balopat suggested changes Jun 26, 2019

View reviewed changes

nkubala force-pushed the dev-control-flow branch from 748ec32 to eb1733d Compare June 27, 2019 22:43

nkubala force-pushed the dev-control-flow branch from eb1733d to de22b28 Compare June 27, 2019 22:55

dgageot self-assigned this Jun 28, 2019

dgageot suggested changes Jun 28, 2019

View reviewed changes

nkubala force-pushed the dev-control-flow branch 5 times, most recently from 7c9d24c to bc75908 Compare July 1, 2019 16:58

dgageot added the needs-rebase label Jul 2, 2019

dgageot approved these changes Jul 2, 2019

View reviewed changes

pkg/skaffold/runner/dev_test.go Outdated Show resolved Hide resolved

nkubala added 4 commits July 2, 2019 10:23

store changeSet struct on runner

0718a4d

transfer control of dev loop from file watcher to dev listener

7042c36

lint and gofmt

c60bc88

revert to old tests and fix up to support new flow

b78a2ca

nkubala force-pushed the dev-control-flow branch from 913efef to 47f1ba3 Compare July 2, 2019 17:29

pull dev loop out to separate runner method so it can be called by th…

e28672d

…e tests

nkubala force-pushed the dev-control-flow branch from 47f1ba3 to e28672d Compare July 2, 2019 17:30

add integration tests for different triggers

5907ee7

balopat approved these changes Jul 3, 2019

View reviewed changes

nkubala merged commit 1d98ece into GoogleContainerTools:master Jul 3, 2019

dgageot mentioned this pull request Jul 5, 2019

Triggers are not tested anymore by unit tests #2411

Closed

nkubala deleted the dev-control-flow branch October 17, 2019 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transfer control of dev loop from file watcher to dev listener #2354

Transfer control of dev loop from file watcher to dev listener #2354

nkubala commented Jun 26, 2019

balopat left a comment

balopat Jun 26, 2019

nkubala Jul 2, 2019

codecov bot commented Jun 27, 2019 •

edited

Loading

dgageot left a comment

dgageot Jun 28, 2019

nkubala Jun 28, 2019

dgageot Jun 28, 2019

nkubala Jun 28, 2019

dgageot Jun 28, 2019

dgageot Jun 28, 2019

nkubala Jun 28, 2019

dgageot Jun 28, 2019

balopat Jun 28, 2019

dgageot Jun 28, 2019

nkubala Jun 28, 2019

dgageot Jun 28, 2019

nkubala Jul 1, 2019

dgageot left a comment

dgageot commented Jul 2, 2019

Transfer control of dev loop from file watcher to dev listener #2354

Transfer control of dev loop from file watcher to dev listener #2354

Conversation

nkubala commented Jun 26, 2019

balopat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 27, 2019 • edited Loading

Codecov Report

dgageot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgageot left a comment

Choose a reason for hiding this comment

dgageot commented Jul 2, 2019

codecov bot commented Jun 27, 2019 •

edited

Loading