Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

meta: fix upgrade to v3 #3048

Merged
merged 7 commits into from
Dec 12, 2024
Merged

meta: fix upgrade to v3 #3048

merged 7 commits into from
Dec 12, 2024

Conversation

roman-khimov
Copy link
Member

No description provided.

Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 65.51724% with 10 lines in your changes missing coverage. Please review.

Project coverage is 22.43%. Comparing base (4f4840c) to head (d16d757).
Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
pkg/local_object_storage/metabase/version.go 65.38% 5 Missing and 4 partials ⚠️
pkg/local_object_storage/metabase/status.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3048      +/-   ##
==========================================
+ Coverage   22.41%   22.43%   +0.01%     
==========================================
  Files         791      791              
  Lines       58405    58381      -24     
==========================================
+ Hits        13093    13096       +3     
+ Misses      44416    44386      -30     
- Partials      896      899       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@carpawell
Copy link
Member

For now, the only idea of how this could happen is restarting before migration is completed.

@roman-khimov
Copy link
Member Author

But it all happens in a single transaction, either it's in or it's out.

Fixes

    error        shard/control.go:17        metabase failure, switching mode        {"shard_id": "XxhmeQZZRSQfDGfiCYo9mB", "stage": "init", "mode": "READ_ONLY", "error": "migrating from 2 to 3 version failed, consider database resync: graveyard value with unexpected 72 length"}

Signed-off-by: Roman Khimov <[email protected]>
@roman-khimov roman-khimov added this to the v0.45.0 milestone Dec 12, 2024
@roman-khimov roman-khimov added bug Something isn't working U1 Critically important to resolve quickly S4 Routine I4 No visible changes labels Dec 12, 2024
@roman-khimov roman-khimov marked this pull request as ready for review December 12, 2024 13:27
Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this explains a lot.

@roman-khimov
Copy link
Member Author

The key is 2addebf, yes. And that was present in the original patch as well.

Copy link
Contributor

@cthulhu-rider cthulhu-rider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roman-khimov d34687c worth changelog to me, very admin-friendly change

@roman-khimov
Copy link
Member Author

@roman-khimov d34687c worth changelog to me, very admin-friendly change

I don't think it changes anything for the admin, especially now. This would be a (really minor) issue for us when migration from 3 to 4 is to appear, old structure can handle this too, but it's somewhat less convenient.

Use complete DB reload to get closer to how node works in real life, it
usually opens an existing DB and any open/init code can affect the result.

Signed-off-by: Roman Khimov <[email protected]>
It wasn't updated previously leading to attempts to upgrade the DB on every
start. Which then leads to

    error        shard/control.go:17        metabase failure, switching mode        {"shard_id": "XxhmeQZZRSQfDGfiCYo9mB", "stage": "init", "mode": "READ_ONLY", "error": "migrating from 2 to 3 version failed, consider database resync: graveyard value with unexpected 72 length"}

on 0.44.0/0.44.1.

Signed-off-by: Roman Khimov <[email protected]>
Reuse getVersion().

Signed-off-by: Roman Khimov <[email protected]>
Too short and generic name, shadowed already in updateVersion().

Signed-off-by: Roman Khimov <[email protected]>
This code was added in 6f243a2 and it looks
like it was mainly to ensure some correct processing of metabases that have
no version info stored at all (they could've had some valid data still).
Nowadays all correct metabases have version data stored, if it's not there
it's either a broken or a new (more likely) DB. Also, doing any things like
that in Open() is suspicious, it shouldn't care much about the state, that's
what Init() is for.

Notice that ErrOutdatedVersion for unknown versions is handled above.

Signed-off-by: Roman Khimov <[email protected]>
Each handler works with a particular version, each updates to the next version,
chaining them we're getting to the current, that's it.

Signed-off-by: Roman Khimov <[email protected]>
@roman-khimov roman-khimov merged commit 474d541 into master Dec 12, 2024
22 checks passed
@roman-khimov roman-khimov deleted the fix-meta-upgrade branch December 12, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working I4 No visible changes S4 Routine U1 Critically important to resolve quickly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants