Skip to content

Commit 9844ec3

Browse files
heschigopherbot
heschi
authored and
gopherbot
committed
internal/relui: use atomic writes in test code too
In CL 429275 I changed gcsfs to write atomically, but I didn't use gcsfs in the test infrastructure, which is probably what was causing the flakes. For whatever reason, the test got much flakier, with timeouts this time :( Finish the job. I'm hoping this fixes the timeouts. I can't reproduce them locally, but I did get some errors where it saw the temporary files, so also hide them from directory listings. For golang/go#53972. Change-Id: I2dff87305f9114b975990cd6649cb4a087fadffd Reviewed-on: https://go-review.googlesource.com/c/build/+/429536 Run-TryBot: Heschi Kreinick <heschi@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> Auto-Submit: Heschi Kreinick <heschi@google.com>
1 parent 656fd83 commit 9844ec3

File tree

5 files changed

+65
-52
lines changed

5 files changed

+65
-52
lines changed

‎internal/gcsfs/gcsfs.go

+18-5
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ func FromURL(ctx context.Context, client *storage.Client, base string) (fs.FS, e
4444
}
4545

4646
// Create creates a new file on fsys, which must be a CreateFS.
47-
func Create(fsys fs.FS, name string) (WriteFile, error) {
47+
func Create(fsys fs.FS, name string) (WriterFile, error) {
4848
cfs, ok := fsys.(CreateFS)
4949
if !ok {
5050
return nil, &fs.PathError{Op: "create", Path: name, Err: fmt.Errorf("not implemented on type %T", fsys)}
@@ -55,16 +55,29 @@ func Create(fsys fs.FS, name string) (WriteFile, error) {
5555
// CreateFS is an fs.FS that supports creating writable files.
5656
type CreateFS interface {
5757
fs.FS
58-
Create(string) (WriteFile, error)
58+
Create(string) (WriterFile, error)
5959
}
6060

61-
// WriteFile is an fs.File that can be written to.
61+
// WriterFile is an fs.File that can be written to.
6262
// The behavior of writing and reading the same file is undefined.
63-
type WriteFile interface {
63+
type WriterFile interface {
6464
fs.File
6565
io.Writer
6666
}
6767

68+
// WriteFile is like os.WriteFile for CreateFSs.
69+
func WriteFile(fsys fs.FS, filename string, contents []byte) error {
70+
f, err := Create(fsys, filename)
71+
if err != nil {
72+
return err
73+
}
74+
defer f.Close()
75+
if _, err := f.Write(contents); err != nil {
76+
return err
77+
}
78+
return f.Close()
79+
}
80+
6881
// gcsFS implements fs.FS for GCS.
6982
type gcsFS struct {
7083
ctx context.Context
@@ -110,7 +123,7 @@ func (fsys *gcsFS) Open(name string) (fs.File, error) {
110123
}
111124

112125
// Create creates the named file.
113-
func (fsys *gcsFS) Create(name string) (WriteFile, error) {
126+
func (fsys *gcsFS) Create(name string) (WriterFile, error) {
114127
f, err := fsys.Open(name)
115128
if err != nil {
116129
return nil, err

‎internal/gcsfs/gcsfs_test.go

+15
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"flag"
1010
"io/fs"
1111
"io/ioutil"
12+
"os"
1213
"path/filepath"
1314
"testing"
1415
"testing/fstest"
@@ -54,6 +55,20 @@ func TestDirFS(t *testing.T) {
5455
}
5556
}
5657

58+
func TestDirFSDotFiles(t *testing.T) {
59+
temp := t.TempDir()
60+
if err := os.WriteFile(temp+"/.foo", nil, 0777); err != nil {
61+
t.Fatal(err)
62+
}
63+
files, err := fs.ReadDir(DirFS(temp), ".")
64+
if err != nil {
65+
t.Fatal(err)
66+
}
67+
if len(files) != 0 {
68+
t.Errorf("ReadDir didn't hide . files: %v", files)
69+
}
70+
}
71+
5772
func TestDirFSWrite(t *testing.T) {
5873
temp := t.TempDir()
5974
fsys := DirFS(temp)

‎internal/gcsfs/osfs.go

+17-5
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"os"
1010
"path"
1111
"runtime"
12+
"strings"
1213
)
1314

1415
var _ = fs.FS((*dirFS)(nil))
@@ -41,7 +42,7 @@ func (dir dirFS) Open(name string) (fs.File, error) {
4142
if err != nil {
4243
return nil, err // nil fs.File
4344
}
44-
return f, nil
45+
return &atomicWriteFile{f, nil}, nil
4546
}
4647

4748
func (dir dirFS) Stat(name string) (fs.FileInfo, error) {
@@ -55,7 +56,7 @@ func (dir dirFS) Stat(name string) (fs.FileInfo, error) {
5556
return f, nil
5657
}
5758

58-
func (dir dirFS) Create(name string) (WriteFile, error) {
59+
func (dir dirFS) Create(name string) (WriterFile, error) {
5960
if !fs.ValidPath(name) || runtime.GOOS == "windows" && containsAny(name, `\:`) {
6061
return nil, &fs.PathError{Op: "create", Path: name, Err: fs.ErrInvalid}
6162
}
@@ -73,15 +74,26 @@ func (dir dirFS) Create(name string) (WriteFile, error) {
7374
finalize := func() error {
7475
return os.Rename(temp.Name(), fullName)
7576
}
76-
return &writingFile{temp, finalize}, nil
77+
return &atomicWriteFile{temp, finalize}, nil
7778
}
7879

79-
type writingFile struct {
80+
type atomicWriteFile struct {
8081
*os.File
8182
finalize func() error
8283
}
8384

84-
func (wf *writingFile) Close() error {
85+
func (wf *atomicWriteFile) ReadDir(n int) ([]fs.DirEntry, error) {
86+
unfiltered, err := wf.File.ReadDir(n)
87+
var result []fs.DirEntry
88+
for _, de := range unfiltered {
89+
if !strings.HasPrefix(de.Name(), ".") {
90+
result = append(result, de)
91+
}
92+
}
93+
return result, err
94+
}
95+
96+
func (wf *atomicWriteFile) Close() error {
8597
if err := wf.File.Close(); err != nil {
8698
return err
8799
}

‎internal/relui/buildrelease_test.go

+12-16
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"crypto/sha256"
1414
"fmt"
1515
"io"
16+
"io/fs"
1617
"io/ioutil"
1718
"net/http"
1819
"net/http/httptest"
@@ -34,6 +35,7 @@ import (
3435
"golang.org/x/build/buildlet"
3536
"golang.org/x/build/gerrit"
3637
"golang.org/x/build/internal"
38+
"golang.org/x/build/internal/gcsfs"
3739
"golang.org/x/build/internal/task"
3840
"golang.org/x/build/internal/untar"
3941
"golang.org/x/build/internal/workflow"
@@ -861,14 +863,15 @@ func fakeSign(ctx context.Context, t *testing.T, dir string) {
861863
}
862864

863865
func fakeSignOnce(t *testing.T, dir string, seen map[string]bool) error {
864-
_, err := os.Stat(filepath.Join(dir, "ready"))
866+
dirFS := gcsfs.DirFS(dir)
867+
_, err := fs.Stat(dirFS, "ready")
865868
if os.IsNotExist(err) {
866869
return nil
867870
}
868871
if err != nil {
869872
return err
870873
}
871-
contents, err := os.ReadDir(dir)
874+
contents, err := fs.ReadDir(dirFS, ".")
872875
if err != nil {
873876
return err
874877
}
@@ -892,24 +895,16 @@ func fakeSignOnce(t *testing.T, dir string, seen map[string]bool) error {
892895
copy = true
893896
}
894897

895-
if err := os.MkdirAll(filepath.Join(dir, "signed"), 0777); err != nil {
896-
t.Fatal(err)
897-
}
898-
899898
writeSignedWithHash := func(filename string, contents []byte) error {
900-
path := filepath.Join(dir, "signed", filename)
901-
if err := ioutil.WriteFile(path, contents, 0777); err != nil {
899+
if err := gcsfs.WriteFile(dirFS, "signed/"+filename, contents); err != nil {
902900
return err
903901
}
904902
hash := fmt.Sprintf("%x", sha256.Sum256(contents))
905-
if err := ioutil.WriteFile(path+".sha256", []byte(hash), 0777); err != nil {
906-
return err
907-
}
908-
return nil
903+
return gcsfs.WriteFile(dirFS, "signed/"+filename+".sha256", []byte(hash))
909904
}
910905

911906
if copy {
912-
bytes, err := ioutil.ReadFile(filepath.Join(dir, fn))
907+
bytes, err := fs.ReadFile(dirFS, fn)
913908
if err != nil {
914909
return err
915910
}
@@ -1022,9 +1017,10 @@ func TestFakeSign(t *testing.T) {
10221017
}
10231018

10241019
func fakeCDNLoad(ctx context.Context, t *testing.T, from, to string) {
1020+
fromFS, toFS := gcsfs.DirFS(from), gcsfs.DirFS(to)
10251021
seen := map[string]bool{}
10261022
periodicallyDo(ctx, t, 100*time.Millisecond, func() error {
1027-
files, err := os.ReadDir(from)
1023+
files, err := fs.ReadDir(fromFS, ".")
10281024
if err != nil {
10291025
return err
10301026
}
@@ -1033,11 +1029,11 @@ func fakeCDNLoad(ctx context.Context, t *testing.T, from, to string) {
10331029
continue
10341030
}
10351031
seen[f.Name()] = true
1036-
contents, err := os.ReadFile(filepath.Join(from, f.Name()))
1032+
contents, err := fs.ReadFile(fromFS, f.Name())
10371033
if err != nil {
10381034
return err
10391035
}
1040-
if err := os.WriteFile(filepath.Join(to, f.Name()), contents, 0777); err != nil {
1036+
if err := gcsfs.WriteFile(toFS, f.Name(), contents); err != nil {
10411037
return err
10421038
}
10431039
}

‎internal/relui/workflows.go

+3-26
Original file line numberDiff line numberDiff line change
@@ -904,14 +904,7 @@ func (tasks *BuildReleaseTasks) copyToStaging(ctx *wf.TaskContext, version strin
904904
return nil, err
905905
}
906906
}
907-
out, err := gcsfs.Create(scratchFS, path.Join(signingStagingDir(ctx, version), "ready"))
908-
if err != nil {
909-
return nil, err
910-
}
911-
if _, err := out.Write([]byte("ready")); err != nil {
912-
return nil, err
913-
}
914-
if err := out.Close(); err != nil {
907+
if err := gcsfs.WriteFile(scratchFS, path.Join(signingStagingDir(ctx, version), "ready"), []byte("ready")); err != nil {
915908
return nil, err
916909
}
917910
return stagedArtifacts, nil
@@ -1084,28 +1077,12 @@ func uploadArtifact(scratchFS, servingFS fs.FS, a artifact) error {
10841077
return err
10851078
}
10861079

1087-
sha256, err := gcsfs.Create(servingFS, a.Filename+".sha256")
1088-
if err != nil {
1089-
return err
1090-
}
1091-
defer sha256.Close()
1092-
if _, err := sha256.Write([]byte(a.SHA256)); err != nil {
1093-
return err
1094-
}
1095-
if err := sha256.Close(); err != nil {
1080+
if err := gcsfs.WriteFile(servingFS, a.Filename+".sha256", []byte(a.SHA256)); err != nil {
10961081
return err
10971082
}
10981083

10991084
if a.GPGSignature != "" {
1100-
asc, err := gcsfs.Create(servingFS, a.Filename+".asc")
1101-
if err != nil {
1102-
return err
1103-
}
1104-
defer asc.Close()
1105-
if _, err := asc.Write([]byte(a.GPGSignature)); err != nil {
1106-
return err
1107-
}
1108-
if err := asc.Close(); err != nil {
1085+
if err := gcsfs.WriteFile(servingFS, a.Filename+".asc", []byte(a.GPGSignature)); err != nil {
11091086
return err
11101087
}
11111088
}

0 commit comments

Comments
 (0)