From 06cfe2edd646a4d206e82439c9e0d63d4750aca6 Mon Sep 17 00:00:00 2001 From: jramsay Date: Fri, 29 Nov 2019 08:34:24 -0800 Subject: [PATCH 1/2] Use gitaly-wrapper for praefect --- .../jc-use-gitaly-wrapper-for-praefect.yml | 5 +++ cmd/gitaly-wrapper/main.go | 16 ++++----- cmd/gitaly-wrapper/main_test.go | 8 ++--- cmd/gitaly/main.go | 3 +- cmd/praefect/main.go | 4 +-- internal/bootstrap/bootstrap.go | 35 +++++++++++++++++++ internal/config/config.go | 7 ---- internal/praefect/config/config.go | 7 ---- 8 files changed, 52 insertions(+), 33 deletions(-) create mode 100644 changelogs/unreleased/jc-use-gitaly-wrapper-for-praefect.yml 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 00000000000..7cb6429d915 --- /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-wrapper/main.go b/cmd/gitaly-wrapper/main.go index e1c1e367fc5..0d06ff9784e 100644 --- a/cmd/gitaly-wrapper/main.go +++ b/cmd/gitaly-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" ) @@ -34,11 +34,11 @@ func main() { log := logrus.WithField("wrapper", os.Getpid()) 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") + log.WithField("pid_file", bootstrap.PidFile()).Info("finding gitaly") gitaly, err := findGitaly() if err != nil { log.WithError(err).Fatal("find gitaly") @@ -91,7 +91,7 @@ func findGitaly() (*os.Process, error) { func spawnGitaly(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 @@ -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 } @@ -152,10 +152,6 @@ func isGitaly(p *os.Process, gitalyBin string) bool { 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/gitaly-wrapper/main_test.go index f4d98a37480..09423f2b076 100644 --- a/cmd/gitaly-wrapper/main_test.go +++ b/cmd/gitaly-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()) diff --git a/cmd/gitaly/main.go b/cmd/gitaly/main.go index 8635084a722..1527c52ef66 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 b0b3063a1f5..dd24846de6d 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/internal/bootstrap/bootstrap.go b/internal/bootstrap/bootstrap.go index 41c1c7d8919..8f9227beed5 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 a814e315cf0..945e5b04800 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 cdd4b3e8156..b074c1f015a 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"` -- GitLab From 299e45974c0bdf8902b3d2dce0131b86a12316aa Mon Sep 17 00:00:00 2001 From: jramsay Date: Sat, 30 Nov 2019 09:00:16 -0800 Subject: [PATCH 2/2] Rename variables in gitaly-wrapper --- .gitignore | 3 +- Makefile | 4 ++ cmd/{gitaly-wrapper => wrapper}/main.go | 46 ++++++++++---------- cmd/{gitaly-wrapper => wrapper}/main_test.go | 6 +-- 4 files changed, 32 insertions(+), 27 deletions(-) rename cmd/{gitaly-wrapper => wrapper}/main.go (71%) rename cmd/{gitaly-wrapper => wrapper}/main_test.go (88%) diff --git a/.gitignore b/.gitignore index febee901750..b0e481a1fdf 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 89b8a5c8ae0..db1b544ff88 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/cmd/gitaly-wrapper/main.go b/cmd/wrapper/main.go similarity index 71% rename from cmd/gitaly-wrapper/main.go rename to cmd/wrapper/main.go index 0d06ff9784e..2120e7a589b 100644 --- a/cmd/gitaly-wrapper/main.go +++ b/cmd/wrapper/main.go @@ -29,67 +29,67 @@ 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 bootstrap.PidFile() == "" { log.Fatalf("missing pid file ENV variable %q", bootstrap.PidFileEnvVar) } - log.WithField("pid_file", bootstrap.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", bootstrap.UpgradesEnabledEnvVar)) @@ -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") } } @@ -139,13 +139,13 @@ 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 } diff --git a/cmd/gitaly-wrapper/main_test.go b/cmd/wrapper/main_test.go similarity index 88% rename from cmd/gitaly-wrapper/main_test.go rename to cmd/wrapper/main_test.go index 09423f2b076..e1cc76aeff7 100644 --- a/cmd/gitaly-wrapper/main_test.go +++ b/cmd/wrapper/main_test.go @@ -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")) }) } -- GitLab