Reintroduce the fsNotify trigger #1562

dgageot · 2019-01-30T13:41:19Z

To make sure we don't require CGO, I've added the kqueue tag at build time.
We should test if it behaves as well as the cgo version on OSX

Fixes #1513

cc @ltouati

codecov-io · 2019-01-30T13:50:58Z

Codecov Report

Merging #1562 into master will decrease coverage by 0.07%.
The diff coverage is 30.76%.

@@            Coverage Diff            @@
##           master   #1562      +/-   ##
=========================================
- Coverage   46.18%   46.1%   -0.08%     
=========================================
  Files         118     118              
  Lines        4961    4986      +25     
=========================================
+ Hits         2291    2299       +8     
- Misses       2439    2456      +17     
  Partials      231     231

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/dev.go	`0% <0%> (ø)`	⬆️
pkg/skaffold/watch/triggers.go	`52.11% <32%> (-10.94%)`	⬇️

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 b62bfbb...c58ccb9. Read the comment docs.

briandealwis · 2019-01-30T15:53:41Z

cmd/skaffold/app/cmd/dev.go

@@ -38,7 +38,7 @@ func NewCmdDev(out io.Writer) *cobra.Command {
 	}
 	AddRunDevFlags(cmd)
 	cmd.Flags().BoolVar(&opts.TailDev, "tail", true, "Stream logs from deployed objects")
-	cmd.Flags().StringVar(&opts.Trigger, "trigger", "polling", "How are changes detected? (polling or manual)")
+	cmd.Flags().StringVar(&opts.Trigger, "trigger", "polling", "How are changes detected? (polling, manual or notify)")


"notify" is rather specific to Go and Linux. I can't think of a better term, but perhaps we can add some more text to describe it? It's also odd that we use "trigger" here but the related options are "watch".

How are changes detected? (polling, manual or notify) polling periodically looks for new files (see --watch-poll-interval), whereas notify leverages OS filesystem notifications (more efficient). manual waits for keyboard input to trigger a new build.

Some documentation would be good, yes.

The idea is that Skaffold knows all the dependencies for an artifact and can compute a "snapshot" of those dependencies (mostly last mod time). Now, to actually recompute a snapshot, do a diff and maybe trigger a rebuild/redeploy, it needs a trigger.
That trigger can be either:

a timer, that runs by default every 500ms. but then Skaffold has to compute the list of deps and the mod times every 500ms

a manual trigger. The user presses a key when they want Skaffold to compute the diff

a inotify based file watcher that watches the workspace as a whole and tells Skaffold.

iNotify based watchers are not good at watching folders and files that might be disapear. but it's quite good at recursively watching a top level folder that's always there.

By combining the snapshot mechanism and a inotify trigger, we get the best of both world.

IDE integration would work the same: The IDE would detect changes, notify Skaffold that "something" changed, no details. Then Skaffold would run a diff

I all cases, Skaffold is capable of debouncing rapid streams of triggers so that it behaves well when a lot of files are changed in a short period of time.

I'd like to see this documented so users know what the expected behavior is for each option

This reverts commit 9cf3292.

briandealwis · 2019-01-31T16:11:29Z

~~I've tried this on examples/jib and this doesn't seem to be working: modifying src/main/java/hello/HelloController.java doesn't trigger a rebuild.~~

~~What is odd is that touch skaffold.yaml triggers a rebuild and redeploy, and then skaffold consumes 100% CPU. But it's not calling out to Jib.~~

luser error: I used the wrong command-line flag 🤦‍♂️

nkubala

a few nits but otherwise LGTM

nkubala · 2019-01-31T18:52:15Z

pkg/skaffold/watch/triggers.go

