diff --git a/.gitignore b/.gitignore index febee9017501ee82b49cc6e0dbee76250571a647..b0e481a1fdfa46cb7d0ff6fe78e7e52e30965a1f 100644 --- a/.gitignore +++ b/.gitignore @@ -3,8 +3,9 @@ /gitaly-hooks cmd/gitaly-ssh/gitaly-ssh /gitaly-ssh -cmd/gitaly-wrapper/gitaly-wrapper +cmd/wrapper/wrapper /gitaly-wrapper +/wrapper cmd/gitaly-remote/gitaly-remote /gitaly-remote **/testdata/gitaly-libexec/ diff --git a/Makefile b/Makefile index 89b8a5c8ae0afc437de7a60c7b5e36dacd78a76b..db1b544ff8824c9b183c1054d817aaa29f8b507b 100644 --- a/Makefile +++ b/Makefile @@ -23,6 +23,9 @@ all: build .PHONY: build build: prepare-build cd $(BUILD_DIR) && $(MAKE) install INSTALL_DEST_DIR=$(CURDIR) + # Remove this in 12.7 once wrapper is used in place of gitaly-wrapper + # https://gitlab.com/gitlab-org/gitaly/issues/2232 + cp "$(BUILD_DIR)/bin/wrapper" "$(BUILD_DIR)/bin/gitaly-wrapper" .PHONY: build-gitaly-remote build-gitaly-remote: prepare-build @@ -43,6 +46,7 @@ assemble: prepare-build .PHONY: binaries binaries: prepare-build cd $(BUILD_DIR) && $(MAKE) $@ + cd $(BULLD_DIR) && cp wrapper gitaly-wrapper .PHONY: prepare-tests prepare-tests: prepare-build diff --git a/changelogs/unreleased/jc-use-gitaly-wrapper-for-praefect.yml b/changelogs/unreleased/jc-use-gitaly-wrapper-for-praefect.yml new file mode 100644 index 0000000000000000000000000000000000000000..7cb6429d9150a0265eb6d0aa0dc2adac4497cc7e --- /dev/null +++ b/changelogs/unreleased/jc-use-gitaly-wrapper-for-praefect.yml @@ -0,0 +1,5 @@ +--- +title: Refactor EnvPidFile and EnvUpgradesEnabled into bootstrap package +merge_request: 1665 +author: +type: other diff --git a/cmd/gitaly/main.go b/cmd/gitaly/main.go index 8635084a722704f23da8e5ac8969c61c3c47c48c..1527c52ef66f76bf26033cc4c06c7a58702d0edb 100644 --- a/cmd/gitaly/main.go +++ b/cmd/gitaly/main.go @@ -53,8 +53,7 @@ func main() { config.ConfigureLogging() // gitaly-wrapper is supposed to set config.EnvUpgradesEnabled in order to enable graceful upgrades - _, isWrapped := os.LookupEnv(config.EnvUpgradesEnabled) - b, err := bootstrap.New(os.Getenv(config.EnvPidFile), isWrapped) + b, err := bootstrap.New(bootstrap.PidFile(), bootstrap.UpgradesEnabled()) if err != nil { log.WithError(err).Fatal("init bootstrap") } diff --git a/cmd/praefect/main.go b/cmd/praefect/main.go index b0b3063a1f5a7f30bac4253a530f806efcd9485f..dd24846de6d24905615a97435b9d8e13fec912c1 100644 --- a/cmd/praefect/main.go +++ b/cmd/praefect/main.go @@ -120,9 +120,7 @@ func run(cfgs []starter.Config, conf config.Config) error { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - _, isWrapped := os.LookupEnv(config.EnvUpgradesEnabled) - - b, err := bootstrap.New(os.Getenv(config.EnvPidFile), isWrapped) + b, err := bootstrap.New(bootstrap.PidFile(), bootstrap.UpgradesEnabled()) if err != nil { return fmt.Errorf("unable to create a bootstrap: %v", err) } diff --git a/cmd/gitaly-wrapper/main.go b/cmd/wrapper/main.go similarity index 62% rename from cmd/gitaly-wrapper/main.go rename to cmd/wrapper/main.go index e1c1e367fc5baddf27f4cc3a00fa6be286dd7275..2120e7a589b5123cee39610f95136d1369e115f8 100644 --- a/cmd/gitaly-wrapper/main.go +++ b/cmd/wrapper/main.go @@ -12,7 +12,7 @@ import ( "time" "github.com/sirupsen/logrus" - "gitlab.com/gitlab-org/gitaly/internal/config" + "gitlab.com/gitlab-org/gitaly/internal/bootstrap" "gitlab.com/gitlab-org/gitaly/internal/ps" ) @@ -29,69 +29,69 @@ func main() { logrus.Fatalf("usage: %s forking_binary [args]", os.Args[0]) } - gitalyBin, gitalyArgs := os.Args[1], os.Args[2:] + bin, args := os.Args[1], os.Args[2:] - log := logrus.WithField("wrapper", os.Getpid()) + log := logrus.WithField("wrapper", os.Getpid()).WithField("binary", bin) log.Info("Wrapper started") - if pidFile() == "" { - log.Fatalf("missing pid file ENV variable %q", config.EnvPidFile) + if bootstrap.PidFile() == "" { + log.Fatalf("missing pid file ENV variable %q", bootstrap.PidFileEnvVar) } - log.WithField("pid_file", pidFile()).Info("finding gitaly") - gitaly, err := findGitaly() + log.WithField("pid_file", bootstrap.PidFile()).Info("finding process") + proc, err := findProcess() if err != nil { - log.WithError(err).Fatal("find gitaly") + log.WithError(err).Fatal("find process") } - if gitaly != nil && isGitaly(gitaly, gitalyBin) { + if proc != nil && matches(proc, bin) { log.Info("adopting a process") } else { log.Info("spawning a process") - proc, err := spawnGitaly(gitalyBin, gitalyArgs) + newProc, err := spawn(bin, args) if err != nil { - log.WithError(err).Fatal("spawn gitaly") + log.WithError(err).Fatal("spawn process") } - gitaly = proc + proc = newProc } - log = log.WithField("gitaly", gitaly.Pid) - log.Info("monitoring gitaly") + log = log.WithField("process", proc.Pid) + log.Info("monitoring process") - forwardSignals(gitaly, log) + forwardSignals(proc, log) // wait - for isAlive(gitaly) { + for isAlive(proc) { time.Sleep(1 * time.Second) } - log.Error("wrapper for gitaly shutting down") + log.Error("wrapper for process shutting down") } -func findGitaly() (*os.Process, error) { +func findProcess() (*os.Process, error) { pid, err := getPid() if err != nil && !os.IsNotExist(err) { return nil, err } // os.FindProcess on unix do not return an error if the process does not exist - gitaly, err := os.FindProcess(pid) + process, err := os.FindProcess(pid) if err != nil { return nil, err } - if isAlive(gitaly) { - return gitaly, nil + if isAlive(process) { + return process, nil } return nil, nil } -func spawnGitaly(bin string, args []string) (*os.Process, error) { +func spawn(bin string, args []string) (*os.Process, error) { cmd := exec.Command(bin, args...) - cmd.Env = append(os.Environ(), fmt.Sprintf("%s=true", config.EnvUpgradesEnabled)) + cmd.Env = append(os.Environ(), fmt.Sprintf("%s=true", bootstrap.UpgradesEnabledEnvVar)) cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout @@ -107,13 +107,13 @@ func spawnGitaly(bin string, args []string) (*os.Process, error) { return cmd.Process, nil } -func forwardSignals(gitaly *os.Process, log *logrus.Entry) { +func forwardSignals(proc *os.Process, log *logrus.Entry) { sigs := make(chan os.Signal, 1) go func() { for sig := range sigs { log.WithField("signal", sig).Warning("forwarding signal") - if err := gitaly.Signal(sig); err != nil { + if err := proc.Signal(sig); err != nil { log.WithField("signal", sig).WithError(err).Error("can't forward the signal") } } @@ -123,7 +123,7 @@ func forwardSignals(gitaly *os.Process, log *logrus.Entry) { } func getPid() (int, error) { - data, err := ioutil.ReadFile(pidFile()) + data, err := ioutil.ReadFile(bootstrap.PidFile()) if err != nil { return 0, err } @@ -139,23 +139,19 @@ func isAlive(p *os.Process) bool { return p.Signal(syscall.Signal(0)) == nil } -func isGitaly(p *os.Process, gitalyBin string) bool { +func matches(p *os.Process, bin string) bool { command, err := ps.Comm(p.Pid) if err != nil { return false } - if path.Base(command) == path.Base(gitalyBin) { + if path.Base(command) == path.Base(bin) { return true } return false } -func pidFile() string { - return os.Getenv(config.EnvPidFile) -} - func jsonLogging() bool { return os.Getenv(envJSONLogging) == "true" } diff --git a/cmd/gitaly-wrapper/main_test.go b/cmd/wrapper/main_test.go similarity index 71% rename from cmd/gitaly-wrapper/main_test.go rename to cmd/wrapper/main_test.go index f4d98a37480158d17665c698314adf004876152b..e1cc76aeff770a1d59a8abd9cf009eb10727823c 100644 --- a/cmd/gitaly-wrapper/main_test.go +++ b/cmd/wrapper/main_test.go @@ -8,20 +8,20 @@ import ( "testing" "github.com/stretchr/testify/require" - "gitlab.com/gitlab-org/gitaly/internal/config" + "gitlab.com/gitlab-org/gitaly/internal/bootstrap" ) // TestStolenPid tests for regressions in https://gitlab.com/gitlab-org/gitaly/issues/1661 func TestStolenPid(t *testing.T) { defer func(oldValue string) { - os.Setenv(config.EnvPidFile, oldValue) - }(os.Getenv(config.EnvPidFile)) + os.Setenv(bootstrap.PidFileEnvVar, oldValue) + }(os.Getenv(bootstrap.PidFileEnvVar)) pidFile, err := ioutil.TempFile("", "pidfile") require.NoError(t, err) defer os.Remove(pidFile.Name()) - os.Setenv(config.EnvPidFile, pidFile.Name()) + os.Setenv(bootstrap.PidFileEnvVar, pidFile.Name()) cmd := exec.Command("tail", "-f") require.NoError(t, cmd.Start()) @@ -31,16 +31,16 @@ func TestStolenPid(t *testing.T) { require.NoError(t, err) require.NoError(t, pidFile.Close()) - tail, err := findGitaly() + tail, err := findProcess() require.NoError(t, err) require.NotNil(t, tail) require.Equal(t, cmd.Process.Pid, tail.Pid) t.Run("stolen", func(t *testing.T) { - require.False(t, isGitaly(tail, "/path/to/gitaly")) + require.False(t, matches(tail, "/path/to/gitaly")) }) t.Run("not stolen", func(t *testing.T) { - require.True(t, isGitaly(tail, "/path/to/tail")) + require.True(t, matches(tail, "/path/to/tail")) }) } diff --git a/internal/bootstrap/bootstrap.go b/internal/bootstrap/bootstrap.go index 41c1c7d891954135717f6501861a4a8d2ff767a9..8f9227beed5cc334650bf8742e8d0eefc2095e6a 100644 --- a/internal/bootstrap/bootstrap.go +++ b/internal/bootstrap/bootstrap.go @@ -13,6 +13,41 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/config" ) +const ( + // PidFileEnvVar is the environment variable name for setting the pid file path + PidFileEnvVar = "PID_FILE" + + // UpgradesEnabledEnvVar is the environment variable that controls whether or not upgrades are enabled + UpgradesEnabledEnvVar = "UPGRADES_ENABLED" +) + +// PidFile retrieves the pid file path determined by the environment variable PidFileEnvVar +func PidFile() string { + pidFile := os.Getenv(PidFileEnvVar) + // TODO: Remove this fallback logic in 12.7 once we have renamed GITALY_PID_FILE to PID_FILE in + // production environments + // https://gitlab.com/gitlab-org/gitaly/issues/2223 + if pidFile == "" { + pidFile = os.Getenv("GITALY_PID_FILE") + } + + return pidFile +} + +// UpgradesEnabled retrieves whether or not upgrades are enabled as determined by the environment +// variable UpgradesEnabledEnvVar +func UpgradesEnabled() bool { + _, isWrapped := os.LookupEnv(UpgradesEnabledEnvVar) + // TODO: Remove this fallback logic in 12.7 once we have renamed GITALY_UPGRADES_ENABLED to + // UPGRADES_ENABLED in production environments + // https://gitlab.com/gitlab-org/gitaly/issues/2223 + if !isWrapped { + _, isWrapped = os.LookupEnv("GITALY_UPGRADES_ENABLED") + } + + return isWrapped +} + // Bootstrap handles graceful upgrades type Bootstrap struct { // StopAction will be invoked during a graceful stop. It must wait until the shutdown is completed diff --git a/internal/config/config.go b/internal/config/config.go index a814e315cf0a74c79b9118ee0e3c01e5479cf8a6..945e5b048001acb5592f5791ddf2cd146c28238b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -21,13 +21,6 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/helper/text" ) -const ( - // EnvPidFile is the name of the environment variable containing the pid file path - EnvPidFile = "GITALY_PID_FILE" - // EnvUpgradesEnabled is an environment variable that when defined gitaly must enable graceful upgrades on SIGHUP - EnvUpgradesEnabled = "GITALY_UPGRADES_ENABLED" -) - var ( // Config stores the global configuration Config Cfg diff --git a/internal/praefect/config/config.go b/internal/praefect/config/config.go index cdd4b3e8156d3864827b9859d7b2d6add4778f72..b074c1f015a8bd8ff58c0a73bf100ee52a49c6f5 100644 --- a/internal/praefect/config/config.go +++ b/internal/praefect/config/config.go @@ -13,13 +13,6 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/praefect/models" ) -const ( - // EnvPidFile is the name of the environment variable containing the pid file path - EnvPidFile = "PRAEFECT_PID_FILE" - // EnvUpgradesEnabled is an environment variable that when defined gitaly must enable graceful upgrades on SIGHUP - EnvUpgradesEnabled = "PRAEFECT_UPGRADES_ENABLED" -) - // Config is a container for everything found in the TOML config file type Config struct { ListenAddr string `toml:"listen_addr"`