Skip to content

Commit

Permalink
feat: use the staged temp file
Browse files Browse the repository at this point in the history 
Turns out we can just pack the reader back. As it's a file we can
just seek back to the beginning and read it again. This feels the
best and correct way, without causing changes to the objectstore.
  • Loading branch information
@SimonRichardson
SimonRichardson committed Jan 6, 2025
1 parent 6b9b034 commit 14b4fc6
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 65 deletions.
80 changes: 60 additions & 20 deletions domain/application/charm/store/store.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,20 +39,29 @@ type Digest struct {
Size int64
}

// StoreResult contains the path of the stored charm archive, the unique name
// of the charm archive, and the object store UUID.
type StoreResult struct {
UniqueName string
ObjectStoreUUID objectstore.UUID
}

// CharmReader is an interface that combines the io.Reader, io.ReaderAt, and
// io.Closer interfaces.
type CharmReader interface {
io.Reader
io.ReaderAt
io.Closer
}

// StoreFromReaderResult contains the unique name of the charm archive and the
// object store UUID.
type StoreFromReaderResult struct {
Charm CharmReader
UniqueName string
ObjectStoreUUID objectstore.UUID
}

// StoreResult contains the path of the stored charm archive, the unique name
// of the charm archive, and the object store UUID.
type StoreResult struct {
StoreFromReaderResult
ArchivePath string
}

// CharmStore provides an API for storing and retrieving charm blobs.
type CharmStore struct {
objectStoreGetter objectstore.ModelObjectStoreGetter
Expand Down Expand Up @@ -101,31 +110,30 @@ func (s *CharmStore) Store(ctx context.Context, path string, size int64, sha384
return StoreResult{}, errors.Errorf("putting charm: %w", err)
}
return StoreResult{
StoreFromReaderResult: StoreFromReaderResult{
UniqueName: uniqueName,
ObjectStoreUUID: uuid,
},
ArchivePath: file.Name(),
UniqueName: uniqueName,
ObjectStoreUUID: uuid,
}, nil
}