+
+// Start Listening for file system changes
+func (t *fsNotifyTrigger) Start(ctx context.Context) (<-chan bool, error) {
+	// TODO(@dgageot): If file changes happen too quickly, events might be lost


we could compute the number of files in the watch directory at start up time and use that for the buffer size. just a thought

nkubala · 2019-01-31T18:54:24Z

cmd/skaffold/app/cmd/dev.go

@@ -38,7 +38,7 @@ func NewCmdDev(out io.Writer) *cobra.Command {
 	}
 	AddRunDevFlags(cmd)
 	cmd.Flags().BoolVar(&opts.TailDev, "tail", true, "Stream logs from deployed objects")
-	cmd.Flags().StringVar(&opts.Trigger, "trigger", "polling", "How are changes detected? (polling or manual)")
+	cmd.Flags().StringVar(&opts.Trigger, "trigger", "polling", "How are changes detected? (polling, manual or notify)")


I'd like to see this documented so users know what the expected behavior is for each option

ltouati · 2019-02-02T19:10:06Z

Hi @dgageot ,

Unfortunately this does not work for very large projects. Here's the error message I get when working on my project ( multiple maven modules + angular frontend)

Deploy complete in 5.965024794s
Cleaning up...
WARN[0163] cleanup: reading manifests: kubectl create: pipe: too many open files
FATA[0163] unable to start trigger: open /Users/ltouati/Documents/perso/cloud-diagram/frontend/node_modules/rxjs-compat/_esm5/add/operator: too many open files

briandealwis · 2019-02-02T23:37:50Z

Could you try doing some digging with lsof and see what process has lots of open files?

ltouati · 2019-02-09T11:05:26Z

@briandealwis Given that skaffold dies printing this message I don't see how I can do a lsof on a dead command.
This change behaves the same way as if we were listening files by files which is unworkable on very large repos

briandealwis · 2019-02-11T15:46:35Z

Sorry @ltouati! I missed the Cleaning up… part.

ltouati · 2019-02-11T16:13:13Z

@briandealwis no worries :)

dgageot requested review from balopat, nkubala, priyawadhwa and r2d4 as code owners January 30, 2019 13:41

googlebot added the cla: yes label Jan 30, 2019

briandealwis reviewed Jan 30, 2019

View reviewed changes

dgageot force-pushed the fsnotifier branch from 8d95290 to 5df56c9 Compare January 31, 2019 05:54

dgageot added 2 commits January 31, 2019 07:58

fsNotify

65fa2c1

Revert "Remove kqueue tag"

c58ccb9

This reverts commit 9cf3292.

dgageot force-pushed the fsnotifier branch from 5df56c9 to c58ccb9 Compare January 31, 2019 06:58

nkubala approved these changes Jan 31, 2019

View reviewed changes

nkubala merged commit f0f28e3 into GoogleContainerTools:master Jan 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintroduce the fsNotify trigger #1562

Reintroduce the fsNotify trigger #1562

dgageot commented Jan 30, 2019

codecov-io commented Jan 30, 2019 •

edited

Loading

briandealwis Jan 30, 2019

dgageot Jan 30, 2019

nkubala Jan 31, 2019

briandealwis commented Jan 31, 2019 •

edited

Loading

nkubala left a comment

nkubala Jan 31, 2019

nkubala Jan 31, 2019

ltouati commented Feb 2, 2019

briandealwis commented Feb 2, 2019

ltouati commented Feb 9, 2019

briandealwis commented Feb 11, 2019

ltouati commented Feb 11, 2019

Reintroduce the fsNotify trigger #1562

Reintroduce the fsNotify trigger #1562

Conversation

dgageot commented Jan 30, 2019

codecov-io commented Jan 30, 2019 • edited Loading

Codecov Report

briandealwis Jan 30, 2019

Choose a reason for hiding this comment

dgageot Jan 30, 2019

Choose a reason for hiding this comment

nkubala Jan 31, 2019

Choose a reason for hiding this comment

briandealwis commented Jan 31, 2019 • edited Loading

nkubala left a comment

Choose a reason for hiding this comment

nkubala Jan 31, 2019

Choose a reason for hiding this comment

nkubala Jan 31, 2019

Choose a reason for hiding this comment

ltouati commented Feb 2, 2019

briandealwis commented Feb 2, 2019

ltouati commented Feb 9, 2019

briandealwis commented Feb 11, 2019

ltouati commented Feb 11, 2019

codecov-io commented Jan 30, 2019 •

edited

Loading

briandealwis commented Jan 31, 2019 •

edited

Loading