fix(cluster_family): Cancel slot migration from incoming node on OOM #5000
Conversation
mkaruza
force-pushed
the
mkaruza/github#4977
branch
from
9c88c11 to
6a5d621
Compare
mkaruza
changed the title
src/server/cluster/cluster_family.cc
Outdated
| if (migration->GetState() == MigrationState::C_FATAL) { | ||
| migration->Stop(); | ||
| } |
There was a problem hiding this comment.
looks like incorrect place for this logic
There was a problem hiding this comment.
If FLOW fails with C_FATAL we'll call it, where would you put migration->Stop to handle stopping of migration?
There was a problem hiding this comment.
Maybe we can move it to reportError or SetState, where we set the fatal status
| @@ -70,6 +76,20 @@ class ClusterShardMigration { | |||
| break; | |||
| } | |||
|
|
|||
| auto oom_check = [&]() -> bool { | |||
| auto used_mem = used_mem_current.load(memory_order_relaxed); | |||
| if ((used_mem + tx_data->command.cmd_len) > max_memory_limit) { | |||
There was a problem hiding this comment.
is used_mem RSS? maybe we need max_memory_limit - 100MB for example, or how we can guarantee 100% success of this condition?
There was a problem hiding this comment.
Yes, it is checks whatever is set with maxmemory. I'll modify so it uses 90% of max memory as upper bound. That should be safe i think.
There was a problem hiding this comment.
Let's discuss it with @adiholden or @romange
| void ReportFatalError(dfly::GenericError err) ABSL_LOCKS_EXCLUDED(state_mu_, error_mu_) { | ||
| errors_count_.fetch_add(1, std::memory_order_relaxed); | ||
| util::fb2::LockGuard lk_state(state_mu_); | ||
| util::fb2::LockGuard lk_error(error_mu_); | ||
| state_ = MigrationState::C_FATAL; | ||
| last_error_ = std::move(err); | ||
| } |
There was a problem hiding this comment.
I don't like this code. Let's think how to make it better. Maybe we can use state_mu_ for error too or merge this method with reportError
There was a problem hiding this comment.
Agree, but i changed ReportError to check GetState() != C_FATAL and i wanted to update state & set error message while having both locks.
mkaruza
force-pushed
the
mkaruza/github#4977
branch
from
e9049f0 to
16a27f6
Compare
If applying command on incoming node will result in OOM (we overflow max_memory_limit) we are closing migration and switch state to FATAL. Signed-off-by: mkaruza <[email protected]>
mkaruza
force-pushed
the
mkaruza/github#4977
branch
from
16a27f6 to
f080a52
Compare
mkaruza
force-pushed
the
mkaruza/github#4977
branch
from
7e7ebf4 to
40851f9
Compare
| @@ -925,7 +926,7 @@ void ClusterFamily::InitMigration(CmdArgList args, SinkReplyBuilder* builder) { | |||
|
|
|||
| if (!migration) { | |||
| VLOG(1) << "Unrecognized incoming migration from " << source_id; | |||
| return builder->SendSimpleString(OutgoingMigration::kUnknownMigration); | |||
| return builder->SendSimpleString(kUnknownMigration); | |||
There was a problem hiding this comment.
I think it should be error not string, like below
There was a problem hiding this comment.
Part of code that was handling kUnknownMigration is using both SendError and SendSimpleString. This PR didn't change that but it moved this constant into different location. kUnknownMigration will be matched only if SendSimpleString is used.
| } | ||
|
|
||
| auto migration = GetIncomingMigration(source_id); | ||
| if (!migration) | ||
| return builder->SendError(kIdNotFound); | ||
|
|
||
| if (!migration->Join(attempt)) { | ||
| return builder->SendError("Join timeout happened"); | ||
| if (migration->GetState() == MigrationState::C_FATAL) { | ||
| return builder->SendError(absl::StrCat("-", kIncomingMigrationOOM)); |
There was a problem hiding this comment.
why do we need "-", maybe we can make it as part of the kIncomingMigrationOOM
There was a problem hiding this comment.
If - is not provided in error string response would be -ERR INCOMING_MIGRATION_OOM which will not be matched. If we use - as part of constant we would need to remove - prefix when matching error response.
| // Break outgoing migration if INIT from incoming node responded with OOM. Usually this will | ||
| // happen on second iteration after first failed with OOM. Sending second INIT is required to | ||
| // cleanup slots on incoming slot migration node. | ||
| if (CheckRespSimpleError(kIncomingMigrationOOM)) { | ||
| ChangeState(MigrationState::C_FATAL); | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think you can move it in next if if (!CheckRespIsSimpleReply("OK")) {
There was a problem hiding this comment.
CheckRespIsSimpleReply expects that response is STRING (+) while we are returning now ERROR (-)
If applying command on incoming node will result in OOM (we overflow
max_memory_limit) we are closing migration and switch state to FATAL.