metabase: fix mode metric #1049

elebedeva · 2024-03-18T22:20:08Z

elebedeva commented

2024-03-18 22:20:08 +00:00

Close #949

It used to always show CLOSED regardless of actual mode.
Now metric represents actual metabase mode of operations.

Metabase mode right after node is up:

Metabase mode after executing frostfs-cli control shards set-mode --mode read-only --all --endpoint s01.frostfs.devenv:8081 --wallet ./../frostfs-dev-env/services/storage/wallet01.json:

Signed-off-by: Ekaterina Lebedeva ekaterina.lebedeva@yadro.com

Close #949 It used to always show CLOSED regardless of actual mode. Now metric represents actual metabase mode of operations. Metabase mode right after node is up: ![Screenshot from 2024-03-19 11-56-56](/attachments/2e4c7a46-4a1f-42e5-b6fd-b22b992f93ea) Metabase mode after executing `frostfs-cli control shards set-mode --mode read-only --all --endpoint s01.frostfs.devenv:8081 --wallet ./../frostfs-dev-env/services/storage/wallet01.json`: ![Screenshot from 2024-03-19 12-01-45](/attachments/c18e31d9-8949-420a-9b88-ef9c4e651019) ![Screenshot from 2024-03-19 14-19-32](/attachments/eaab36b6-ce62-4ca7-814a-930e714adee9) Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>

Screenshot from 2024-03-19 11-56-56.png

21 KiB

Screenshot from 2024-03-19 12-01-45.png

37 KiB

Screenshot from 2024-03-19 14-19-32.png

23 KiB

elebedeva changed title from ~~WIP: metabase: fix metabase mode metric~~ to metabase: fix metabase mode metric

2024-03-19 09:56:54 +00:00

elebedeva requested review from storage-core-committers 2024-03-19 09:57:09 +00:00

elebedeva requested review from storage-core-developers 2024-03-19 09:57:11 +00:00

fyrchik requested changes 2024-03-19 12:09:13 +00:00

