[DenisPolo]

… maintenance mode

https://issues.apache.org/jira/browse/IGNITE-27344

Enhanced IgniteLogInfoProviderImpl#ackNodeBasicMetrics() to provide more detailed information when a node is started in Maintenance Mode.

The enhancement includes:

An explicit indication that the node has been launched in Maintenance Mode.
Logging of currently registered maintenance tasks, including their names, descriptions, parameters, and executed actions.
Reporting of maintenance task execution status, including transitions to COMPLETED, CANCELLED, and FAILED.
For example, when a node starts in Maintenance Mode, the following message is displayed:

>>> +----------------------------------------------------------------+
>>> ATTENTION! MAINTENANCE MODE ENABLED!
>>> 
>>> Node is being started in maintenance mode.
>>> Starting IsolatedDiscoverySpi instead of configured discovery SPI.
>>> +----------------------------------------------------------------+

Maintenance task execution is also reflected in node basic metrics.
For example, for defragmentationMaintenanceTask:

Task registration / execution started:

    ^-- REGISTERED name=defragmentationMaintenanceTask, description=Caches defragmentation, params=[default], executedActions=[execute=ACTIVE]

Successful completion:

	^-- UNREGISTERED name=defragmentationMaintenanceTask, description=Caches defragmentation, params=[default], executedActions=[execute=ACTIVE->COMPLETED]

Cancellation:

    ^-- UNREGISTERED name=defragmentationMaintenanceTask, description=Caches defragmentation, params=[default], executedActions=[execute=ACTIVE->CANCELLED, stop=ACTIVE->COMPLETED]

Execution failure:

    ^-- REGISTERED name=defragmentationMaintenanceTask, description=Caches defragmentation, params=[default], executedActions=[execute=ACTIVE->FAILED]

Previously, the information provided by ackNodeBasicMetrics() did not give sufficient visibility into the node's Maintenance Mode state or the progress and outcome of maintenance task execution.
This enhancement makes it easier to identify nodes running in Maintenance Mode and monitor maintenance task execution without requiring additional investigation. It also helps operators understand whether a maintenance task is running, completed successfully, cancelled, or failed, simplifying troubleshooting and maintenance operations.

[nizhikov]

Thanks for the contribution. I built the branch, ran MaintenanceLoggingTest (passes) and checkstyle on ignite-core (clean). The mechanics work, but I don't think the reported status is correct for the real maintenance tasks yet. Details below.

Blocking

1. Status is wrong for defragmentation (the use case named in the ticket). ExecuteDefragmentationAction.execute() only starts a daemon thread and returns true, so the log shows the task as COMPLETE within milliseconds while defragmentation is actually running. Status is set purely around the execute() call in MaintenanceProcessor#proceedWithMaintenance.

2. Action results are ignored. ExecuteDefragmentationAction and RebuildIndexAction signal failure by returning Boolean.FALSE, not by throwing. The PR only sets FAILED on an exception, so a failed index rebuild is logged as COMPLETE.

3. Manual actions are not tracked. The corrupted-PDS task has no automatic action; its actions are executed from control.sh via actionsForMaintenanceTask(...).get(i).execute(), and nothing on that path updates the state. For that task the log never shows ACTIVE/COMPLETE/FAILED, which contradicts "currently executing maintenance task and its completion status" from the ticket.

4. CALLED is set on listing, not on execution. actionsForMaintenanceTask flips the status every time the action list is retrieved. PersistenceTask calls it for persistence info too, so a read-only control.sh query changes the reported state, and it overwrites a previous COMPLETE/FAILED. "CALLED" also doesn't describe a task state.

   Suggestion for 1–4: wrap MaintenanceAction in a tracking decorator for both the automatic action and the list returned by actionsForMaintenanceTask, so start, end, thrown exception and a false result are all observed regardless of who triggers the action. Alternatively, drop the fine-grained status and report only what is reliably known: active tasks and the currently running action.

