Skip to content

Commit 4345fc2

Browse files
BarkovBGBarkovBG
committed
[Disk Manager] Align ssd-nbs2 client with merged YDB NbsService API
DeletePartition now uses DiskId; create treats ALREADY_EXISTS as success and delete treats NOT_FOUND as success, matching the merged YDB contract.
1 parent 12abebd commit 4345fc2

9 files changed

Lines changed: 270 additions & 46 deletions

File tree

‎cloud/disk_manager/internal/pkg/clients/nbs2/client.go‎

Lines changed: 85 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,29 @@ import (
1818

1919
////////////////////////////////////////////////////////////////////////////////
2020

21-
// Ydb.StatusIds.StatusCode.SUCCESS
22-
const ydbStatusSuccess int32 = 400000
21+
// Ydb.StatusIds.StatusCode values used by Ydb.Nbs.V1.NbsService.
22+
const (
23+
ydbStatusSuccess int32 = 400000
24+
ydbStatusBadRequest int32 = 400010
25+
ydbStatusUnauthorized int32 = 400020
26+
ydbStatusInternalError int32 = 400030
27+
ydbStatusAborted int32 = 400040
28+
ydbStatusUnavailable int32 = 400050
29+
ydbStatusOverloaded int32 = 400060
30+
ydbStatusSchemeError int32 = 400070
31+
ydbStatusGenericError int32 = 400080
32+
ydbStatusTimeout int32 = 400090
33+
ydbStatusBadSession int32 = 400100
34+
ydbStatusPreconditionFailed int32 = 400120
35+
ydbStatusAlreadyExists int32 = 400130
36+
ydbStatusNotFound int32 = 400140
37+
ydbStatusSessionExpired int32 = 400150
38+
ydbStatusCancelled int32 = 400160
39+
ydbStatusUndetermined int32 = 400170
40+
ydbStatusUnsupported int32 = 400180
41+
ydbStatusSessionBusy int32 = 400190
42+
ydbStatusExternalError int32 = 400200
43+
)
2344

2445
type client struct {
2546
zoneID string
@@ -63,36 +84,36 @@ func (c *client) CreatePartition(
6384
return "", err
6485
}
6586

66-
op, err := checkOperation(resp.GetOperation())
87+
op, err := checkOperation(
88+
resp.GetOperation(),
89+
ydbStatusSuccess,
90+
ydbStatusAlreadyExists,
91+
)
6792
if err != nil {
6893
return "", err
6994
}
7095

7196
result := &nbs2_protos.CreatePartitionResult{}
72-
err = unpackOperationResult(op, result)
73-
if err != nil {
74-
return "", err
75-
}
76-
if len(result.GetTabletId()) == 0 {
77-
return "", errors.NewNonRetriableErrorf(
78-
"CreatePartition for disk %v returned empty tablet id",
79-
params.DiskID,
80-
)
97+
if op.GetResult() != nil && len(op.GetResult().GetValue()) > 0 {
98+
err = unpackOperationResult(op, result)
99+
if err != nil {
100+
return "", err
101+
}
81102
}
82103

83104
return result.GetTabletId(), nil
84105
}
85106

86-
func (c *client) DeletePartition(ctx context.Context, tabletID string) error {
87-
if len(tabletID) == 0 {
88-
return errors.NewNonRetriableErrorf("tablet id is required")
107+
func (c *client) DeletePartition(ctx context.Context, diskID string) error {
108+
if len(diskID) == 0 {
109+
return errors.NewNonRetriableErrorf("disk id is required")
89110
}
90111

91112
req := &nbs2_protos.DeletePartitionRequest{
92113
OperationParams: &nbs2_protos.OperationParams{
93114
OperationMode: nbs2_protos.OperationParams_SYNC,
94115
},
95-
TabletId: tabletID,
116+
DiskId: diskID,
96117
}
97118

98119
resp := &nbs2_protos.DeletePartitionResponse{}
@@ -101,7 +122,11 @@ func (c *client) DeletePartition(ctx context.Context, tabletID string) error {
101122
return err
102123
}
103124

104-
_, err = checkOperation(resp.GetOperation())
125+
_, err = checkOperation(
126+
resp.GetOperation(),
127+
ydbStatusSuccess,
128+
ydbStatusNotFound,
129+
)
105130
return err
106131
}
107132

@@ -147,7 +172,11 @@ func (c *client) invoke(
147172
return nil
148173
}
149174

150-
func checkOperation(op *nbs2_protos.Operation) (*nbs2_protos.Operation, error) {
175+
func checkOperation(
176+
op *nbs2_protos.Operation,
177+
okStatuses ...int32,
178+
) (*nbs2_protos.Operation, error) {
179+
151180
if op == nil {
152181
return nil, errors.NewRetriableErrorf("empty operation in nbs response")
153182
}
@@ -157,14 +186,45 @@ func checkOperation(op *nbs2_protos.Operation) (*nbs2_protos.Operation, error) {
157186
op.GetId(),
158187
)
159188
}
160-
if op.GetStatus() != ydbStatusSuccess {
161-
return nil, errors.NewRetriableErrorf(
162-
"nbs operation %v failed with status %v",
163-
op.GetId(),
164-
op.GetStatus(),
165-
)
189+
if len(okStatuses) == 0 {
190+
okStatuses = []int32{ydbStatusSuccess}
191+
}
192+
for _, status := range okStatuses {
193+
if op.GetStatus() == status {
194+
return op, nil
195+
}
196+
}
197+
return nil, operationStatusError(op)
198+
}
199+
200+
func operationStatusError(op *nbs2_protos.Operation) error {
201+
msg := fmt.Sprintf(
202+
"nbs operation %v failed with status %v",
203+
op.GetId(),
204+
op.GetStatus(),
205+
)
206+
if isRetriableYdbStatus(op.GetStatus()) {
207+
return errors.NewRetriableErrorf("%s", msg)
208+
}
209+
return errors.NewNonRetriableErrorf("%s", msg)
210+
}
211+
212+
func isRetriableYdbStatus(status int32) bool {
213+
switch status {
214+
case ydbStatusInternalError,
215+
ydbStatusAborted,
216+
ydbStatusUnavailable,
217+
ydbStatusOverloaded,
218+
ydbStatusTimeout,
219+
ydbStatusBadSession,
220+
ydbStatusSessionExpired,
221+
ydbStatusCancelled,
222+
ydbStatusUndetermined,
223+
ydbStatusSessionBusy:
224+
return true
225+
default:
226+
return false
166227
}
167-
return op, nil
168228
}
169229

170230
func unpackOperationResult(op *nbs2_protos.Operation, msg proto.Message) error {

‎cloud/disk_manager/internal/pkg/clients/nbs2/client_test.go‎

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"github.com/golang/protobuf/proto"
77
"github.com/stretchr/testify/require"
88
nbs2_protos "github.com/ydb-platform/nbs/cloud/disk_manager/internal/pkg/clients/nbs2/protos"
9+
"github.com/ydb-platform/nbs/cloud/tasks/errors"
910
"google.golang.org/protobuf/types/known/anypb"
1011
)
1112

@@ -36,9 +37,48 @@ func TestCheckOperationFailedStatus(t *testing.T) {
3637
_, err := checkOperation(&nbs2_protos.Operation{
3738
Id: "op",
3839
Ready: true,
39-
Status: 400030, // INTERNAL_ERROR
40+
Status: ydbStatusInternalError,
4041
})
4142
require.Error(t, err)
43+
require.True(t, errors.CanRetry(err))
44+
}
45+
46+
func TestCheckOperationCreateAlreadyExists(t *testing.T) {
47+
op, err := checkOperation(
48+
&nbs2_protos.Operation{
49+
Id: "op",
50+
Ready: true,
51+
Status: ydbStatusAlreadyExists,
52+
},
53+
ydbStatusSuccess,
54+
ydbStatusAlreadyExists,
55+
)
56+
require.NoError(t, err)
57+
require.Equal(t, "op", op.GetId())
58+
}
59+
60+
func TestCheckOperationDeleteNotFound(t *testing.T) {
61+
op, err := checkOperation(
62+
&nbs2_protos.Operation{
63+
Id: "op",
64+
Ready: true,
65+
Status: ydbStatusNotFound,
66+
},
67+
ydbStatusSuccess,
68+
ydbStatusNotFound,
69+
)
70+
require.NoError(t, err)
71+
require.Equal(t, "op", op.GetId())
72+
}
73+
74+
func TestCheckOperationPermanentStatus(t *testing.T) {
75+
_, err := checkOperation(&nbs2_protos.Operation{
76+
Id: "op",
77+
Ready: true,
78+
Status: ydbStatusGenericError,
79+
})
80+
require.Error(t, err)
81+
require.False(t, errors.CanRetry(err))
4282
}
4383

4484
func TestNormalizeEndpoint(t *testing.T) {

‎cloud/disk_manager/internal/pkg/clients/nbs2/interface.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ type CreatePartitionParams struct {
1515

1616
type Client interface {
1717
CreatePartition(ctx context.Context, params CreatePartitionParams) (tabletID string, err error)
18-
DeletePartition(ctx context.Context, tabletID string) error
18+
DeletePartition(ctx context.Context, diskID string) error
1919
ZoneID() string
2020
}
2121

‎cloud/disk_manager/internal/pkg/clients/nbs2/mocks/client_mock.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,8 @@ func (c *ClientMock) CreatePartition(
2222
return args.String(0), args.Error(1)
2323
}
2424

25-
func (c *ClientMock) DeletePartition(ctx context.Context, tabletID string) error {
26-
args := c.Called(ctx, tabletID)
25+
func (c *ClientMock) DeletePartition(ctx context.Context, diskID string) error {
26+
args := c.Called(ctx, diskID)
2727
return args.Error(0)
2828
}
2929

‎cloud/disk_manager/internal/pkg/clients/nbs2/protos/ydb_nbs.proto‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,13 @@ message CreatePartitionResult {
5656

5757
message DeletePartitionRequest {
5858
OperationParams operation_params = 1;
59-
string TabletId = 2;
59+
string DiskId = 2;
6060
}
6161

6262
message DeletePartitionResponse {
6363
Operation operation = 1;
6464
}
6565

6666
message DeletePartitionResult {
67-
string TabletId = 1;
67+
string DiskId = 1;
6868
}

‎cloud/disk_manager/internal/pkg/services/disks/create_empty_disk_task.go‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -214,17 +214,13 @@ func (t *createEmptyDiskTask) Cancel(
214214
if common.IsNbs2DiskKind(t.params.Kind) ||
215215
common.IsNbs2DiskKindString(diskMeta.Kind) {
216216

217-
tabletID := t.state.GetTabletId()
218-
if len(diskMeta.TabletID) > 0 {
219-
tabletID = diskMeta.TabletID
220-
}
221-
if len(tabletID) > 0 && t.nbs2Factory != nil {
217+
if t.nbs2Factory != nil {
222218
client, err := t.nbs2Factory.GetClient(ctx, diskMeta.ZoneID)
223219
if err != nil {
224220
return err
225221
}
226222

227-
err = client.DeletePartition(ctx, tabletID)
223+
err = client.DeletePartition(ctx, diskMeta.ID)
228224
if err != nil {
229225
return err
230226
}

‎cloud/disk_manager/internal/pkg/services/disks/create_empty_disk_task_test.go‎

Lines changed: 98 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -726,6 +726,61 @@ func TestCreateEmptyNbs2DiskTask(t *testing.T) {
726726
)
727727
}
728728

729+
func TestCreateEmptyNbs2DiskTaskEmptyTabletId(t *testing.T) {
730+
ctx := context.Background()
731+
storage := storage_mocks.NewStorageMock()
732+
nbs2Factory := nbs2_mocks.NewFactoryMock()
733+
nbs2Client := nbs2_mocks.NewClientMock()
734+
execCtx := newExecutionContextMock()
735+
736+
params := &protos.CreateDiskParams{
737+
BlocksCount: 123,
738+
BlockSize: 4096,
739+
Kind: types.DiskKind_DISK_KIND_SSD_NBS2,
740+
StoragePoolName: "ddp1",
741+
CloudId: "cloud",
742+
FolderId: "folder",
743+
Disk: &types.Disk{
744+
ZoneId: "zone",
745+
DiskId: "disk",
746+
},
747+
}
748+
task := &createEmptyDiskTask{
749+
storage: storage,
750+
nbs2Factory: nbs2Factory,
751+
params: params,
752+
state: &protos.CreateEmptyDiskTaskState{},
753+
}
754+
755+
storage.On("CreateDisk", ctx, mock.Anything).Return(&resources.DiskMeta{
756+
ID: "disk",
757+
}, nil)
758+
storage.On("DiskCreated", ctx, mock.MatchedBy(func(meta resources.DiskMeta) bool {
759+
return meta.ID == "disk" && meta.TabletID == ""
760+
})).Return(nil)
761+
762+
nbs2Factory.On("GetClient", ctx, "zone").Return(nbs2Client, nil)
763+
nbs2Client.On("CreatePartition", ctx, nbs2.CreatePartitionParams{
764+
DiskID: "disk",
765+
BlockSize: 4096,
766+
BlocksCount: 123,
767+
StoragePoolName: "ddp1",
768+
}).Return("", nil)
769+
770+
execCtx.On("SaveState", ctx).Return(nil)
771+
772+
err := task.Run(ctx, execCtx)
773+
require.NoError(t, err)
774+
require.Empty(t, task.state.TabletId)
775+
mock.AssertExpectationsForObjects(
776+
t,
777+
storage,
778+
nbs2Factory,
779+
nbs2Client,
780+
execCtx,
781+
)
782+
}
783+
729784
func TestCancelCreateEmptyNbs2DiskTask(t *testing.T) {
730785
ctx := context.Background()
731786
storage := storage_mocks.NewStorageMock()
@@ -762,7 +817,49 @@ func TestCancelCreateEmptyNbs2DiskTask(t *testing.T) {
762817
storage.On("DiskDeleted", ctx, "disk", mock.Anything).Return(nil)
763818

764819
nbs2Factory.On("GetClient", ctx, "zone").Return(nbs2Client, nil)
765-
nbs2Client.On("DeletePartition", ctx, "tablet-1").Return(nil)
820+
nbs2Client.On("DeletePartition", ctx, "disk").Return(nil)
821+
822+
err := task.Cancel(ctx, execCtx)
823+
require.NoError(t, err)
824+
mock.AssertExpectationsForObjects(t, storage, nbs2Factory, nbs2Client, execCtx)
825+
}
826+
827+
func TestCancelCreateEmptyNbs2DiskTaskWithoutTabletId(t *testing.T) {
828+
ctx := context.Background()
829+
storage := storage_mocks.NewStorageMock()
830+
nbs2Factory := nbs2_mocks.NewFactoryMock()
831+
nbs2Client := nbs2_mocks.NewClientMock()
832+
execCtx := newExecutionContextMock()
833+
834+
params := &protos.CreateDiskParams{
835+
Kind: types.DiskKind_DISK_KIND_SSD_NBS2,
836+
Disk: &types.Disk{
837+
ZoneId: "zone",
838+
DiskId: "disk",
839+
},
840+
}
841+
task := &createEmptyDiskTask{
842+
storage: storage,
843+
nbs2Factory: nbs2Factory,
844+
params: params,
845+
state: &protos.CreateEmptyDiskTaskState{},
846+
}
847+
848+
storage.On(
849+
"DeleteDisk",
850+
ctx,
851+
"disk",
852+
"toplevel_task_id",
853+
mock.Anything,
854+
).Return(&resources.DiskMeta{
855+
ID: "disk",
856+
ZoneID: "zone",
857+
Kind: "ssd-nbs2",
858+
}, nil)
859+
storage.On("DiskDeleted", ctx, "disk", mock.Anything).Return(nil)
860+
861+
nbs2Factory.On("GetClient", ctx, "zone").Return(nbs2Client, nil)
862+
nbs2Client.On("DeletePartition", ctx, "disk").Return(nil)
766863

767864
err := task.Cancel(ctx, execCtx)
768865
require.NoError(t, err)

0 commit comments

Comments
 (0)