[Disk Manager] Fix tests on tasks metrics: do not use NotBefore method of mocks #3263

gy2411 · 2025-03-26T16:00:06Z

In this pr we fix TestHangingTasksMetrics and TestListerMetricsCleanup tests. These tests worked incorrectly, but we didn't know about that because they passed.

The problem is the following. In go mocks, if a mock method call violates the requirement NotBefore, the method panics. In TestHangingTasksMetrics and TestListerMetricsCleanup tests we have mock metrics but 'real' task controller. If a method of mock metrics panics during task execution, the panic is handled in task controller. This leads to the strange situation: Must not be called before error occurs but the test passes.

Moreover, we used NotBefore requirement in these tests incorrectly. Namely, we required that 0 is set to totalHangingTaskCount not before then 1 is. But actually, this happens:

0 is set before the task becomes hanging
The task becomes hanging, then 1 is set
Tasks stops being hanging, then 0 is set

In this pr we stop using NotBefore method in these tests.

2025-03-26T03:21:49.564Z WARN tasks/runner.go:156 got error for tasks.CollectListerMetrics with task id fb8fa5af-e8ee-42d8-8423-cd1900514973: panic: mock: Unexpected Method Call
-----------------------------

Set(float64)
  1: 0

Must not be called before:

Set(float64)
  2: 1
goroutine 1643 [running]:
runtime/debug.Stack()
 /-S/contrib/go/_std_1.21/src/runtime/debug/stack.go:24 +0x5e
github.com/ydb-platform/nbs/cloud/tasks/errors.NewPanicError(...)
 /-S/cloud/tasks/errors/errors.go:287
github.com/ydb-platform/nbs/cloud/tasks.(*runnerForRun).run.func1()
 /-S/cloud/tasks/runner.go:296 +0x3d
panic({0x28cd20?, 0xc000e94ba0?})
 /-S/contrib/go/_std_1.21/src/runtime/panic.go:914 +0x21f
github.com/stretchr/testify/mock.(*Mock).fail(0xc001f8e8a0, {0x4c42df?, 0xc00122ccd0?}, {0xc000c47110?, 0x1?, 0x1?})
 /-S/vendor/github.com/stretchr/testify/mock/mock.go:332 +0x134
github.com/stretchr/testify/mock.(*Mock).MethodCalled(0xc001f8e8a0, {0xa83350, 0x3}, {0xc000e949c0, 0x1, 0x1})
 /-S/vendor/github.com/stretchr/testify/mock/mock.go:506 +0x965
github.com/ydb-platform/nbs/cloud/tasks/metrics/mocks.(*Mock).Called(0xc001f8e8a0, {0xc000e949c0, 0x1, 0x1})
 /-S/cloud/tasks/metrics/mocks/metrics_mock.go:59 +0x149
github.com/ydb-platform/nbs/cloud/tasks/metrics/mocks.(*GaugeMock).Set(0xc001f8e8a0, 0x0)
 /-S/cloud/tasks/metrics/mocks/metrics_mock.go:87 +0x7d
github.com/ydb-platform/nbs/cloud/tasks.(*collectListerMetricsTask).cleanupMetrics(0xc000fad450, {0xc000d7b030?, 0x1809c65?, 0x45ba80?})
 /-S/cloud/tasks/collect_lister_metrics_task.go:206 +0x252
panic({0x28cd20?, 0xc000e945e0?})
 /-S/contrib/go/_std_1.21/src/runtime/panic.go:914 +0x21f
github.com/stretchr/testify/mock.(*Mock).fail(0xc001f8e8a0, {0x4c42df?, 0xc00122ccd0?}, {0xc000c46ff0?, 0x1?, 0x1?})
 /-S/vendor/github.com/stretchr/testify/mock/mock.go:332 +0x134
github.com/stretchr/testify/mock.(*Mock).MethodCalled(0xc001f8e8a0, {0xa83350, 0x3}, {0xc000e943c0, 0x1, 0x1})
 /-S/vendor/github.com/stretchr/testify/mock/mock.go:506 +0x965
github.com/ydb-platform/nbs/cloud/tasks/metrics/mocks.(*Mock).Called(0xc001f8e8a0, {0xc000e943c0, 0x1, 0x1})
 /-S/cloud/tasks/metrics/mocks/metrics_mock.go:59 +0x149
