Make notify based trigger the default

[dgageot]

We should try to make the notify trigger the default and if it fails fallback on polling

For eg, places where it could fail:

• https://github.com/GoogleContainerTools/skaffold/pull/1562/files#diff-c1405ced371c63a910bff1e1d46de15bR155
• Maybe https://github.com/GoogleContainerTools/skaffold/pull/1562/files#diff-c1405ced371c63a910bff1e1d46de15bR165

That would provide the best performance but with a bit of a safety net.

[dgageot]

Before we do make that trigger the default, we need to make sure it works well with a LOT of files.

• On windows, it seems ok as it is. It uses readdcw and doesn't need CGO.
• On linux, it uses inotify, doesn't need CGO. We just need to test how it behaves.
• On OSX, it can either use kqueue or fsevents. The last one requires CGO and give better results.

cc @balopat

[kwngo]

What's the progress looking like on this? Anything we can do to help?

[balopat]

@dgageot was working on an xgo based cross compilation in #2006 - if that works out we can have fsevents on Mac, but no one else is working on this actively currently. It is definitely high priority for us this quarter.

A reproducible load testing suite would be nice to create for this, one that we could just kick off manually (maybe at one point automatically) - it would create a LOT (trademark @dgageot) of files, run the watcher on it with just recording the event history, and would run a separate go routine changing files randomly
and compare the two event histories and print out a simple report (maybe as skaffold diagnose?).

If we could run this on different file counts and file change frequencies, we could start to use it to

1. get confidence in setting the notify based trigger by default
2. have a better understanding of the limits and CPU/memory characteristics of the watcher
3. maybe tune the watcher e.g. for debounce logic, etc. to cater for the most typical use cases.

[dgageot]

So here's a recap of the situation:

• all binaries have been cross compiled with CGO=1, using xgo, for a few releases.
• skaffold dev tests have been running for days with both the notify trigger and the polling trigger.
• I've just added an integration test for the sync feature to make sure it runs with both triggers: Test Sync mode with both triggers #2449.
• I've been defaulting to the notify trigger for months now.
• The notify trigger gracefully downgrades to the polling trigger if it fails to be started.

I believe we should be able to switch to using the notify trigger as the default for the next release.
@balopat wdyt?

[nkubala]

I've also been using it intermittently just to see if it behaves differently and I don't notice any difference. I'm a +1 on switching it for the next release.

[balopat]

I'm down, let's do it!

[yolkov]

'notify' not working for Docker Desktop for Mac

[dgageot]

Hi @yolkov! Do you have more details to share? I use Docker Desktop for Mac and it works well for me.

[yolkov]

@dgageot

Docker Desktop
version 2.0.4.1 (34207)
Channel edge

kubectl version
Client Version: version.Info{Major:"1", Minor:"14", GitVersion:"v1.14.3", GitCommit:"5e53fd6bc17c0dec8434817e69b04a25d8ae0ff0", GitTreeState:"clean", BuildDate:"2019-06-06T01:44:30Z", GoVersion:"go1.12.5", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"14", GitVersion:"v1.14.1", GitCommit:"b7394102d6ef778017f2ca4046abbaa23b88c290", GitTreeState:"clean", BuildDate:"2019-04-08T17:02:58Z", GoVersion:"go1.12.1", Compiler:"gc", Platform:"linux/amd64"}

helm version
Client: &version.Version{SemVer:"v2.14.1", GitCommit:"5270352a09c7e8b6e8c9593002a73535276507c0", GitTreeState:"clean"}
Server: &version.Version{SemVer:"v2.14.1", GitCommit:"5270352a09c7e8b6e8c9593002a73535276507c0", GitTreeState:"clean"}

skaffold version
v0.34.0

apiVersion: skaffold/v1beta13
kind: Config
build:
  artifacts:
  - image: prefix/example-service
    context: ../.
    sync:
      manual:
      - src: config/**
        dest: /app
      - src: module/**
        dest: /app
      - src: public/**
        dest: /app
      - src: vendor/**
        dest: /app
    docker:
      dockerfile: ./Dockerfile
  local:
    push: false
deploy:
  helm:
    releases:
    - name: example-service
      chartPath: chartmuseum/php-nginx
      valuesFiles:
      - ./dev/values.yaml
      - ./dev/values.service.yaml
      values:
        #image: prefix/example-service
        image.fullname: prefix/example-service
      namespace: example-service
      version: 0.8.0
      setValues:
        image.pullPolicy: IfNotPresent
      wait: true
      remote: true
      #imageStrategy:
      #  helm: {}

skaffold dev -v debug
...
Watching for changes...
DEBU[0077] Checking base image prefix/php-fpm:7.3 for ONBUILD triggers.
DEBU[0077] Found dependencies for dockerfile: [{. /app false}]
DEBU[0077] Skipping excluded path: secrets
DEBU[0077] Skipping excluded path: .gitignore
DEBU[0077] Skipping excluded path: .git

And nothing then I modify files

[yolkov]

'notify' work only for rebuild docker image and ignore for sync: manual:

[mecampbellsoup]

I seem to also be having some inotify event misses...

[tejal29]

@mecampbellsoup Please open a new issue with your skaffold.yaml and skaffold logs.