5. Public API leak. MaintenanceRegistry is public API, and the new tasksStatuses() returns a preformatted string with hard-coded indentation and ^-- prefixes. Formatting belongs in IgniteLogInfoProviderImpl. Expose a collection of states, or keep it internal (there is a single implementation, so the log provider can work with MaintenanceProcessor directly). Same for MaintenanceTaskState: it is a logging helper but lives in the public org.apache.ignite.maintenance package.

Should fix

6. NPE on tasksStates.get(...). registerWorkflowCallback does not require the task to be active, so a callback registered for an unknown name now NPEs in proceedWithMaintenance instead of just running. In-core callers are guarded, but the API is public. Use computeIfAbsent or validate in registerWorkflowCallback.

7. Stale entries. Tasks removed via unregisterMaintenanceTask (including those rejected by shouldProceedWithMaintenance in prepareAndExecuteMaintenance) stay in tasksStates as REGISTERED and keep being logged. Remove them or show a distinct "unregistered/fixed" state.

8. Padding baked into the enum. Status.val() returns "ACTIVE " etc. Alignment is a formatter concern; pad where the string is built and keep enum values clean. The "No maintenance tasks registered" branch is unreachable while isMaintenanceMode() is true.

9. Separate log record instead of extending the metrics message. The ticket asks for the info in ackNodeBasicMetrics, but the PR emits a separate log.info before the metrics block. Appending to the existing msg keeps one record per period and is easier to grep.

10. Naming / style. getTask/getStatus/setStatus → task()/status() per Ignite conventions; mntcProc in IgniteLogInfoProviderImpl holds a MaintenanceRegistry, so mntcReg; continuation lines use 8/12-space indents where surrounding code uses 4; COMPLETE vs COMPLETED; the IgniteKernal banner has top/bottom borders but no side borders, unlike existing banners.

Test

11. ACTIVE state observation relies on Thread.sleep(2000) inside actions racing the 1s metrics timer. A latch inside the action that blocks until the ACTIVE line is observed would be deterministic.
12. Failure is simulated with JUnit fail() inside execute() and the test then swallows AssertionError from prepareAndExecuteMaintenance. Throw a RuntimeException and assert on it instead.
13. The last actionsForMaintenanceTask(TASK_NAME + 2).get(1).execute() has no assertion after it and looks like leftover debugging.
14. SimpleAction / SimpleMaintenanceCallback can be static nested classes; static ACTION_EXECUTED is never reset and will break if a second test method is added.
15. No coverage for the real automatic actions (defragmentation, index rebuild), which is exactly where points 1 and 2 bite.

Process

Please move the JIRA to Patch Available, fill in the PR description, and attach a TC.Bot visa.

[nizhikov]

Round 2 review (head 2a32c93). Thanks for the quick turnaround — most of round one is addressed: the status now lands in the metrics message, tasksStatuses() is gone from the public interface, padding is out of the enum, unregistered tasks are reflected, CALLED-on-listing is gone, and the real defragmentation / index rebuild / corrupted-PDS flows have tests.

I ran checkstyle (clean), built core, and ran MaintenanceLoggingTest (15), MaintenanceDefragmentationLoggingTest (3), MaintenanceRegistrySimpleTest, IgnitePdsDefragmentationTest, WalEnableDisableWithRestartsTest and IndexCorruptionRebuildTest — all green.

Blocking

1. Breaking change to the public MaintenanceAction interface. state(MaintenanceTaskState) is added as an abstract method with no default, so every third-party MaintenanceAction implementation stops compiling and existing binaries get AbstractMethodError. The public interface in org.apache.ignite.maintenance now also imports org.apache.ignite.internal.maintenance.MaintenanceTaskState, i.e. public API depends on an internal type. The decorator approach from round one still applies: MaintenanceProcessor can wrap every action it hands out (the automatic action and the list from actionsForMaintenanceTask) and record ACTIVE before execute(), COMPLETED/FAILED after, with no public API change. Only the asynchronous defragmentation completion needs an internal hook, which can live in DefragmentationWorkflowCallback. If the interface change is kept on purpose, it needs at least a default no-op implementation and a public (not internal) state type.

