From 9ad64b02d183e72b6548d7a36f9b96949f624e70 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Thu, 19 Nov 2015 15:41:15 -0500 Subject: [PATCH 1/9] speculator: guard against concurrent git commands --- scripts/speculator/main.go | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/scripts/speculator/main.go b/scripts/speculator/main.go index 97e67c8c..4472164c 100644 --- a/scripts/speculator/main.go +++ b/scripts/speculator/main.go @@ -23,6 +23,7 @@ import ( "path" "strconv" "strings" + "sync" "syscall" "time" @@ -136,17 +137,26 @@ func writeError(w http.ResponseWriter, code int, err error) { } type server struct { + mu sync.Mutex // Must be locked around any git command on matrixDocCloneURL matrixDocCloneURL string } +func (s *server) updateBase() error { + s.mu.Lock() + defer s.mu.Unlock() + return gitFetch(s.matrixDocCloneURL) +} + // generateAt generates spec from repo at sha. // Returns the path where the generation was done. func (s *server) generateAt(sha string) (dst string, err error) { - err = gitFetch(s.matrixDocCloneURL) + err = s.updateBase() if err != nil { return } + s.mu.Lock() dst, err = gitClone(s.matrixDocCloneURL, true) + s.mu.Unlock() if err != nil { return } @@ -164,7 +174,10 @@ func (s *server) getSHAOf(ref string) (string, error) { cmd.Dir = path.Join(s.matrixDocCloneURL) var b bytes.Buffer cmd.Stdout = &b - if err := cmd.Run(); err != nil { + s.mu.Lock() + err := cmd.Run() + s.mu.Unlock() + if err != nil { return "", fmt.Errorf("error generating spec: %v\nOutput from gendoc:\n%v", err, b.String()) } return strings.TrimSpace(b.String()), nil @@ -174,7 +187,7 @@ func (s *server) serveSpec(w http.ResponseWriter, req *http.Request) { var sha string if strings.ToLower(req.URL.Path) == "/spec/head" { - if err := gitFetch(s.matrixDocCloneURL); err != nil { + if err := s.updateBase(); err != nil { writeError(w, 500, err) return } @@ -383,7 +396,7 @@ func main() { if err != nil { log.Fatal(err) } - s := server{masterCloneDir} + s := server{matrixDocCloneURL: masterCloneDir} http.HandleFunc("/spec/", forceHTML(s.serveSpec)) http.HandleFunc("/diff/rst/", s.serveRSTDiff) http.HandleFunc("/diff/html/", forceHTML(s.serveHTMLDiff)) From 757b0bcd12269b0a9905efee7520e61abc81ec7e Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Thu, 19 Nov 2015 16:08:37 -0500 Subject: [PATCH 2/9] Try to build continuserv and speculator --- jenkins.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/jenkins.sh b/jenkins.sh index 0b217e58..b2aa8489 100755 --- a/jenkins.sh +++ b/jenkins.sh @@ -7,3 +7,8 @@ set -ex (cd scripts && ./gendoc.py -v) (cd api && npm install && node validator.js -s "client-server/v1" && node validator.js -s "client-server/v2_alpha") (cd event-schemas/ && ./check.sh) + +if which go >/dev/null 2>/dev/null; then + (cd scripts/continuserv && go build) + (cd scripts/speculator && go build) +fi From dd53847211c57553180efca9d843c1c8741d6de3 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Thu, 19 Nov 2015 16:11:19 -0500 Subject: [PATCH 3/9] Include command stderr in error text --- scripts/speculator/main.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/scripts/speculator/main.go b/scripts/speculator/main.go index 4472164c..00a59eea 100644 --- a/scripts/speculator/main.go +++ b/scripts/speculator/main.go @@ -70,13 +70,15 @@ const ( func gitClone(url string, shared bool) (string, error) { directory := path.Join("/tmp/matrix-doc", strconv.FormatInt(rand.Int63(), 10)) - cmd := exec.Command("git", "clone", url, directory) + if err := os.MkdirAll(directory, 0755); err != nil { + return "", fmt.Errorf("error making directory %s: %v", directory, err) + } + args := []string{"clone", url, directory} if shared { - cmd.Args = append(cmd.Args, "--shared") + args = append(args, "--shared") } - - if err := cmd.Run(); err != nil { - return "", fmt.Errorf("error cloning repo: %v", err) + if err := runGitCommand(directory, args); err != nil { + return "", err } return directory, nil } @@ -92,8 +94,10 @@ func gitFetch(path string) error { func runGitCommand(path string, args []string) error { cmd := exec.Command("git", args...) cmd.Dir = path + var b bytes.Buffer + cmd.Stderr = &b if err := cmd.Run(); err != nil { - return fmt.Errorf("error running %q: %v", strings.Join(cmd.Args, " "), err) + return fmt.Errorf("error running %q: %v (stderr: %s)", strings.Join(cmd.Args, " "), err, b.String()) } return nil } From 8872e17f9355a47f9b7a2822cfd1303d8e56e2bc Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Thu, 19 Nov 2015 16:14:22 -0500 Subject: [PATCH 4/9] Fall back to last known HEAD sha if fetch fails --- scripts/speculator/main.go | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/scripts/speculator/main.go b/scripts/speculator/main.go index 00a59eea..380cd2bc 100644 --- a/scripts/speculator/main.go +++ b/scripts/speculator/main.go @@ -87,10 +87,6 @@ func gitCheckout(path, sha string) error { return runGitCommand(path, []string{"checkout", sha}) } -func gitFetch(path string) error { - return runGitCommand(path, []string{"fetch"}) -} - func runGitCommand(path string, args []string) error { cmd := exec.Command("git", args...) cmd.Dir = path @@ -148,7 +144,7 @@ type server struct { func (s *server) updateBase() error { s.mu.Lock() defer s.mu.Unlock() - return gitFetch(s.matrixDocCloneURL) + return runGitCommand(s.matrixDocCloneURL, []string{"fetch"}) } // generateAt generates spec from repo at sha. @@ -191,16 +187,12 @@ func (s *server) serveSpec(w http.ResponseWriter, req *http.Request) { var sha string if strings.ToLower(req.URL.Path) == "/spec/head" { - if err := s.updateBase(); err != nil { + if headSha, err := s.lookupHeadSHA(); headSha == "" { writeError(w, 500, err) return + } else { + sha = headSha } - originHead, err := s.getSHAOf("origin/master") - if err != nil { - writeError(w, 500, err) - return - } - sha = originHead } else { pr, err := lookupPullRequest(*req.URL, "/spec") if err != nil { @@ -237,6 +229,25 @@ func (s *server) serveSpec(w http.ResponseWriter, req *http.Request) { specCache.Add(sha, b) } +// lookupHeadSHA looks up what origin/master's HEAD SHA is. +// It attempts to `git fetch` before doing so. +// If this fails, it may still return a stale sha, but will also return an error. +func (s *server) lookupHeadSHA() (sha string, retErr error) { + retErr = s.updateBase() + if retErr != nil { + log.Printf("Error fetching: %v, attempting to fall back to current known value", retErr) + } + originHead, err := s.getSHAOf("origin/master") + if err != nil { + retErr = err + } + sha = originHead + if retErr != nil && originHead != "" { + log.Printf("Successfully fell back to possibly stale sha: %s", sha) + } + return +} + func checkAuth(pr *PullRequest) error { if !pr.User.IsTrusted() { return fmt.Errorf("%q is not a trusted pull requester", pr.User.Login) From 6f1d00097be403998e5490845835ecb23d2c741c Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Thu, 19 Nov 2015 16:15:13 -0500 Subject: [PATCH 5/9] Only bother trying to fetch if we need to --- scripts/speculator/main.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/scripts/speculator/main.go b/scripts/speculator/main.go index 380cd2bc..4972210e 100644 --- a/scripts/speculator/main.go +++ b/scripts/speculator/main.go @@ -147,12 +147,20 @@ func (s *server) updateBase() error { return runGitCommand(s.matrixDocCloneURL, []string{"fetch"}) } +func (s *server) knowsAbout(sha string) bool { + s.mu.Lock() + defer s.mu.Unlock() + return runGitCommand(s.matrixDocCloneURL, []string{"cat-file", "-e", sha + "^{commit}"}) == nil +} + // generateAt generates spec from repo at sha. // Returns the path where the generation was done. func (s *server) generateAt(sha string) (dst string, err error) { - err = s.updateBase() - if err != nil { - return + if !s.knowsAbout(sha) { + err = s.updateBase() + if err != nil { + return + } } s.mu.Lock() dst, err = gitClone(s.matrixDocCloneURL, true) From e045f28b44137303acccc833ffb486008c46be06 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Mon, 23 Nov 2015 17:20:54 +0000 Subject: [PATCH 6/9] Pull out constant for permissions Also, drop permissions from 0755 to 0700 --- scripts/speculator/main.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/scripts/speculator/main.go b/scripts/speculator/main.go index 4972210e..cfe0f4ee 100644 --- a/scripts/speculator/main.go +++ b/scripts/speculator/main.go @@ -64,13 +64,14 @@ func (u *User) IsTrusted() bool { } const ( - pullsPrefix = "https://api.github.com/repos/matrix-org/matrix-doc/pulls" - matrixDocCloneURL = "https://github.com/matrix-org/matrix-doc.git" + pullsPrefix = "https://api.github.com/repos/matrix-org/matrix-doc/pulls" + matrixDocCloneURL = "https://github.com/matrix-org/matrix-doc.git" + permissionsOwnerFull = 0700 ) func gitClone(url string, shared bool) (string, error) { directory := path.Join("/tmp/matrix-doc", strconv.FormatInt(rand.Int63(), 10)) - if err := os.MkdirAll(directory, 0755); err != nil { + if err := os.MkdirAll(directory, permissionsOwnerFull); err != nil { return "", fmt.Errorf("error making directory %s: %v", directory, err) } args := []string{"clone", url, directory} From 866fa582763279063856ea89c3459e4eeb5ae2b7 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Mon, 23 Nov 2015 17:22:53 +0000 Subject: [PATCH 7/9] Rename --- scripts/speculator/main.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/scripts/speculator/main.go b/scripts/speculator/main.go index cfe0f4ee..e1874383 100644 --- a/scripts/speculator/main.go +++ b/scripts/speculator/main.go @@ -148,7 +148,8 @@ func (s *server) updateBase() error { return runGitCommand(s.matrixDocCloneURL, []string{"fetch"}) } -func (s *server) knowsAbout(sha string) bool { +// canCheckout returns whether a given sha can currently be checked out from s.matrixDocCloneURL. +func (s *server) canCheckout(sha string) bool { s.mu.Lock() defer s.mu.Unlock() return runGitCommand(s.matrixDocCloneURL, []string{"cat-file", "-e", sha + "^{commit}"}) == nil @@ -157,7 +158,7 @@ func (s *server) knowsAbout(sha string) bool { // generateAt generates spec from repo at sha. // Returns the path where the generation was done. func (s *server) generateAt(sha string) (dst string, err error) { - if !s.knowsAbout(sha) { + if !s.canCheckout(sha) { err = s.updateBase() if err != nil { return From c432396079645052c8a9c60eb5be822889d25117 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Mon, 23 Nov 2015 17:26:32 +0000 Subject: [PATCH 8/9] Add comment --- scripts/speculator/main.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/speculator/main.go b/scripts/speculator/main.go index e1874383..0c9a2bfa 100644 --- a/scripts/speculator/main.go +++ b/scripts/speculator/main.go @@ -197,6 +197,8 @@ func (s *server) serveSpec(w http.ResponseWriter, req *http.Request) { var sha string if strings.ToLower(req.URL.Path) == "/spec/head" { + // err may be non-nil here but if headSha is non-empty we will serve a possibly-stale result in favour of erroring. + // This is to deal with cases like where github is down but we still want to serve the spec. if headSha, err := s.lookupHeadSHA(); headSha == "" { writeError(w, 500, err) return From 6a6cbd9d24a7cbef9db04a57b6d273e09d51ddf1 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Mon, 23 Nov 2015 17:28:58 +0000 Subject: [PATCH 9/9] Always try to build continuserv & speculator on jenkins --- jenkins.sh | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/jenkins.sh b/jenkins.sh index b2aa8489..f5ed3b15 100755 --- a/jenkins.sh +++ b/jenkins.sh @@ -8,7 +8,11 @@ set -ex (cd api && npm install && node validator.js -s "client-server/v1" && node validator.js -s "client-server/v2_alpha") (cd event-schemas/ && ./check.sh) -if which go >/dev/null 2>/dev/null; then - (cd scripts/continuserv && go build) - (cd scripts/speculator && go build) -fi +: ${GOPATH:=${WORKSPACE}/.gopath} +mkdir -p "${GOPATH}" +export GOPATH +go get github.com/hashicorp/golang-lru +go get gopkg.in/fsnotify.v1 + +(cd scripts/continuserv && go build) +(cd scripts/speculator && go build)