Skip to content

Commit 3b744b5

Browse files
ydbotydbot
authored
[Backport stable-25-3-1] PR #31062: Fix sole command rollback (#31165)
2 parents 91787ce + 260c0da commit 3b744b5

File tree

2 files changed

+188
-33
lines changed

2 files changed

+188
-33
lines changed

‎ydb/core/mind/bscontroller/config_cmd.cpp‎

Lines changed: 43 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,13 @@ namespace NKikimr::NBsController {
5353
}
5454
}
5555

56+
void Rollback() {
57+
Y_ENSURE(Success);
58+
Success = false;
59+
RollbackSuccess = true;
60+
Error = "transaction rollback";
61+
}
62+
5663
void Finish() {
5764
Response->SetSuccess(Success);
5865
Response->SetRollbackSuccess(RollbackSuccess);
@@ -61,32 +68,45 @@ namespace NKikimr::NBsController {
6168
}
6269
}
6370

64-
bool ExecuteSoleCommand(const NKikimrBlobStorage::TConfigRequest::TCommand& cmd, TTransactionContext& txc) {
71+
bool IsSoleCommand(const NKikimrBlobStorage::TConfigRequest::TCommand& cmd) {
72+
switch (cmd.GetCommandCase()) {
73+
case NKikimrBlobStorage::TConfigRequest::TCommand::kEnableSelfHeal:
74+
case NKikimrBlobStorage::TConfigRequest::TCommand::kEnableDonorMode:
75+
case NKikimrBlobStorage::TConfigRequest::TCommand::kSetScrubPeriodicity:
76+
case NKikimrBlobStorage::TConfigRequest::TCommand::kSetPDiskSpaceMarginPromille:
77+
case NKikimrBlobStorage::TConfigRequest::TCommand::kUpdateSettings:
78+
return true;
79+
default:
80+
return false;
81+
}
82+
}
83+
84+
void ExecuteSoleCommand(const NKikimrBlobStorage::TConfigRequest::TCommand& cmd, TTransactionContext& txc) {
6585
NIceDb::TNiceDb db(txc.DB);
6686
switch (cmd.GetCommandCase()) {
6787
case NKikimrBlobStorage::TConfigRequest::TCommand::kEnableSelfHeal:
6888
Self->SelfHealEnable = cmd.GetEnableSelfHeal().GetEnable();
6989
db.Table<Schema::State>().Key(true).Update<Schema::State::SelfHealEnable>(Self->SelfHealEnable);
70-
return true;
90+
return;
7191

7292
case NKikimrBlobStorage::TConfigRequest::TCommand::kEnableDonorMode:
7393
Self->DonorMode = cmd.GetEnableDonorMode().GetEnable();
7494
db.Table<Schema::State>().Key(true).Update<Schema::State::DonorModeEnable>(Self->DonorMode);
75-
return true;
95+
return;
7696

7797
case NKikimrBlobStorage::TConfigRequest::TCommand::kSetScrubPeriodicity: {
7898
const ui32 seconds = cmd.GetSetScrubPeriodicity().GetScrubPeriodicity();
7999
Self->ScrubPeriodicity = TDuration::Seconds(seconds);
80100
db.Table<Schema::State>().Key(true).Update<Schema::State::ScrubPeriodicity>(seconds);
81101
Self->ScrubState.OnScrubPeriodicityChange();
82-
return true;
102+
return;
83103
}
84104

85105
case NKikimrBlobStorage::TConfigRequest::TCommand::kSetPDiskSpaceMarginPromille: {
86106
const ui32 value = cmd.GetSetPDiskSpaceMarginPromille().GetPDiskSpaceMarginPromille();
87107
Self->PDiskSpaceMarginPromille = value;
88108
db.Table<Schema::State>().Key(true).Update<Schema::State::PDiskSpaceMarginPromille>(value);
89-
return true;
109+
return;
90110
}
91111

92112
case NKikimrBlobStorage::TConfigRequest::TCommand::kUpdateSettings: {
@@ -157,11 +177,11 @@ namespace NKikimr::NBsController {
157177
Self->TryToRelocateBrokenDisksLocallyFirst = value;
158178
db.Table<T>().Key(true).Update<T::TryToRelocateBrokenDisksLocallyFirst>(Self->TryToRelocateBrokenDisksLocallyFirst);
159179
}
160-
return true;
180+
return;
161181
}
162182

163183
default:
164-
return false;
184+
throw TExError() << "unsupported sole command " << static_cast<int>(cmd.GetCommandCase());
165185
}
166186
}
167187

@@ -170,18 +190,17 @@ namespace NKikimr::NBsController {
170190
THPTimer timer;
171191

172192
// check if there is some special sole command
173-
if (Cmd.CommandSize() == 1) {
174-
bool res = true;
193+
if (Cmd.CommandSize() == 1 && IsSoleCommand(Cmd.GetCommand(0))) {
175194
WrapCommand([&] {
176-
res = ExecuteSoleCommand(Cmd.GetCommand(0), txc);
195+
if (Cmd.GetRollback()) {
196+
Rollback();
197+
} else {
198+
ExecuteSoleCommand(Cmd.GetCommand(0), txc);
199+
}
177200
});
178-
if (res) {
179-
Finish();
180-
LogCommand(txc, TDuration::Seconds(timer.Passed()));
181-
return true;
182-
}
183-
Y_ABORT_UNLESS(Success);
184-
Response->MutableStatus()->RemoveLast();
201+
Finish();
202+
LogCommand(txc, TDuration::Seconds(timer.Passed()));
203+
return true;
185204
}
186205

187206
const auto& hostRecords = EnforceHostRecords ? *EnforceHostRecords : Self->HostRecords;
@@ -266,9 +285,7 @@ namespace NKikimr::NBsController {
266285
}
267286

268287
if (Success && Cmd.GetRollback()) {
269-
Success = false;
270-
RollbackSuccess = true;
271-
Error = "transaction rollback";
288+
Rollback();
272289
}
273290

274291
if (Success && SelfHeal && !Self->SelfHealEnable) {
@@ -372,21 +389,14 @@ namespace NKikimr::NBsController {
372389
HANDLE_COMMAND(MovePDisk)
373390
HANDLE_COMMAND(UpdateBridgeGroupInfo)
374391
HANDLE_COMMAND(ReconfigureVirtualGroup)
375-
376-
case NKikimrBlobStorage::TConfigRequest::TCommand::kAddMigrationPlan:
377-
case NKikimrBlobStorage::TConfigRequest::TCommand::kDeleteMigrationPlan:
378-
case NKikimrBlobStorage::TConfigRequest::TCommand::kDeclareIntent:
379-
case NKikimrBlobStorage::TConfigRequest::TCommand::kReadIntent:
380-
case NKikimrBlobStorage::TConfigRequest::TCommand::kEnableSelfHeal:
381-
case NKikimrBlobStorage::TConfigRequest::TCommand::kEnableDonorMode:
382-
case NKikimrBlobStorage::TConfigRequest::TCommand::kSetScrubPeriodicity:
383-
case NKikimrBlobStorage::TConfigRequest::TCommand::kSetPDiskSpaceMarginPromille:
384-
case NKikimrBlobStorage::TConfigRequest::TCommand::kUpdateSettings:
385-
case NKikimrBlobStorage::TConfigRequest::TCommand::COMMAND_NOT_SET:
386-
throw TExError() << "unsupported command";
392+
default: break;
387393
}
388394

389-
throw TExError() << "unsupported command";
395+
if (IsSoleCommand(cmd)) {
396+
throw TExError() << "command must be sole";
397+
} else {
398+
throw TExError() << "unsupported command " << static_cast<int>(cmd.GetCommandCase());
399+
}
390400
}
391401

392402
void Complete(const TActorContext&) override {

‎ydb/core/mind/bscontroller/ut_bscontroller/main.cpp‎

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1233,4 +1233,149 @@ Y_UNIT_TEST_SUITE(BsControllerConfig) {
12331233
}
12341234
}
12351235
}
1236+
1237+
Y_UNIT_TEST(CommandRollbackWhenAlone) {
1238+
TEnvironmentSetup env(1, 1);
1239+
RunTestWithReboots(env.TabletIds, [&] { return env.PrepareInitialEventsFilter(); }, [&](const TString& dispatchName, std::function<void(TTestActorRuntime&)> setup, bool& outActiveZone) {
1240+
TFinalizer finalizer(env);
1241+
env.Prepare(dispatchName, setup, outActiveZone);
1242+
1243+
NKikimrBlobStorage::TConfigRequest request;
1244+
auto* cmd = request.AddCommand()->MutableDefineHostConfig();
1245+
cmd->SetHostConfigId(1);
1246+
cmd->SetName("TestCommandRollbackWhenAlone");
1247+
request.SetRollback(true);
1248+
1249+
NKikimrBlobStorage::TConfigResponse response = env.Invoke(request);
1250+
Cerr << (TStringBuilder() << response.DebugString() << Endl);
1251+
1252+
UNIT_ASSERT(!response.GetSuccess());
1253+
UNIT_ASSERT(response.GetRollbackSuccess());
1254+
UNIT_ASSERT_VALUES_EQUAL(response.GetErrorDescription(), "transaction rollback");
1255+
1256+
UNIT_ASSERT_VALUES_EQUAL(response.StatusSize(), 1);
1257+
UNIT_ASSERT(response.GetStatus(0).GetSuccess());
1258+
});
1259+
}
1260+
1261+
Y_UNIT_TEST(CommandRollbackWhenCombined) {
1262+
TEnvironmentSetup env(1, 1);
1263+
RunTestWithReboots(env.TabletIds, [&] { return env.PrepareInitialEventsFilter(); }, [&](const TString& dispatchName, std::function<void(TTestActorRuntime&)> setup, bool& outActiveZone) {
1264+
TFinalizer finalizer(env);
1265+
env.Prepare(dispatchName, setup, outActiveZone);
1266+
1267+
NKikimrBlobStorage::TConfigRequest request1;
1268+
request1.AddCommand()->MutableReadHostConfig();
1269+
auto* cmd = request1.AddCommand()->MutableDefineHostConfig();
1270+
cmd->SetHostConfigId(1);
1271+
cmd->SetName("TestCommandRollbackWhenCombined");
1272+
request1.AddCommand()->MutableReadHostConfig();
1273+
request1.SetRollback(true);
1274+
1275+
NKikimrBlobStorage::TConfigResponse response1 = env.Invoke(request1);
1276+
Cerr << (TStringBuilder() << response1.DebugString() << Endl);
1277+
1278+
UNIT_ASSERT(!response1.GetSuccess());
1279+
UNIT_ASSERT(response1.GetRollbackSuccess());
1280+
UNIT_ASSERT_VALUES_EQUAL(response1.GetErrorDescription(), "transaction rollback");
1281+
UNIT_ASSERT_VALUES_EQUAL(response1.StatusSize(), 3);
1282+
1283+
UNIT_ASSERT(response1.GetStatus(0).GetSuccess());
1284+
UNIT_ASSERT_VALUES_EQUAL(response1.GetStatus(0).HostConfigSize(), 0);
1285+
1286+
UNIT_ASSERT(response1.GetStatus(1).GetSuccess());
1287+
1288+
UNIT_ASSERT(response1.GetStatus(2).GetSuccess());
1289+
UNIT_ASSERT_VALUES_EQUAL(response1.GetStatus(2).HostConfigSize(), 1);
1290+
1291+
NKikimrBlobStorage::TConfigRequest request2;
1292+
request2.AddCommand()->MutableReadHostConfig();
1293+
NKikimrBlobStorage::TConfigResponse response2 = env.Invoke(request2);
1294+
Cerr << (TStringBuilder() << response2.DebugString() << Endl);
1295+
UNIT_ASSERT(response2.GetSuccess());
1296+
UNIT_ASSERT_VALUES_EQUAL(response2.StatusSize(), 1);
1297+
UNIT_ASSERT(response2.GetStatus(0).GetSuccess());
1298+
UNIT_ASSERT_VALUES_EQUAL(response2.GetStatus(0).HostConfigSize(), 0);
1299+
});
1300+
}
1301+
1302+
Y_UNIT_TEST(SoleCommandRollback) {
1303+
TEnvironmentSetup env(1, 1);
1304+
RunTestWithReboots(env.TabletIds, [&] { return env.PrepareInitialEventsFilter(); }, [&](const TString& dispatchName, std::function<void(TTestActorRuntime&)> setup, bool& outActiveZone) {
1305+
TFinalizer finalizer(env);
1306+
env.Prepare(dispatchName, setup, outActiveZone);
1307+
1308+
using TColor = NKikimrBlobStorage::TPDiskSpaceColor;
1309+
auto updateSettings = [&env](TColor::E colorBorder, bool rollback = false) {
1310+
NKikimrBlobStorage::TConfigRequest request;
1311+
auto* us = request.AddCommand()->MutableUpdateSettings();
1312+
us->AddPDiskSpaceColorBorder(colorBorder);
1313+
request.SetRollback(rollback);
1314+
return env.Invoke(request);
1315+
};
1316+
1317+
NKikimrBlobStorage::TConfigResponse response1 = updateSettings(TColor::CYAN);
1318+
Cerr << (TStringBuilder() << response1.DebugString() << Endl);
1319+
UNIT_ASSERT_C(response1.GetSuccess(), response1.GetErrorDescription());
1320+
1321+
NKikimrBlobStorage::TConfigResponse response2 = updateSettings(TColor::YELLOW, true);
1322+
Cerr << (TStringBuilder() << response2.DebugString() << Endl);
1323+
UNIT_ASSERT(!response2.GetSuccess());
1324+
UNIT_ASSERT(response2.GetRollbackSuccess());
1325+
UNIT_ASSERT_VALUES_EQUAL(response2.GetErrorDescription(), "transaction rollback");
1326+
UNIT_ASSERT_VALUES_EQUAL(response2.StatusSize(), 1);
1327+
UNIT_ASSERT(response2.GetStatus(0).GetSuccess());
1328+
1329+
NKikimrBlobStorage::TConfigRequest request3;
1330+
request3.AddCommand()->MutableQueryBaseConfig();
1331+
NKikimrBlobStorage::TConfigResponse response3 = env.Invoke(request3);
1332+
Cerr << (TStringBuilder() << response3.DebugString() << Endl);
1333+
UNIT_ASSERT(response3.GetSuccess());
1334+
UNIT_ASSERT_VALUES_EQUAL(response3.StatusSize(), 1);
1335+
auto baseConfig = response3.GetStatus(0).GetBaseConfig();
1336+
UNIT_ASSERT_VALUES_EQUAL(baseConfig.GetSettings().GetPDiskSpaceColorBorder(0), TColor::CYAN);
1337+
});
1338+
}
1339+
1340+
Y_UNIT_TEST(SoleCommandErrorWhenCombined) {
1341+
TEnvironmentSetup env(1, 1);
1342+
RunTestWithReboots(env.TabletIds, [&] { return env.PrepareInitialEventsFilter(); }, [&](const TString& dispatchName, std::function<void(TTestActorRuntime&)> setup, bool& outActiveZone) {
1343+
TFinalizer finalizer(env);
1344+
env.Prepare(dispatchName, setup, outActiveZone);
1345+
1346+
NKikimrBlobStorage::TConfigRequest request;
1347+
request.AddCommand()->MutableDefineHostConfig();
1348+
request.AddCommand()->MutableEnableSelfHeal();
1349+
request.AddCommand()->MutableQueryBaseConfig();
1350+
NKikimrBlobStorage::TConfigResponse response = env.Invoke(request);
1351+
Cerr << (TStringBuilder() << response.DebugString() << Endl);
1352+
UNIT_ASSERT(!response.GetSuccess());
1353+
UNIT_ASSERT_VALUES_EQUAL(response.GetErrorDescription(), "command must be sole");
1354+
UNIT_ASSERT_VALUES_EQUAL(response.StatusSize(), 2);
1355+
1356+
UNIT_ASSERT(response.GetStatus(0).GetSuccess());
1357+
1358+
UNIT_ASSERT(!response.GetStatus(1).GetSuccess());
1359+
UNIT_ASSERT_VALUES_EQUAL(response.GetStatus(1).GetErrorDescription(), "command must be sole");
1360+
});
1361+
}
1362+
1363+
Y_UNIT_TEST(UnsupportedCommandError) {
1364+
TEnvironmentSetup env(1, 1);
1365+
RunTestWithReboots(env.TabletIds, [&] { return env.PrepareInitialEventsFilter(); }, [&](const TString& dispatchName, std::function<void(TTestActorRuntime&)> setup, bool& outActiveZone) {
1366+
TFinalizer finalizer(env);
1367+
env.Prepare(dispatchName, setup, outActiveZone);
1368+
1369+
NKikimrBlobStorage::TConfigRequest request;
1370+
request.AddCommand();
1371+
NKikimrBlobStorage::TConfigResponse response = env.Invoke(request);
1372+
Cerr << (TStringBuilder() << response.DebugString() << Endl);
1373+
UNIT_ASSERT(!response.GetSuccess());
1374+
UNIT_ASSERT_VALUES_EQUAL(response.GetErrorDescription(), "unsupported command 0");
1375+
UNIT_ASSERT_VALUES_EQUAL(response.StatusSize(), 1);
1376+
1377+
UNIT_ASSERT(!response.GetStatus(0).GetSuccess());
1378+
UNIT_ASSERT_VALUES_EQUAL(response.GetStatus(0).GetErrorDescription(), "unsupported command 0");
1379+
});
1380+
}
12361381
}

0 commit comments

Comments
 (0)