2. Bookkeeping pushed into every action is fragile. Each implementation must remember state.currAction(name()).state(ACTIVE) plus a terminal state on every return path. CheckCorruptedCacheStoresCleanAction already needs two COMPLETED calls and RebuildIndexAction three terminal calls; an action that forgets stays ACTIVE forever, and one that throws before the processor injected the state NPEs. A single wrapper in the processor removes this class of bugs.

3. MaintenanceTaskState.state is a plain ArrayList mutated from several threads. The defragmentation thread appends, control.sh threads append, and the metrics timer iterates it via stream() in tasksStates(). That's a ConcurrentModificationException during a long defragmentation. Use CopyOnWriteArrayList or synchronize; a lastState() accessor would also replace the repeated states.get(states.size() - 1).

4. testRebuildIndexActionCompletedLog is vacuous. The thin-client SQL (CREATE TABLE, CREATE INDEX, 2048 inserts) never executes: IgniteClient.query() returns a lazy cursor and nothing iterates it, and core's test classpath has no SQL engine anyway. The test takes ~1.3 s; RebuildIndexAction finds no index descriptor, skips, and reports COMPLETED. To cover a real rebuild, the test belongs in modules/indexing next to IndexCorruptionRebuildTest.

Should fix

5. tasksStates() returns List<List<String>>. Positional string tuples indexed 0..4 in IgniteLogInfoProviderImpl are hard to read and easy to break. Return Collection<MaintenanceTaskState> and format from the object.

6. NOT_REGISTERED state. registerWorkflowCallback for a name with no stored task fabricates a MaintenanceTask(name, "", null) and logs it. That only happens on misuse (all in-core callers are guarded), and the javadoc ("registered through registerWorkflowCallback, not registered from storage") is confusing. Reject or warn instead of inventing a state.

7. Defragmentation COMPLETED after a cancel can never be recorded. ExecuteDefragmentationAction checks state.state().contains(CANCELLED) over the whole history. Cancel once, start again via control.sh, let it finish → COMPLETED is suppressed. Check the last state, not the history.

8. State order for index rebuild reads ACTIVE->UNREGISTERED->COMPLETED because unregisterMaintenanceTask() runs before state(COMPLETED). Swap them (or record COMPLETED in the wrapper).

9. Side effects in peek. actionsForMaintenanceTask uses stream().peek(a -> a.state(...)).toList(). peek is for debugging and may be elided; use a plain loop.

10. Mixed lifecycle and action states in one history. REGISTERED/UNREGISTERED/NOT_REGISTERED describe the task, ACTIVE/COMPLETED/FAILED/CANCELLED describe an action, and action names aren't in the history — so ACTIVE->COMPLETED->ACTIVE->COMPLETED for the corrupted-PDS task doesn't say which action ran. Consider actions=[check=COMPLETED, clean=ACTIVE], or at least the last action with its state.

11. Style. Single-line if (cond) stmt; throughout the new code (MaintenanceProcessor, the actions, the log provider). Checkstyle allows it, but surrounding code and Ignite convention put the statement on the next line.

Test nits

12. StopAction.execute() in MaintenanceLoggingTest contains assertTrue(... check(2000)) inside the action itself; assertions belong in the test method.
13. mntcReg is a static field reassigned per test and read from action threads; a per-test instance field avoids cross-test leakage.
14. testLogRegisterActiveFailedDefragmentationTask wraps a Mockito mock of CachePartitionDefragmentationManager in a real DefragmentationWorkflowCallback. Works, but IgnitePdsDefragmentationTest already has infrastructure for failing defragmentation that may be reusable.

Process

PR description is still the template — please fill in what/why.

Verdict

Not ready yet, mainly because of the public API break in MaintenanceAction and the unsynchronized state list. The direction is right and the round-one semantics issues are fixed for the covered flows; with tracking moved back into the processor as a wrapper and the list made thread-safe, this should be close.