github.com/ydb-platform/nbs/cloud/tasks/metrics/mocks.(*GaugeMock).Set(0xc001f8e8a0, 0x0)
 /-S/cloud/tasks/metrics/mocks/metrics_mock.go:87 +0x7d
github.com/ydb-platform/nbs/cloud/tasks.(*collectListerMetricsTask).collectTasksMetrics(0xc000fad450, {0x829478, 0xc000b22630}, 0xc000d7ac40, {0x491066, 0x15})
 /-S/cloud/tasks/collect_lister_metrics_task.go:125 +0x202
github.com/ydb-platform/nbs/cloud/tasks.(*collectListerMetricsTask).collectHangingTasksMetrics(0xc000fad450, {0x829478, 0xc000b22630})
 /-S/cloud/tasks/collect_lister_metrics_task.go:140 +0xcb
github.com/ydb-platform/nbs/cloud/tasks.(*collectListerMetricsTask).Run(0xc000fad450, {0x829478, 0xc000b22630}, {0x8322e0?, 0xc00012a780?})
 /-S/cloud/tasks/collect_lister_metrics_task.go:71 +0x23a

github-actions · 2025-03-26T17:14:37Z

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 0953d60.

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
6478	6466	0	0	8	4

BarkovBG · 2025-03-27T12:25:38Z

cloud/tasks/tasks_tests/tasks_test.go