pkg/local_object_storage/metabase/shard_id.go Outdated

					
				@ -36,0 +50,4 @@

						return nil, ErrDegradedMode

					}

					if err := db.openDB(mode); err != nil {

fyrchik commented

2024-03-19 12:09:11 +00:00

This function (GetShardID()) is hard to use correctly: it reads the shard ID but also opens the database (and doesn't close it (!))
What prevents us from closing it here? This way we could simplify shard.UpdateID() and also always use mode.readonly here (degraded check should be on shard it is useful uder operation).

This function (`GetShardID()`) is hard to use correctly: it reads the shard ID but also opens the database (and doesn't close it (!)) What prevents us from closing it here? This way we could simplify `shard.UpdateID()` and also always use mode.readonly here (degraded check should be on shard it is useful uder operation).

elebedeva commented

2024-03-21 17:50:59 +00:00

Fixed

pkg/local_object_storage/shard/id.go Outdated

					
				@ -38,1 +36,3 @@

						metabaseOpened = false

					if idFromMetabase, err = s.metaBase.GetShardID(s.GetMode()); err != nil {

						err = fmt.Errorf("failed to read shard id from metabase: %w", err)

						if strings.Contains(err.Error(), "failed to open metabase") {

fyrchik commented

2024-03-19 12:07:40 +00:00

errors.Is is more preferable in such situations.

`errors.Is` is more preferable in such situations.

👍 1

elebedeva force-pushed fix/metabase-mode-metric from 9f752b9a1d to ed35d301fc

2024-03-21 17:44:21 +00:00

Compare

elebedeva force-pushed fix/metabase-mode-metric from ed35d301fc to c209057b57

2024-03-21 17:45:09 +00:00

Compare

elebedeva requested review from fyrchik 2024-03-21 17:51:33 +00:00

dstepanov-yadro commented

2024-03-22 07:45:36 +00:00

Change looks to complex. Is it not obvious that GetShardID and SetShardID should open and close metabase. Please explain why not just add db.metrics.SetMode(mode) in db.Open? Looks like this fixes metrics issue.

Another issue is that metabase opens to get/set shardID in degraded mode. But this could be fixed only with additional checks in Shard.UpdateID().

Change looks to complex. Is it not obvious that `GetShardID` and `SetShardID` should open and close metabase. Please explain why not just add `db.metrics.SetMode(mode)` in `db.Open`? Looks like this fixes metrics issue. Another issue is that metabase opens to get/set shardID in degraded mode. But this could be fixed only with additional checks in `Shard.UpdateID()`.

elebedeva commented

2024-03-22 09:53:27 +00:00

Change looks to complex. Is it not obvious that GetShardID and SetShardID should open and close metabase. Please explain why not just add db.metrics.SetMode(mode) in db.Open? Looks like this fixes metrics issue.

Just adding db.metrics.SetMode(mode) in db.Open causes this scenario:

engine.createShard() calls shard.UpdateID() for a new shard. At this point shard id in metabase is empty.
shard.UpdateID() calls shard.metaBase.Open (db.Open) to read shard id from metabase.
db.Open reports its mode metrics with missing shard id.
in metabase mode metrics we see mode of "shard undefined"

To fix this issue functions GetShardID and SetShardID were added. GetShardID does not report metabase mode metrics at all, and SetShardID reports metabase mode metrics only if writing shard id to metabase was successful.

> Change looks to complex. Is it not obvious that `GetShardID` and `SetShardID` should open and close metabase. Please explain why not just add `db.metrics.SetMode(mode)` in `db.Open`? Looks like this fixes metrics issue. Just adding `db.metrics.SetMode(mode)` in `db.Open` causes this scenario: 1. `engine.createShard()` calls `shard.UpdateID()` for a new shard. At this point shard id in metabase is empty. 2. `shard.UpdateID()` calls `shard.metaBase.Open` (`db.Open`) to read shard id from metabase. 3. `db.Open` reports its mode metrics with missing shard id. 4. in metabase mode metrics we see mode of "shard undefined" ![Screenshot from 2024-03-19 01-01-57](/attachments/b6e47306-f51e-4476-b424-b86dbb31e1ee) ![Screenshot from 2024-03-19 01-02-45](/attachments/659ce06a-dc05-4b6f-9df2-9bcd016f8d34) ![Screenshot from 2024-03-19 01-06-23](/attachments/c037a44f-4926-4499-a016-ec1542ea0e76) To fix this issue functions `GetShardID` and `SetShardID` were added. `GetShardID` does not report metabase mode metrics at all, and `SetShardID` reports metabase mode metrics only if writing shard id to metabase was successful.

Screenshot from 2024-03-19 01-01-57.png

14 KiB

Screenshot from 2024-03-19 01-02-45.png

24 KiB

Screenshot from 2024-03-19 01-06-23.png

55 KiB

elebedeva force-pushed fix/metabase-mode-metric from c209057b57 to c5e18cfc7d

2024-03-28 11:45:22 +00:00

Compare

elebedeva changed title from ~~metabase: fix metabase mode metric~~ to metabase: fix mode metric

2024-03-28 13:19:06 +00:00

dstepanov-yadro approved these changes 2024-03-29 06:58:25 +00:00

acid-ant commented

2024-03-29 07:38:00 +00:00

@elebedeva, agree with @dstepanov-yadro - changes is too complex. How about, instead of setting mode in Open method, set mode for metabase when shard_id created - somewhere here, as it is done for writecache?

@elebedeva, agree with @dstepanov-yadro - changes is too complex. How about, instead of setting mode in `Open` method, set mode for `metabase` when `shard_id` created - somewhere [here](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/8690db697c40926e3e77601fde5977caa4f03b2e/pkg/local_object_storage/shard/id.go#L58), as it is done for `writecache`?

elebedeva commented

2024-04-01 16:15:26 +00:00

@elebedeva, agree with @dstepanov-yadro - changes is too complex. How about, instead of setting mode in Open method, set mode for metabase when shard_id created - somewhere here, as it is done for writecache?

Setting metabase mode outside of Open() seems strange to me. Sure, it solves the problem with undefined shard but it means we should also do metrics.SetMode() after every metabase.Open() (or mode metric would be always CLOSED) - it seems even more complicated. Adding parameters like reportModeMetric bool to Open() also seems impossible bc this function must implement certain interface declared here.

> @elebedeva, agree with @dstepanov-yadro - changes is too complex. How about, instead of setting mode in `Open` method, set mode for `metabase` when `shard_id` created - somewhere [here](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/8690db697c40926e3e77601fde5977caa4f03b2e/pkg/local_object_storage/shard/id.go#L58), as it is done for `writecache`? Setting metabase mode _outside_ of `Open()` seems strange to me. Sure, it solves the problem with undefined shard but it means we should also do `metrics.SetMode()` after every `metabase.Open()` (or mode metric would be always `CLOSED`) - it seems even more complicated. Adding parameters like `reportModeMetric bool` to `Open()` also seems impossible bc this function must implement certain interface declared [here](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/e12fcc041d421c4847b027db3ba0c7525610a321/pkg/local_object_storage/shard/control.go#L46).

👍 1

elebedeva force-pushed fix/metabase-mode-metric from c5e18cfc7d to d5194ab2a6

2024-04-01 16:16:22 +00:00

Compare

acid-ant approved these changes 2024-04-01 18:54:53 +00:00

fyrchik merged commit d5194ab2a6 into master

2024-04-02 07:48:04 +00:00

Sign in to join this conversation.

No reviewers