From e2f1b2d58c2002734a57f7f84e1c61899081bb11 Mon Sep 17 00:00:00 2001 From: gayurgin Date: Tue, 26 Nov 2024 17:01:48 +0300 Subject: [PATCH] [Disk Manager] return imageMeta/snapshotMeta by value in createImage/deleteImage methods of resources storage --- .../internal/pkg/resources/images.go | 41 +++++++++++-------- .../internal/pkg/resources/images_test.go | 20 ++++----- .../pkg/resources/mocks/storage_mock.go | 8 ++-- .../internal/pkg/resources/snapshots.go | 41 +++++++++++-------- .../internal/pkg/resources/snapshots_test.go | 20 ++++----- .../internal/pkg/resources/storage.go | 4 +- 6 files changed, 70 insertions(+), 64 deletions(-) diff --git a/cloud/disk_manager/internal/pkg/resources/images.go b/cloud/disk_manager/internal/pkg/resources/images.go index 871c4df293..d8c924f206 100644 --- a/cloud/disk_manager/internal/pkg/resources/images.go +++ b/cloud/disk_manager/internal/pkg/resources/images.go @@ -309,11 +309,13 @@ func (s *storageYDB) createImage( ctx context.Context, session *persistence.Session, image ImageMeta, -) (*ImageMeta, error) { +) (ImageMeta, error) { + + var emptyImageMeta ImageMeta tx, err := session.BeginRWTransaction(ctx) if err != nil { - return nil, err + return emptyImageMeta, err } defer tx.Rollback(ctx) @@ -321,16 +323,16 @@ func (s *storageYDB) createImage( // HACK: see NBS-974 for details. snapshotExists, err := s.snapshotExists(ctx, tx, image.ID) if err != nil { - return nil, err + return emptyImageMeta, err } if snapshotExists { err = tx.Commit(ctx) if err != nil { - return nil, err + return emptyImageMeta, err } - return nil, errors.NewNonCancellableErrorf( + return emptyImageMeta, errors.NewNonCancellableErrorf( "image with id %v can't be created, because snapshot with id %v already exists", image.ID, image.ID, @@ -339,7 +341,7 @@ func (s *storageYDB) createImage( createRequest, err := proto.Marshal(image.CreateRequest) if err != nil { - return nil, errors.NewNonRetriableErrorf( + return emptyImageMeta, errors.NewNonRetriableErrorf( "failed to marshal create request for image with id %v: %w", image.ID, err, @@ -358,27 +360,27 @@ func (s *storageYDB) createImage( persistence.ValueParam("$id", persistence.UTF8Value(image.ID)), ) if err != nil { - return nil, err + return emptyImageMeta, err } defer res.Close() states, err := scanImageStates(ctx, res) if err != nil { - return nil, err + return emptyImageMeta, err } if len(states) != 0 { err = tx.Commit(ctx) if err != nil { - return nil, err + return emptyImageMeta, err } state := states[0] if state.status >= imageStatusDeleting { logging.Info(ctx, "can't create already deleting/deleted image with id %v", image.ID) - return nil, errors.NewSilentNonRetriableErrorf( + return emptyImageMeta, errors.NewSilentNonRetriableErrorf( "can't create already deleting/deleted image with id %v", image.ID, ) @@ -389,10 +391,10 @@ func (s *storageYDB) createImage( state.createTaskID == image.CreateTaskID && state.createdBy == image.CreatedBy { - return state.toImageMeta(), nil + return *state.toImageMeta(), nil } - return nil, errors.NewNonCancellableErrorf( + return emptyImageMeta, errors.NewNonCancellableErrorf( "image with different params already exists, old=%v, new=%v", state, image, @@ -423,7 +425,10 @@ func (s *storageYDB) createImage( case nil: state.encryptionKeyHash = nil default: - return nil, errors.NewNonRetriableErrorf("unknown key %s", key) + return emptyImageMeta, errors.NewNonRetriableErrorf( + "unknown key %s", + key, + ) } } else { state.encryptionMode = uint32(types.EncryptionMode_NO_ENCRYPTION) @@ -442,15 +447,15 @@ func (s *storageYDB) createImage( persistence.ValueParam("$states", persistence.ListValue(state.structValue())), ) if err != nil { - return nil, err + return emptyImageMeta, err } err = tx.Commit(ctx) if err != nil { - return nil, err + return emptyImageMeta, err } - return state.toImageMeta(), nil + return *state.toImageMeta(), nil } func (s *storageYDB) imageCreated( @@ -832,9 +837,9 @@ func (s *storageYDB) listImages( func (s *storageYDB) CreateImage( ctx context.Context, image ImageMeta, -) (*ImageMeta, error) { +) (ImageMeta, error) { - var created *ImageMeta + var created ImageMeta err := s.db.Execute( ctx, diff --git a/cloud/disk_manager/internal/pkg/resources/images_test.go b/cloud/disk_manager/internal/pkg/resources/images_test.go index 3bd8944c63..c9eba630d6 100644 --- a/cloud/disk_manager/internal/pkg/resources/images_test.go +++ b/cloud/disk_manager/internal/pkg/resources/images_test.go @@ -55,12 +55,12 @@ func TestImagesCreateImage(t *testing.T) { created, err := storage.CreateImage(ctx, image) require.NoError(t, err) - require.NotNil(t, created) + require.Equal(t, image.ID, created.ID) // Check idempotency. created, err = storage.CreateImage(ctx, image) require.NoError(t, err) - require.NotNil(t, created) + require.Equal(t, image.ID, created.ID) err = storage.ImageCreated(ctx, image.ID, time.Now(), 0, 0) require.NoError(t, err) @@ -72,15 +72,14 @@ func TestImagesCreateImage(t *testing.T) { // Check idempotency. created, err = storage.CreateImage(ctx, image) require.NoError(t, err) - require.NotNil(t, created) + require.Equal(t, image.ID, created.ID) require.EqualValues(t, "disk", created.SrcDiskID) image.CreateTaskID = "other" - created, err = storage.CreateImage(ctx, image) + _, err = storage.CreateImage(ctx, image) require.Error(t, err) require.True(t, errors.Is(err, errors.NewEmptyNonCancellableError())) - require.Nil(t, created) } func TestImagesDeleteImage(t *testing.T) { @@ -106,7 +105,7 @@ func TestImagesDeleteImage(t *testing.T) { created, err := storage.CreateImage(ctx, image) require.NoError(t, err) - require.NotNil(t, created) + require.Equal(t, image.ID, created.ID) expected := image expected.CreateRequest = nil @@ -216,7 +215,7 @@ func TestImagesClearDeletedImages(t *testing.T) { created, err := storage.CreateImage(ctx, image) require.NoError(t, err) - require.NotNil(t, created) + require.Equal(t, image.ID, created.ID) _, err = storage.DeleteImage(ctx, image.ID, "delete", deletedAt) require.NoError(t, err) @@ -237,7 +236,7 @@ func TestImagesClearDeletedImages(t *testing.T) { created, err = storage.CreateImage(ctx, image) require.NoError(t, err) - require.NotNil(t, created) + require.Equal(t, image.ID, created.ID) } func TestImagesCreateImageShouldFailIfSnapshotAlreadyExists(t *testing.T) { @@ -264,10 +263,9 @@ func TestImagesCreateImageShouldFailIfSnapshotAlreadyExists(t *testing.T) { _, err = storage.CreateSnapshot(ctx, snapshot) require.NoError(t, err) - created, err := storage.CreateImage(ctx, ImageMeta{ID: snapshot.ID}) + _, err = storage.CreateImage(ctx, ImageMeta{ID: snapshot.ID}) require.Error(t, err) require.True(t, errors.Is(err, errors.NewEmptyNonCancellableError())) - require.Nil(t, created) } func TestImagesDeleteImageShouldFailIfSnapshotAlreadyExists(t *testing.T) { @@ -331,7 +329,7 @@ func TestImagesGetImage(t *testing.T) { created, err := storage.CreateImage(ctx, image) require.NoError(t, err) - require.NotNil(t, created) + require.Equal(t, image.ID, created.ID) image.CreateRequest = nil diff --git a/cloud/disk_manager/internal/pkg/resources/mocks/storage_mock.go b/cloud/disk_manager/internal/pkg/resources/mocks/storage_mock.go index 3a2fac5485..da0b07e179 100644 --- a/cloud/disk_manager/internal/pkg/resources/mocks/storage_mock.go +++ b/cloud/disk_manager/internal/pkg/resources/mocks/storage_mock.go @@ -88,10 +88,10 @@ func (s *StorageMock) ListDisks( func (s *StorageMock) CreateImage( ctx context.Context, image resources.ImageMeta, -) (*resources.ImageMeta, error) { +) (resources.ImageMeta, error) { args := s.Called(ctx, image) - return args.Get(0).(*resources.ImageMeta), args.Error(1) + return args.Get(0).(resources.ImageMeta), args.Error(1) } func (s *StorageMock) ImageCreated( @@ -161,10 +161,10 @@ func (s *StorageMock) ListImages( func (s *StorageMock) CreateSnapshot( ctx context.Context, snapshot resources.SnapshotMeta, -) (*resources.SnapshotMeta, error) { +) (resources.SnapshotMeta, error) { args := s.Called(ctx, snapshot) - return args.Get(0).(*resources.SnapshotMeta), args.Error(1) + return args.Get(0).(resources.SnapshotMeta), args.Error(1) } func (s *StorageMock) SnapshotCreated( diff --git a/cloud/disk_manager/internal/pkg/resources/snapshots.go b/cloud/disk_manager/internal/pkg/resources/snapshots.go index a5bb3a649b..047c871cc6 100644 --- a/cloud/disk_manager/internal/pkg/resources/snapshots.go +++ b/cloud/disk_manager/internal/pkg/resources/snapshots.go @@ -296,27 +296,29 @@ func (s *storageYDB) createSnapshot( ctx context.Context, session *persistence.Session, snapshot SnapshotMeta, -) (*SnapshotMeta, error) { +) (SnapshotMeta, error) { + + var emptySnapshotMeta SnapshotMeta tx, err := session.BeginRWTransaction(ctx) if err != nil { - return nil, err + return emptySnapshotMeta, err } defer tx.Rollback(ctx) // HACK: see NBS-974 for details. imageExists, err := s.imageExists(ctx, tx, snapshot.ID) if err != nil { - return nil, err + return emptySnapshotMeta, err } if imageExists { err = tx.Commit(ctx) if err != nil { - return nil, err + return emptySnapshotMeta, err } - return nil, errors.NewNonCancellableErrorf( + return emptySnapshotMeta, errors.NewNonCancellableErrorf( "snapshot with id %v can't be created, because image with id %v already exists", snapshot.ID, snapshot.ID, @@ -325,7 +327,7 @@ func (s *storageYDB) createSnapshot( createRequest, err := proto.Marshal(snapshot.CreateRequest) if err != nil { - return nil, errors.NewNonRetriableErrorf( + return emptySnapshotMeta, errors.NewNonRetriableErrorf( "failed to marshal create request for snapshot with id %v: %w", snapshot.ID, err, @@ -344,26 +346,26 @@ func (s *storageYDB) createSnapshot( persistence.ValueParam("$id", persistence.UTF8Value(snapshot.ID)), ) if err != nil { - return nil, err + return emptySnapshotMeta, err } defer res.Close() states, err := scanSnapshotStates(ctx, res) if err != nil { - return nil, err + return emptySnapshotMeta, err } if len(states) != 0 { err = tx.Commit(ctx) if err != nil { - return nil, err + return emptySnapshotMeta, err } state := states[0] if state.status >= snapshotStatusDeleting { logging.Info(ctx, "can't create already deleting/deleted snapshot with id %v", snapshot.ID) - return nil, errors.NewSilentNonRetriableErrorf( + return emptySnapshotMeta, errors.NewSilentNonRetriableErrorf( "can't create already deleting/deleted snapshot with id %v", snapshot.ID, ) @@ -374,10 +376,10 @@ func (s *storageYDB) createSnapshot( state.createTaskID == snapshot.CreateTaskID && state.createdBy == snapshot.CreatedBy { - return state.toSnapshotMeta(), nil + return *state.toSnapshotMeta(), nil } - return nil, errors.NewNonCancellableErrorf( + return emptySnapshotMeta, errors.NewNonCancellableErrorf( "snapshot with different params already exists, old=%v, new=%v", state, snapshot, @@ -408,7 +410,10 @@ func (s *storageYDB) createSnapshot( case nil: state.encryptionKeyHash = nil default: - return nil, errors.NewNonRetriableErrorf("unknown key %s", key) + return emptySnapshotMeta, errors.NewNonRetriableErrorf( + "unknown key %s", + key, + ) } } else { state.encryptionMode = uint32(types.EncryptionMode_NO_ENCRYPTION) @@ -427,15 +432,15 @@ func (s *storageYDB) createSnapshot( persistence.ValueParam("$states", persistence.ListValue(state.structValue())), ) if err != nil { - return nil, err + return emptySnapshotMeta, err } err = tx.Commit(ctx) if err != nil { - return nil, err + return emptySnapshotMeta, err } - return state.toSnapshotMeta(), nil + return *state.toSnapshotMeta(), nil } func (s *storageYDB) snapshotCreated( @@ -808,9 +813,9 @@ func (s *storageYDB) listSnapshots( func (s *storageYDB) CreateSnapshot( ctx context.Context, snapshot SnapshotMeta, -) (*SnapshotMeta, error) { +) (SnapshotMeta, error) { - var created *SnapshotMeta + var created SnapshotMeta err := s.db.Execute( ctx, diff --git a/cloud/disk_manager/internal/pkg/resources/snapshots_test.go b/cloud/disk_manager/internal/pkg/resources/snapshots_test.go index 1d4bcbc036..25247c7a62 100644 --- a/cloud/disk_manager/internal/pkg/resources/snapshots_test.go +++ b/cloud/disk_manager/internal/pkg/resources/snapshots_test.go @@ -61,12 +61,12 @@ func TestSnapshotsCreateSnapshot(t *testing.T) { created, err := storage.CreateSnapshot(ctx, snapshot) require.NoError(t, err) - require.NotNil(t, created) + require.Equal(t, snapshot.ID, created.ID) // Check idempotency. created, err = storage.CreateSnapshot(ctx, snapshot) require.NoError(t, err) - require.NotNil(t, created) + require.Equal(t, snapshot.ID, created.ID) err = storage.SnapshotCreated(ctx, snapshot.ID, time.Now(), 0, 0) require.NoError(t, err) @@ -78,13 +78,12 @@ func TestSnapshotsCreateSnapshot(t *testing.T) { // Check idempotency. created, err = storage.CreateSnapshot(ctx, snapshot) require.NoError(t, err) - require.NotNil(t, created) + require.Equal(t, snapshot.ID, created.ID) snapshot.CreateTaskID = "other" - created, err = storage.CreateSnapshot(ctx, snapshot) + _, err = storage.CreateSnapshot(ctx, snapshot) require.Error(t, err) require.True(t, errors.Is(err, errors.NewEmptyNonCancellableError())) - require.Nil(t, created) } func TestSnapshotsDeleteSnapshot(t *testing.T) { @@ -115,7 +114,7 @@ func TestSnapshotsDeleteSnapshot(t *testing.T) { created, err := storage.CreateSnapshot(ctx, snapshot) require.NoError(t, err) - require.NotNil(t, created) + require.Equal(t, snapshot.ID, created.ID) expected := snapshot expected.CreateRequest = nil @@ -231,7 +230,7 @@ func TestSnapshotsClearDeletedSnapshots(t *testing.T) { created, err := storage.CreateSnapshot(ctx, snapshot) require.NoError(t, err) - require.NotNil(t, created) + require.Equal(t, snapshot.ID, created.ID) _, err = storage.DeleteSnapshot(ctx, snapshot.ID, "delete", deletedAt) require.NoError(t, err) @@ -252,7 +251,7 @@ func TestSnapshotsClearDeletedSnapshots(t *testing.T) { created, err = storage.CreateSnapshot(ctx, snapshot) require.NoError(t, err) - require.NotNil(t, created) + require.Equal(t, snapshot.ID, created.ID) } func TestSnapshotsCreateSnapshotShouldFailIfImageAlreadyExists(t *testing.T) { @@ -275,10 +274,9 @@ func TestSnapshotsCreateSnapshotShouldFailIfImageAlreadyExists(t *testing.T) { _, err = storage.CreateImage(ctx, image) require.NoError(t, err) - created, err := storage.CreateSnapshot(ctx, SnapshotMeta{ID: image.ID}) + _, err = storage.CreateSnapshot(ctx, SnapshotMeta{ID: image.ID}) require.Error(t, err) require.True(t, errors.Is(err, errors.NewEmptyNonCancellableError())) - require.Nil(t, created) } func TestSnapshotsDeleteSnapshotShouldFailIfImageAlreadyExists(t *testing.T) { @@ -343,7 +341,7 @@ func TestSnapshotsGetSnapshot(t *testing.T) { created, err := storage.CreateSnapshot(ctx, snapshot) require.NoError(t, err) - require.NotNil(t, created) + require.Equal(t, snapshot.ID, created.ID) snapshot.CreateRequest = nil diff --git a/cloud/disk_manager/internal/pkg/resources/storage.go b/cloud/disk_manager/internal/pkg/resources/storage.go index 5754e32a12..69114cac1f 100644 --- a/cloud/disk_manager/internal/pkg/resources/storage.go +++ b/cloud/disk_manager/internal/pkg/resources/storage.go @@ -130,7 +130,7 @@ type Storage interface { ) ([]string, error) // Returns image if action has been accepted by storage and nil otherwise. - CreateImage(ctx context.Context, image ImageMeta) (*ImageMeta, error) + CreateImage(ctx context.Context, image ImageMeta) (ImageMeta, error) ImageCreated( ctx context.Context, @@ -163,7 +163,7 @@ type Storage interface { ) ([]string, error) // Returns snapshot if action has been accepted by storage and nil otherwise. - CreateSnapshot(ctx context.Context, snapshot SnapshotMeta) (*SnapshotMeta, error) + CreateSnapshot(ctx context.Context, snapshot SnapshotMeta) (SnapshotMeta, error) SnapshotCreated( ctx context.Context,