+			if totalHangingTasksCount.CompareAndSwap(0, 1) {
+				gaugeSetWg.Done()


а если не получится записать, то мы же никогда не дождемся Wait?

В корректном испольнении обязательно получится записать.

Сначала таск зависает, и в атомик запсывается 1 (успешно, т.к. изначально там 0).

Тест дожидается этого.

Тест отменяет таск.

В атомик записывается 0 (успешно, так как там уже записан 1).

Тест дожидается этого и завершается.

if нужен, чтобы не дёргать Done, когда этого не нужно делать.

изменение по моему мнению выглядит слишком сложным

может легче стартовать раннеров после того, как зашедулили hanging tasks?

может попробовать дождаться первого 0 в метрике totalHangingTasksCount?

вообще проблема НЕ может воспроизводиться на метрике hangingTasksCount, поэтому менять нужно только код вокруг totalHangingTasksCount

может быть в testify есть методы, которые позволяют сделать то, что нам нужно? какой-нибудь After или что-то вроде того?

может легче стартовать раннеров после того, как зашедулили hanging tasks?
может попробовать дождаться первого 0 в метрике totalHangingTasksCount?

Тут дело в том, что 0 в эту метрику будут постоянно записываться в фоне с каким-то периодом -- и так до тех пор, пока не пройдёт HangingTaskTimeout. Поэтому не понятно, как эти меры могли бы помочь.

Вообще проблема НЕ может воспроизводиться на метрике hangingTasksCount, поэтому менять нужно только код вокруг totalHangingTasksCount

Да, но код хочется сделать единнобразным. Иначе получается ещё сложнее.

может быть в testify есть методы, которые позволяют сделать то, что нам нужно? какой-нибудь After или что-то вроде того?

Не вижу подходящего инструмента(
В качестве альтернативы думал вызывать On для перехода в 0 уже после того, как дождались 1. Но есть и другая проблема -- как гарантировать, что wg.Done() не будет вызван лишний раз?

ИМХО, текущий вариант довольно естественный. Нам интересуют два события:

Переход счётчика из 0 в 1

Переход счётчика из 1 в 0

И проверки в CompareAndSwap срабатывают ровно на эти события.

Изменил подход согласно предложению @jkuradobery #3263 (review)

gy2411 · 2025-03-28T14:02:03Z

cloud/tasks/tasks_tests/tasks_test.go

@@ -112,6 +112,7 @@ func newDefaultConfig() *tasks_config.TasksConfig {
 	clearEndedTasksLimit := uint64(10)
 	maxRetriableErrorCount := uint64(2)
 	hangingTaskTimeout := "100s"
+	maxPanicCount := uint64(0)


Добавил это в конфиг для всех tasks_tests, чтобы не пропускать паники в тасках. Теперь, если в каком-либо тесте таска запаникует, сразу вернётся неретрабельная ошибка, и тест упадёт.

github-actions · 2025-03-28T15:11:19Z

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 8175d56.

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
6476	6476	0	0	0	0

jkuradobery

I propose we send setter for a gauge to a channel. And we wait for 0 -> 1-> 0 history in the channel (there can be contiguous duplicates, but they should be ignored).
This way we can get rid of waitgroups and improve readability.

cloud/tasks/tasks_tests/tasks_test.go

github-actions · 2025-03-31T12:26:18Z

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit d9ab64e.

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
6476	6476	0	0	0	0

github-actions · 2025-03-31T14:20:09Z

Note

This is an automated comment that will be appended during run.

🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit 8e98788.

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
6477	6469	0	0	6	2

cloud/tasks/tasks_tests/tasks_test.go

jkuradobery

Good job, LGTM

fix tests on tasks metrics: do not use NotBefore method of mocks

0953d60

BarkovBG reviewed Mar 27, 2025

View reviewed changes

set max panic count to zero in config for tasks tests

8175d56

gy2411 commented Mar 28, 2025

View reviewed changes

jkuradobery requested changes Mar 28, 2025

View reviewed changes

gy2411 commented Mar 31, 2025

View reviewed changes

cloud/tasks/tasks_tests/tasks_test.go Show resolved Hide resolved

jkuradobery previously approved these changes Mar 31, 2025

View reviewed changes

use channels in tests on hanging tasks metrics

8e98788

gy2411 dismissed jkuradobery’s stale review via 8e98788 March 31, 2025 13:05

gy2411 force-pushed the users/gayurgin/fix_tasks_metrics_tests branch from d9ab64e to 8e98788 Compare March 31, 2025 13:05

BarkovBG reviewed Apr 1, 2025

View reviewed changes

cloud/tasks/tasks_tests/tasks_test.go Show resolved Hide resolved

cloud/tasks/tasks_tests/tasks_test.go Show resolved Hide resolved

cloud/tasks/tasks_tests/tasks_test.go Show resolved Hide resolved

add comments, add empty line

1e06a2e

BarkovBG approved these changes Apr 2, 2025

View reviewed changes

jkuradobery approved these changes Apr 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Disk Manager] Fix tests on tasks metrics: do not use NotBefore method of mocks #3263

[Disk Manager] Fix tests on tasks metrics: do not use NotBefore method of mocks #3263

gy2411 commented Mar 26, 2025 •

edited

Loading

github-actions bot commented Mar 26, 2025

BarkovBG Mar 27, 2025

gy2411 Mar 27, 2025 •

edited

Loading

BarkovBG Mar 28, 2025

gy2411 Mar 28, 2025 •

edited

Loading

gy2411 Mar 31, 2025

gy2411 Mar 28, 2025

github-actions bot commented Mar 28, 2025

jkuradobery left a comment

github-actions bot commented Mar 31, 2025

github-actions bot commented Mar 31, 2025

jkuradobery left a comment

		if totalHangingTasksCount.CompareAndSwap(0, 1) {
		gaugeSetWg.Done()

[Disk Manager] Fix tests on tasks metrics: do not use NotBefore method of mocks #3263

Are you sure you want to change the base?

[Disk Manager] Fix tests on tasks metrics: do not use NotBefore method of mocks #3263

Conversation

gy2411 commented Mar 26, 2025 • edited Loading

github-actions bot commented Mar 26, 2025

BarkovBG Mar 27, 2025

Choose a reason for hiding this comment

gy2411 Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

BarkovBG Mar 28, 2025

Choose a reason for hiding this comment

gy2411 Mar 28, 2025 • edited Loading

Choose a reason for hiding this comment

gy2411 Mar 31, 2025

Choose a reason for hiding this comment

gy2411 Mar 28, 2025

Choose a reason for hiding this comment

github-actions bot commented Mar 28, 2025

jkuradobery left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 31, 2025

github-actions bot commented Mar 31, 2025

jkuradobery left a comment

Choose a reason for hiding this comment

gy2411 commented Mar 26, 2025 •

edited

Loading

gy2411 Mar 27, 2025 •

edited

Loading

gy2411 Mar 28, 2025 •

edited

Loading