// StoreFromReader stores the charm from the provided reader into the object
// store. The caller is expected to remove the temporary file after the call.
// IThis does not check the integrity of the charm hash.
func (s *CharmStore) StoreFromReader(ctx context.Context, reader io.Reader, hashPrefix string) (StoreFromReaderResult, Digest, error) {
func (s *CharmStore) StoreFromReader(ctx context.Context, reader io.Reader, hashPrefix string) (_ StoreFromReaderResult, _ Digest, err error) {
file, err := os.CreateTemp("", "charm-")
if err != nil {
return StoreFromReaderResult{}, Digest{}, errors.Errorf("creating temporary file: %w", err)
}

// Ensure that we close any open handles to the file.
// Clean up the temporary file if an error occurs.
defer func() {
_ = file.Close()
if err := os.Remove(file.Name()); err != nil {
// We don't need to log this as error, as the file will be removed
// when the process exits or by the OS. It's not a direct action
// by the user, hence Info.
s.logger.Infof("removing temporary file: %v", err)
if err == nil {
return
}
if closeErr := file.Close(); closeErr != nil {
s.logger.Errorf("closing temporary file: %v", closeErr)
}
if removeErr := os.Remove(file.Name()); removeErr != nil {
s.logger.Errorf("removing temporary file: %v", removeErr)
}
}()

Expand Down Expand Up @@ -166,7 +174,15 @@ func (s *CharmStore) StoreFromReader(ctx context.Context, reader io.Reader, hash
return StoreFromReaderResult{}, Digest{}, errors.Errorf("putting charm: %w", err)
}

if _, err := file.Seek(0, io.SeekStart); err != nil {
return StoreFromReaderResult{}, Digest{}, errors.Errorf("seeking temporary file: %w", err)
}

return StoreFromReaderResult{
Charm: &charmReaderCloser{
file: file,
logger: s.logger,
},
UniqueName: uniqueName,
ObjectStoreUUID: uuid,
}, Digest{
Expand Down Expand Up @@ -212,6 +228,30 @@ func (s *CharmStore) GetBySHA256Prefix(ctx context.Context, sha256Prefix string)
return reader, nil
}

type charmReaderCloser struct {
file *os.File
logger logger.Logger
}

func (c *charmReaderCloser) Read(p []byte) (n int, err error) {
return c.file.Read(p)
}

func (c *charmReaderCloser) ReadAt(p []byte, off int64) (n int, err error) {
return c.file.ReadAt(p, off)
}

func (c *charmReaderCloser) Close() error {
err := c.file.Close()
if removeErr := os.Remove(c.file.Name()); removeErr != nil {
// We don't need to log this as error, as the file will be removed
// when the process exits or by the OS. It's not a direct action
// by the user, hence Info.
c.logger.Infof("removing temporary file: %v", removeErr)
}
return err
}

func storeAndComputeHashes(writer io.Writer, reader io.Reader) (string, string, int64, error) {
hasher256 := sha256.New()
hasher384 := sha512.New384()
Expand Down
14 changes: 4 additions & 10 deletions domain/application/service/application_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1338,11 +1338,8 @@ func (s *applicationServiceSuite) TestResolveCharmDownload(c *gc.C) {

s.state.EXPECT().GetAsyncCharmDownloadInfo(gomock.Any(), appUUID).Return(info, nil)
s.charmStore.EXPECT().Store(gomock.Any(), path, int64(42), "hash-384").Return(store.StoreResult{
StoreFromReaderResult: store.StoreFromReaderResult{
UniqueName: "somepath",
ObjectStoreUUID: objectStoreUUID,
},
ArchivePath: "/tmp/somepath",
UniqueName: "somepath",
ObjectStoreUUID: objectStoreUUID,
}, nil)
s.state.EXPECT().ResolveCharmDownload(gomock.Any(), charmUUID, application.ResolvedCharmDownload{
Actions: actions,
Expand Down Expand Up @@ -1512,11 +1509,8 @@ func (s *applicationServiceSuite) TestResolveCharmDownloadAlreadyStored(c *gc.C)

s.state.EXPECT().GetAsyncCharmDownloadInfo(gomock.Any(), appUUID).Return(info, nil)
s.charmStore.EXPECT().Store(gomock.Any(), path, int64(42), "hash-384").Return(store.StoreResult{
StoreFromReaderResult: store.StoreFromReaderResult{
UniqueName: "somepath",
ObjectStoreUUID: objectStoreUUID,
},
ArchivePath: "/tmp/somepath",
UniqueName: "somepath",
ObjectStoreUUID: objectStoreUUID,
}, nil)
s.state.EXPECT().ResolveCharmDownload(gomock.Any(), charmUUID, application.ResolvedCharmDownload{
Actions: actions,
Expand Down
19 changes: 4 additions & 15 deletions domain/application/service/charm.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
package service

import (
"bytes"
"context"
"fmt"
"io"
Expand Down Expand Up @@ -691,13 +690,8 @@ func (s *Service) ResolveUploadCharm(ctx context.Context, args charm.ResolveUplo
}

func (s *Service) resolveLocalUploadedCharm(ctx context.Context, args charm.ResolveUploadCharm) (charm.CharmLocator, error) {
// The charm has been stored in the objectstore. Rather than read it back
// out (consider S3 in a different region), we can just tee the reader
// into a buffer and read the charm from the buffer.
buffer := new(bytes.Buffer)

// Store the charm and validate it against the sha256 prefix.
result, digest, err := s.charmStore.StoreFromReader(ctx, io.TeeReader(args.Reader, buffer), args.SHA256Prefix)
result, digest, err := s.charmStore.StoreFromReader(ctx, args.Reader, args.SHA256Prefix)
if err != nil {
return charm.CharmLocator{}, internalerrors.Errorf("resolving uploaded charm: %w", err)
}
Expand All @@ -712,7 +706,7 @@ func (s *Service) resolveLocalUploadedCharm(ctx context.Context, args charm.Reso
// of the SHA256 hash (that's enough to prevent collisions).

// Make sure it's actually a valid charm.
ch, err := internalcharm.ReadCharmArchiveBytes(buffer.Bytes())
ch, err := internalcharm.ReadCharmArchiveFromReader(result.Charm, digest.Size)
if err != nil {
return charm.CharmLocator{}, internalerrors.Errorf("reading charm archive: %w", err)
}
Expand Down Expand Up @@ -774,12 +768,7 @@ func (s *Service) resolveMigratingUploadedCharm(ctx context.Context, args charm.
return charm.CharmLocator{}, errors.Annotate(err, "locating existing charm")
}

// The charm has been stored in the objectstore. Rather than read it back
// out (consider S3 in a different region), we can just tee the reader
// into a buffer and read the charm from the buffer.
buffer := new(bytes.Buffer)

result, digest, err := s.charmStore.StoreFromReader(ctx, io.TeeReader(args.Reader, buffer), args.SHA256Prefix)
result, digest, err := s.charmStore.StoreFromReader(ctx, args.Reader, args.SHA256Prefix)
if err != nil {
return charm.CharmLocator{}, internalerrors.Errorf("resolving uploaded charm: %w", err)
}
Expand All @@ -794,7 +783,7 @@ func (s *Service) resolveMigratingUploadedCharm(ctx context.Context, args charm.
// before accepting the charm. For now this is a check to ensure that the
// charm is valid.
// Work should be done to ensure the integrity of the charm archive.
if _, err := internalcharm.ReadCharmArchiveBytes(buffer.Bytes()); err != nil {
if _, err := internalcharm.ReadCharmArchiveFromReader(result.Charm, digest.Size); err != nil {
return charm.CharmLocator{}, errors.Annotatef(err, "reading charm archive")
}

Expand Down
40 changes: 20 additions & 20 deletions domain/application/service/charm_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1351,9 +1351,9 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmNotImporting(c *gc.C

dst := c.MkDir()
path := testcharms.Repo.CharmArchivePath(dst, "dummy")
reader, err := os.Open(path)
file, err := os.Open(path)
c.Assert(err, jc.ErrorIsNil)
stat, err := reader.Stat()
stat, err := file.Stat()
c.Assert(err, jc.ErrorIsNil)

charmID := charmtesting.GenCharmID(c)
Expand All @@ -1365,11 +1365,11 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmNotImporting(c *gc.C

s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").
DoAndReturn(func(ctx context.Context, r io.Reader, s string) (store.StoreFromReaderResult, store.Digest, error) {
// Force the reader, so we populate the buffer.
_, err := io.Copy(io.Discard, r)
_, err := file.Seek(0, io.SeekStart)
c.Assert(err, jc.ErrorIsNil)

return store.StoreFromReaderResult{
Charm: file,
ObjectStoreUUID: objectStoreUUID,
UniqueName: "unique-name",
}, store.Digest{
Expand All @@ -1390,7 +1390,7 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmNotImporting(c *gc.C

locator, err := s.service.ResolveUploadCharm(context.Background(), charm.ResolveUploadCharm{
Source: corecharm.Local,
Reader: reader,
Reader: file,
SHA256Prefix: "abc",
Architecture: arch.AMD64,
Revision: 1,
Expand Down Expand Up @@ -1435,9 +1435,9 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmNotImportingFailedSe

dst := c.MkDir()
path := testcharms.Repo.CharmArchivePath(dst, "dummy")
reader, err := os.Open(path)
file, err := os.Open(path)
c.Assert(err, jc.ErrorIsNil)
stat, err := reader.Stat()
stat, err := file.Stat()
c.Assert(err, jc.ErrorIsNil)

charmID := charmtesting.GenCharmID(c)
Expand All @@ -1449,11 +1449,11 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmNotImportingFailedSe

s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").
DoAndReturn(func(ctx context.Context, r io.Reader, s string) (store.StoreFromReaderResult, store.Digest, error) {
// Force the reader, so we populate the buffer.
_, err := io.Copy(io.Discard, r)
_, err := file.Seek(0, io.SeekStart)
c.Assert(err, jc.ErrorIsNil)

return store.StoreFromReaderResult{
Charm: file,
ObjectStoreUUID: objectStoreUUID,
UniqueName: "unique-name",
}, store.Digest{
Expand All @@ -1468,7 +1468,7 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmNotImportingFailedSe

_, err = s.service.ResolveUploadCharm(context.Background(), charm.ResolveUploadCharm{
Source: corecharm.Local,
Reader: reader,
Reader: file,
SHA256Prefix: "abc",
Architecture: arch.AMD64,
Revision: 1,
Expand All @@ -1485,9 +1485,9 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmImporting(c *gc.C) {

dst := c.MkDir()
path := testcharms.Repo.CharmArchivePath(dst, "dummy")
reader, err := os.Open(path)
file, err := os.Open(path)
c.Assert(err, jc.ErrorIsNil)
stat, err := reader.Stat()
stat, err := file.Stat()
c.Assert(err, jc.ErrorIsNil)

charmID := charmtesting.GenCharmID(c)
Expand All @@ -1500,11 +1500,11 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmImporting(c *gc.C) {
s.state.EXPECT().GetCharmID(gomock.Any(), "test", 1, charm.LocalSource).Return(charmID, nil)
s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").
DoAndReturn(func(ctx context.Context, r io.Reader, s string) (store.StoreFromReaderResult, store.Digest, error) {
// Force the reader, so we populate the buffer.
_, err := io.Copy(io.Discard, r)
_, err := file.Seek(0, io.SeekStart)
c.Assert(err, jc.ErrorIsNil)

return store.StoreFromReaderResult{
Charm: file,
ObjectStoreUUID: objectStoreUUID,
UniqueName: "unique-name",
}, store.Digest{
Expand All @@ -1527,7 +1527,7 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmImporting(c *gc.C) {

locator, err := s.service.ResolveUploadCharm(context.Background(), charm.ResolveUploadCharm{
Source: corecharm.Local,
Reader: reader,
Reader: file,
SHA256Prefix: "abc",
Architecture: arch.AMD64,
Revision: 1,
Expand Down Expand Up @@ -1607,9 +1607,9 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmImportingFailedResol

dst := c.MkDir()
path := testcharms.Repo.CharmArchivePath(dst, "dummy")
reader, err := os.Open(path)
file, err := os.Open(path)
c.Assert(err, jc.ErrorIsNil)
stat, err := reader.Stat()
stat, err := file.Stat()
c.Assert(err, jc.ErrorIsNil)

charmID := charmtesting.GenCharmID(c)
Expand All @@ -1622,11 +1622,11 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmImportingFailedResol
s.state.EXPECT().GetCharmID(gomock.Any(), "test", 1, charm.LocalSource).Return(charmID, nil)
s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").
DoAndReturn(func(ctx context.Context, r io.Reader, s string) (store.StoreFromReaderResult, store.Digest, error) {
// Force the reader, so we populate the buffer.
_, err := io.Copy(io.Discard, r)
_, err := file.Seek(0, io.SeekStart)
c.Assert(err, jc.ErrorIsNil)

return store.StoreFromReaderResult{
Charm: file,
ObjectStoreUUID: objectStoreUUID,
UniqueName: "unique-name",
}, store.Digest{
Expand All @@ -1649,7 +1649,7 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmImportingFailedResol

_, err = s.service.ResolveUploadCharm(context.Background(), charm.ResolveUploadCharm{
Source: corecharm.Local,
Reader: reader,
Reader: file,
SHA256Prefix: "abc",
Architecture: arch.AMD64,
Revision: 1,
Expand Down

0 comments on commit 14b4fc6

Please sign in to comment.