Add manual request approval to CMS #21593
Conversation
SammyVimes
requested review from
Copilot
and removed request for
stanislav-shchetinin
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
SammyVimes
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🟢 |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces manual approval functionality for CMS (Cluster Management Service) requests, allowing administrators to approve previously rejected scheduled requests and receive permissions even when normal safety checks would prevent approval.
- Adds a new
APPROVEaction type to the CMS management API - Implements CLI command support for approving scheduled requests
- Provides manual override capability for requests that fail automatic approval due to availability constraints
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ydb/core/protos/cms.proto | Adds APPROVE enum value and ManuallyApprovedPermissions field to protobuf definitions |
| ydb/core/driver_lib/cli_utils/cli_cmds_cms.cpp | Implements CLI command class for approving requests |
| ydb/core/cms/cms_ut_common.h | Adds test helper method declaration for approval testing |
| ydb/core/cms/cms_ut_common.cpp | Implements test helper method for approval operations |
| ydb/core/cms/cms_ut.cpp | Adds comprehensive unit tests for manual approval scenarios |
| ydb/core/cms/cms_impl.h | Declares ManuallyApproveRequest method in CMS implementation |
| ydb/core/cms/cms.cpp | Implements core manual approval logic with dedicated actor |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
(cherry picked from commit 8b7713d)
(cherry picked from commit 8b7713d)
| {} | ||
|
|
||
| void Handle(TEvCms::TEvPermissionResponse::TPtr &ev) { | ||
| auto resp = ev->Get(); |
There was a problem hiding this comment.
| auto resp = ev->Get(); | |
| const auto& resp = ev->Get(); |
|
|
||
| const NKikimrCms::TStatus status = resp->Record.GetStatus(); | ||
|
|
||
| THolder<TEvCms::TEvManageRequestResponse> manageResponse = MakeHolder<TEvCms::TEvManageRequestResponse>(); |
There was a problem hiding this comment.
| THolder<TEvCms::TEvManageRequestResponse> manageResponse = MakeHolder<TEvCms::TEvManageRequestResponse>(); | |
| auto manageResponse = MakeHolder<TEvCms::TEvManageRequestResponse>(); |
Не нужно повторять, а читаемость только улучшается.
| ev, TStatus::WRONG_REQUEST, "Unknown request for manual approval", ctx); | ||
| } | ||
|
|
||
| THolder<TRequestInfo> copy = MakeHolder<TRequestInfo>(it->second); |
There was a problem hiding this comment.
| THolder<TRequestInfo> copy = MakeHolder<TRequestInfo>(it->second); | |
| auto copy = MakeHolder<TRequestInfo>(it->second); |
То же самое.
| THolder<TRequestInfo> copy = MakeHolder<TRequestInfo>(it->second); | ||
|
|
||
| // Create a permission for each action in the scheduled request | ||
| THolder<TEvCms::TEvPermissionResponse> resp = MakeHolder<TEvCms::TEvPermissionResponse>(); |
There was a problem hiding this comment.
| THolder<TEvCms::TEvPermissionResponse> resp = MakeHolder<TEvCms::TEvPermissionResponse>(); | |
| auto resp = MakeHolder<TEvCms::TEvPermissionResponse>(); |
| resp->Record.MutableStatus()->SetCode(TStatus::ALLOW); | ||
| for (const auto& action : copy->Request.GetActions()) { | ||
| auto items = ClusterInfo->FindLockedItems(action, &ctx); | ||
| for (const auto& item : items) { |
There was a problem hiding this comment.
Тут двойной indirection, т.к. item — это указатель.
| duration += TDuration::MicroSeconds(copy->Request.GetDuration()); | ||
| // To get permissions ASAP and not in the priority order. | ||
| item->DeactivateScheduledLocks(Min<i32>()); | ||
| bool isLocked = item->IsLocked(error, State->Config.DefaultRetryTime, TActivationContext::Now(), duration); |
There was a problem hiding this comment.
| bool isLocked = item->IsLocked(error, State->Config.DefaultRetryTime, TActivationContext::Now(), duration); | |
| const bool isLocked = item->IsLocked(error, State->Config.DefaultRetryTime, ctx.Now(), duration); |
|
|
||
| auto* perm = resp->Record.AddPermissions(); | ||
| perm->MutableAction()->CopyFrom(action); | ||
| TInstant deadline = TActivationContext::Now() + TDuration::MicroSeconds(copy->Request.GetDuration()); |
There was a problem hiding this comment.
| TInstant deadline = TActivationContext::Now() + TDuration::MicroSeconds(copy->Request.GetDuration()); | |
| const TInstant deadline = ctx.Now() + TDuration::MicroSeconds(copy->Request.GetDuration()); |
Там, откуда ты скопировал TActivationContext::Now() нет TActorContext, а тут есть.
Changelog entry
Adds ability to manually approve scheduled CMS request and receive permissions. This allows, for example, locking an already failed node.
Changelog category
Description for reviewers
...