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

Add config file migration code #4784

Closed
fasmat opened this issue Aug 7, 2023 · 6 comments
Closed

Add config file migration code #4784

fasmat opened this issue Aug 7, 2023 · 6 comments
Assignees

Comments

@fasmat
Copy link
Member

fasmat commented Aug 7, 2023

Description

The schema of the config file might change with new features or bug fixes. A current example would be: spacemeshos/post#212.

A node should be able to load a config file in an older format, migrate it to the current and persist it again. Preferably with some information about the version of the config file.

Acceptance criteria

  • When the node initially loads the config file from disk it determines the version information of the config file
    • Add a new schema (or similarly named) field to config. If not present assume version 0 (current version)
  • Load file and parse as the version presented
    • if not current version migrate the config and persist again

Implementation hints

@fasmat fasmat self-assigned this Aug 7, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Dev team kanban Aug 7, 2023
@fasmat
Copy link
Member Author

fasmat commented Aug 7, 2023

cc: @dshulyak

@fasmat fasmat moved this from 📋 Backlog to 🏗 Doing in Dev team kanban Aug 7, 2023
@dshulyak
Copy link
Contributor

dshulyak commented Aug 7, 2023

I don't think that node should modify config provided by a user, moreover configuration can be provided with flags. If we want to backward compatibility for configuration across several releases there should be a code that works with old data

suggested approach just asks for a problem especially with smapp. smapp already maintains more than one config with overwrites. it may download new config and replace old at any point. if you will be insisting on this approach please ensure that it really works always

@fasmat
Copy link
Member Author

fasmat commented Aug 7, 2023

The concrete problem that triggered this issue is that smeshing-opts-providerid will be changed from an uint to a string with spacemeshos/post#212 (to make it nil-able).

If the config file is not updated and has the value set as integer, the node will crash during startup with the error message: "expected map, but received float64" which is misleading (it should be "expected string, but received int").

I don't know how we should make the node work with old configuration data? If we read the old config with the old code this config parameter will incorrectly be read as 0 if it is not set. 0 is a valid provider ID though and does not mean "no provider". This has lead to errors before - there is no way to NOT set a provider.

In terms of command line parameters the change in question does luckily not need any adaptions in this area. --smeshing-opts-provider=1337 works the same way after the change as it has before 🙂

We could let smapp take care of the config transition, but then nodes that run standalone will crash when they are updated. That might be OK, but as said earlier the error that will be displayed to the user won't help the user in figuring out what the issue is or how to fix it.

cc @brusherru

@dshulyak
Copy link
Contributor

dshulyak commented Aug 8, 2023

i would either add smeshing-opts-providerid-str or whole new section with version (smesher-opts-v1) and print a warning that previous option deprecated (if that option is non-default) and will removed in next release.

i am doing exactly that with that hare3, but there are also other upgradability nuances

@poszu
Copy link
Contributor

poszu commented Aug 8, 2023

I agree with @dshulyak. It will be extremely difficult to maintain, IMO it's best to avoid if possible.

Regarding the problem with smeshing-opts-provider field, it's possible to get around this without changing the type of field in JSON. I suggested it here: https://github.com/spacemeshos/go-spacemesh/pull/4773/files#r1287167363

@fasmat
Copy link
Member Author

fasmat commented Aug 8, 2023

Thanks I ended up implementing something similar that indeed makes a config migration unnecessary.

@fasmat fasmat closed this as completed Aug 8, 2023
@github-project-automation github-project-automation bot moved this from 🏗 Doing to ✅ Done in Dev team kanban Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

3 participants