From 9fdb66e0a424b911ce2ee768d9d08d027b330968 Mon Sep 17 00:00:00 2001 From: Jay Date: Thu, 4 Jan 2024 20:44:34 +0000 Subject: [PATCH 1/4] Conditionally write tiles. --- experimental/gcp-log/internal/storage/storage.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/experimental/gcp-log/internal/storage/storage.go b/experimental/gcp-log/internal/storage/storage.go index 7f781aa..a362613 100644 --- a/experimental/gcp-log/internal/storage/storage.go +++ b/experimental/gcp-log/internal/storage/storage.go @@ -405,7 +405,8 @@ func (c *Client) StoreTile(ctx context.Context, level, index uint64, tile *api.T tPath := filepath.Join(layout.TilePath("", level, index, tileSize%256)) obj := bkt.Object(tPath) - w := obj.NewWriter(ctx) + // Tiles, partial or full, should only be written once. + w := obj.If(gcs.Conditions{DoesNotExist: true}).NewWriter(ctx) if c.otherCacheControl != "" { w.ObjectAttrs.CacheControl = c.otherCacheControl } From 17bfbbd6b953fae9108328a527508e3718a4f5e5 Mon Sep 17 00:00:00 2001 From: Jay Date: Mon, 8 Jan 2024 14:54:59 +0000 Subject: [PATCH 2/4] Check that the tile content is the same if it exists already. --- .../gcp-log/internal/storage/storage.go | 51 +++++++++++++++++-- 1 file changed, 48 insertions(+), 3 deletions(-) diff --git a/experimental/gcp-log/internal/storage/storage.go b/experimental/gcp-log/internal/storage/storage.go index a362613..61fe31f 100644 --- a/experimental/gcp-log/internal/storage/storage.go +++ b/experimental/gcp-log/internal/storage/storage.go @@ -16,6 +16,7 @@ package storage import ( + "bytes" "context" "errors" "fmt" @@ -376,7 +377,7 @@ func (c *Client) Sequence(ctx context.Context, leafhash []byte, leaf []byte) (ui return 0, fmt.Errorf("couldn't create leafhash object: %w", err) } if err := wLeaf.Close(); err != nil { - return 0, fmt.Errorf("couldn't close writer for object %q", leafPath) + return 0, fmt.Errorf("couldn't close writer for object %q, %w", leafPath, err) } // All done! @@ -384,6 +385,37 @@ func (c *Client) Sequence(ctx context.Context, leafhash []byte, leaf []byte) (ui } } +// assertContent checks that the content at `gcsPath` matches the passed in `data`. +func (c *Client) assertContent(ctx context.Context, gcsPath string, data []byte) (equal bool, err error) { + bkt := c.gcsClient.Bucket(c.bucket) + + obj := bkt.Object(gcsPath) + r, err := obj.NewReader(ctx) + if err != nil { + if errors.Is(err, gcs.ErrObjectNotExist) { + // Return the generic NotExist error so that tileCache.Visit can differentiate + // between this and other errors. + return false, err + } + + klog.V(2).Infof("GetTile: failed to create reader for object %q in bucket %q: %v", + gcsPath, c.bucket, err) + return false, err + } + defer r.Close() + + var gcsData []byte + gcsData, err = io.ReadAll(r) + if err != nil { + return false, err + } + + if bytes.Equal(gcsData, data) { + return true, nil + } + return false, nil +} + // StoreTile writes a tile out to GCS. // Fully populated tiles are stored at the path corresponding to the level & // index parameters, partially populated (i.e. right-hand edge) tiles are @@ -403,10 +435,23 @@ func (c *Client) StoreTile(ctx context.Context, level, index uint64, tile *api.T // Pass an empty rootDir since we don't need this concept in GCS. tPath := filepath.Join(layout.TilePath("", level, index, tileSize%256)) + + // Check that either we are writing the tile for the first time, or that the + // write would be a no-op since the content is the same. + // + // gcs.ErrObjectNotExist is okay, we will just write the tile below. + if equal, err := c.assertContent(ctx, tPath, t); err != nil && !errors.Is(err, gcs.ErrObjectNotExist) { + return fmt.Errorf("failed to read content of %q: %w", tPath, err) + } else if err == nil && !equal { + return fmt.Errorf("assertion that tile content for %q has not changed failed", tPath) + } else if err == nil && equal { + klog.V(2).Infof("StoreTile: Tile already exists for level %d index %x ts: %x", level, index, tileSize) + return nil + } + obj := bkt.Object(tPath) - // Tiles, partial or full, should only be written once. - w := obj.If(gcs.Conditions{DoesNotExist: true}).NewWriter(ctx) + w := obj.NewWriter(ctx) if c.otherCacheControl != "" { w.ObjectAttrs.CacheControl = c.otherCacheControl } From 6b6173e481e66b797ef4a6a9ee9cb0839dbdad7c Mon Sep 17 00:00:00 2001 From: Jay Date: Mon, 8 Jan 2024 21:34:25 +0000 Subject: [PATCH 3/4] Only check content of tile path if it already exists. --- .../gcp-log/internal/storage/storage.go | 51 ++++++++++--------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/experimental/gcp-log/internal/storage/storage.go b/experimental/gcp-log/internal/storage/storage.go index 61fe31f..b1936a8 100644 --- a/experimental/gcp-log/internal/storage/storage.go +++ b/experimental/gcp-log/internal/storage/storage.go @@ -392,20 +392,13 @@ func (c *Client) assertContent(ctx context.Context, gcsPath string, data []byte) obj := bkt.Object(gcsPath) r, err := obj.NewReader(ctx) if err != nil { - if errors.Is(err, gcs.ErrObjectNotExist) { - // Return the generic NotExist error so that tileCache.Visit can differentiate - // between this and other errors. - return false, err - } - - klog.V(2).Infof("GetTile: failed to create reader for object %q in bucket %q: %v", + klog.V(2).Infof("assertContent: failed to create reader for object %q in bucket %q: %v", gcsPath, c.bucket, err) return false, err } defer r.Close() - var gcsData []byte - gcsData, err = io.ReadAll(r) + gcsData, err := io.ReadAll(r) if err != nil { return false, err } @@ -435,28 +428,36 @@ func (c *Client) StoreTile(ctx context.Context, level, index uint64, tile *api.T // Pass an empty rootDir since we don't need this concept in GCS. tPath := filepath.Join(layout.TilePath("", level, index, tileSize%256)) - - // Check that either we are writing the tile for the first time, or that the - // write would be a no-op since the content is the same. - // - // gcs.ErrObjectNotExist is okay, we will just write the tile below. - if equal, err := c.assertContent(ctx, tPath, t); err != nil && !errors.Is(err, gcs.ErrObjectNotExist) { - return fmt.Errorf("failed to read content of %q: %w", tPath, err) - } else if err == nil && !equal { - return fmt.Errorf("assertion that tile content for %q has not changed failed", tPath) - } else if err == nil && equal { - klog.V(2).Infof("StoreTile: Tile already exists for level %d index %x ts: %x", level, index, tileSize) - return nil - } - obj := bkt.Object(tPath) - w := obj.NewWriter(ctx) + // Tiles, partial or full, should only be written once. + w := obj.If(gcs.Conditions{DoesNotExist: true}).NewWriter(ctx) if c.otherCacheControl != "" { w.ObjectAttrs.CacheControl = c.otherCacheControl } if _, err := w.Write(t); err != nil { return fmt.Errorf("failed to write tile object %q to bucket %q: %w", tPath, c.bucket, err) } - return w.Close() + + if err := w.Close(); err != nil { + switch ee := err.(type) { + case *googleapi.Error: + // If we run into a precondition failure error, check that the object + // which exists contains the same content that we want to write. + if ee.Code == http.StatusPreconditionFailed { + if equal, err := c.assertContent(ctx, tPath, t); err != nil { + return fmt.Errorf("failed to read content of %q: %w", tPath, err) + } else if !equal { + return fmt.Errorf("assertion that tile content for %q has not changed failed", tPath) + } else if equal { + klog.V(2).Infof("StoreTile: Tile already exists for level %d index %x ts: %x", level, index, tileSize) + return nil + } + } + default: + return err + } + } + + return nil } From e53ecbf92b3f41ce3ed99f1c41de6b4737bb3313 Mon Sep 17 00:00:00 2001 From: Jay Date: Tue, 9 Jan 2024 11:15:41 +0000 Subject: [PATCH 4/4] Respond to comments. --- experimental/gcp-log/internal/storage/storage.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/experimental/gcp-log/internal/storage/storage.go b/experimental/gcp-log/internal/storage/storage.go index b1936a8..9ff52f8 100644 --- a/experimental/gcp-log/internal/storage/storage.go +++ b/experimental/gcp-log/internal/storage/storage.go @@ -449,10 +449,10 @@ func (c *Client) StoreTile(ctx context.Context, level, index uint64, tile *api.T return fmt.Errorf("failed to read content of %q: %w", tPath, err) } else if !equal { return fmt.Errorf("assertion that tile content for %q has not changed failed", tPath) - } else if equal { - klog.V(2).Infof("StoreTile: Tile already exists for level %d index %x ts: %x", level, index, tileSize) - return nil } + + klog.V(2).Infof("StoreTile: identical tile already exists for level %d index %x ts: %x", level, index, tileSize) + return nil } default: return err