Add graceful termination for custom builders

[cedrickring]

Fixes #2656

I added a 2 seconds grace period for the process to complete after receiving SIGINT.

[codecov]

Codecov Report

Merging #2886 into master will decrease coverage by 0.13%.
The diff coverage is 59.25%.

Impacted Files	Coverage Δ
pkg/skaffold/build/custom/custom.go	63.93% <59.25%> (-1.86%)	⬇️
pkg/skaffold/deploy/status_check.go	59.13% <0%> (-4.92%)	⬇️
pkg/skaffold/initializer/init.go	57.45% <0%> (-1.69%)	⬇️
pkg/skaffold/deploy/deploy.go	100% <0%> (ø)	⬆️
pkg/skaffold/deploy/resource/deployment.go	100% <0%> (ø)	⬆️
pkg/skaffold/schema/versions.go	74.35% <0%> (ø)	⬆️
pkg/skaffold/schema/v1beta14/upgrade.go	77.77% <0%> (ø)
pkg/skaffold/schema/v1beta14/config.go	100% <0%> (ø)
pkg/skaffold/deploy/helm.go	74.2% <0%> (+0.2%)	⬆️
pkg/skaffold/deploy/kubectl.go	65.54% <0%> (+0.58%)	⬆️
... and 1 more

[priyawadhwa]

Hey @cedrickring thanks for fixing this! Would you mind writing a unit test for this feature?

[tejal29]

@cedrickring, i feel the better way to do this is by defining a different cleanUpCommand.

apiVersion: skaffold/v1beta13
kind: Config
build:
  artifacts:
  - image: ...
    custom:
      buildCommand: ./build.sh
      cleanUpCommand: ./clean-up.sh

And then, skaffold to execute the cleanUpCommand and not wait for a constant time to cleanup.
What do you think?

[cedrickring]

Hey @tejal29, thanks for the feedback.

The (up to) 30 seconds wait is not exactly for cleanup imo, but for the command to exit gracefully. Right now the process gets killed immediately after pressing Ctrl+C and doesn't have time to exit normally (temp files aren't cleaned up etc). Usually a process shouldn't take the whole 30 seconds to terminate.

I think a cleanup script could be an additional option for this, but not a replacement.

[tejal29]

@cedrickring, thanks for clarifying. I see, you are implementing what golang/go#22757 proposes.

I think, if we implement this solution, we should reduce the timeout for 2 seconds like
https://github.com/mvdan/sh/pull/170/files#diff-b071a7aa58a1e42f9c5278440e585c77R49

[tejal29]

i am little bit concerned with these tests adding sleeps. Can you just add a bash command like sleep 5 in the build script?

Also, run it multiple times (100) to make sure its reliable.

[cedrickring]

Imo 100 repeats are a bit much for a unit test. Also inlining a bash script required some extra changes in code.

[cedrickring]

Also the tests require the interrupt signal to be trapped. Otherwise they will always error.

[tejal29]

sorry, i meant running it 100 times via go run -n -test.run <>. I will do it myself before reviewing.

[tejal29]

This looks really great. Just one question, i am assuming you have the done channel to return immediately if the process is already done? Lets put a defer close as soon as you create the channel

We can also shorten this code block further

func (b *ArtifactBuilder) handleGracefulTermination(ctx context.Context, cmd *exec.Cmd) error {
  done := make(chan struct{})
  defer(close(done))
  go func() {
    select {
    case <- ctx.Done():
      // Send interrupt signal if not windows.
      return 
    case <- time.After(termination*time.Second):
     // Send kill signal
      return 
    case <-done
     return
   }
  }()
 err := cmd.Wait()
 return err
}


 }

I think this is it. And then we can merge this in!

Thank you for your patience.

[cedrickring]

According to your suggestion, the process will only run for termination seconds and will then be killed. i think the nested select is still necessary to wait for the context to be cancelled...

[tejal29]

Sounds good. Running kokoro tests now