Move embedded haproxy process to a distinct pid group

[jcmoraisjr]

The embedded haproxy process is being created in the same pid group of the controller, which makes it receive the same signals received by the controller when running under a process supervisor. This makes haproxy be prematurely terminated when the controller is still shutting down.

[jcmoraisjr]

#1028

[avmnu-sng]

@jcmoraisjr Don't we need to update haproxy-shutdown.sh to send USR1, it still sends TERM which is hard stop.

haproxy-ingress/rootfs/haproxy-shutdown.sh

Line 23 in 914b581

kill $pid

kill -USR1 $pid

Even for the master-worker mode.

haproxy-ingress/pkg/haproxy/instance.go

Line 661 in 914b581

if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {

[jcmoraisjr]

Hi, in fact this shouldn't make any difference because, when shutdown.sh is called, the controller process is being shutting down as well, dropping all the other process in that same container. So moving from term to usr1 would be a noop to the observer. I understand your concern, and I also think we've already discussed that on another issue, and the point here is that the correct place to configure a long living haproxy connection during rolling updates is not here, but instead making the controller to postpone its own shutdown. Moreover, this script belongs to a way to run haproxy which is being deprecated. master-worker is the new default from v0.15, and it calls haproxy directly without any help of external scripts.

[avmnu-sng]

@jcmoraisjr Yeah, we discussed it earlier. I guess I understand now why USR1 is still a no-op. For example, suppose we have wait-before-shutdown=10s. Then, after 10s, irrespective of USR1 or TERM on HAProxy, the ingress controller exits immediately. It would only work if we wait after triggering the HAProxy shutdown. Even if we move to master-worker, we still need a soft stop on HAProxy, and shouldn't ideally, the controller should exit after HAProxy shutdown completes.

[jcmoraisjr]

I see your point. When running haproxy as a daemon we don't have a good control of the underlying process. In master worker mode we run it in the foreground, this gives us a few more options.

Here is where the process is started, stopped, and asked to be stopped:

haproxy-ingress/pkg/haproxy/instance.go

Lines 632 to 667 in a7e4b1f

	func (i *instance) startHAProxySync() {
	cmd := exec.Command(
	"haproxy",
	"-W",
	"-S", i.options.MasterSocket+",mode,600",
	"-f", i.options.HAProxyCfgDir)
	cmd.Stdout = os.Stdout
	cmd.Stderr = os.Stderr
	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
	if err := cmd.Start(); err != nil {
	i.logger.Error("error starting haproxy: %v", err)
	return
	}
	wait := make(chan struct{})
	go func() {
	if err := cmd.Wait(); err != nil {
	if exitError, ok := err.(*exec.ExitError); ok {
	i.logger.Info("haproxy stopped (exit code: %d)", exitError.ExitCode())
	} else {
	i.logger.Error("error while running haproxy: %v", err)
	}
	} else {
	i.logger.Info("haproxy stopped")
	}
	close(wait)
	}()
	select {
	case <-i.options.StopCh:
	i.logger.Info("stopping haproxy master process (pid: %d)", cmd.Process.Pid)
	if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {
	i.logger.Error("error stopping haproxy process: %v", err)
	}
	<-wait
	case <-wait:
	}
	}

and here is the channel that makes the controller wait until haproxy is gone:

haproxy-ingress/pkg/haproxy/instance.go

Lines 402 to 423 in a7e4b1f

	func (i *instance) Shutdown() {
	if !i.up || i.options.IsExternal {
	// lifecycle isn't controlled by HAProxy Ingress
	return
	}
	if i.options.IsMasterWorker {
	<-i.waitProc
	return
	}
	i.logger.Info("shutting down embedded haproxy")
	out, err := exec.Command(
	i.options.RootFSPrefix+"/haproxy-shutdown.sh",
	i.options.LocalFSPrefix,
	).CombinedOutput()
	outstr := string(out)
	if outstr != "" {
	i.logger.Warn("output from the shutdown process: %v", outstr)
	}
	if err != nil {
	i.logger.Error("error shutting down haproxy: %v", err)
	}
	}

In this scenario it makes sense to send usr1 because the controller will wait for it. I'll give this a chance, maybe adding as a config option. Tks btw for bringing this scenario again and again =)

[avmnu-sng]

@jcmoraisjr No worries; finally, we are on the same page. For now, with Setpgid: true, running in master-worker mode with a sensible wait-before-shutdown value would take us closer to a soft-stop experience.

Please give the config option approach a shot; it would be helpful. Thanks!