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

storage: don't let probing discard storage config in the middle of partitioning #1660

Merged
merged 3 commits into from
Apr 18, 2023

Conversation

ogayot
Copy link
Member

@ogayot ogayot commented Apr 14, 2023

At least in the desktop installer with storage v2, things go bad if a probert storage run finishes during partitioning. Unless storage is already configured, we automatically discard any change done so far upon receiving a response from a probert storage run. This leads to various problems.

This PR aims at making sure the storage configuration is not reset by a probert run while we are in the middle of partitioning.

Essentially, any request that modifies the partitioning configuration will now ensure that a transaction is started. If we receive the result of a probert run in the middle of a transaction, the new data is not applied. Instead it is stored and will only be used if the user asks for a reset/refresh.

It also introduces a new endpoint storage/v2/ensure_transaction that a client can use before doing any modification to the partitioning configuration. This ensures that the data returned by a subsequent GET /storage/v2/ call will not be invalidated by a probert run.

TODO:

  • unit tests
  • API test(s)
  • VM testing
  • testing with desktop installer

@dbungert
Copy link
Collaborator

This sounds correct but, as you mention, would bear significant testing.

@@ -772,16 +785,19 @@ async def v2_guided_GET(self, wait: bool = False) \
async def v2_guided_POST(self, data: GuidedChoiceV2) \
-> GuidedStorageResponseV2:
log.debug(data)
self.locked_probe_data = True
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we load the probe data here too.

@ogayot ogayot force-pushed the no-probert-during-partitioning branch from 1f595e0 to cf4e4fc Compare April 17, 2023 13:30
ogayot added 3 commits April 18, 2023 10:52
…rtitioning

If a probert run finished in the middle of partitioning, the probing
data would be loaded automatically and this would essentially discard
changes made so far by the user. This is not a desirable behavior.

Upon starting partitioning, we now prevent later probert runs from
automatically loading the probe data. Instead, the data is stored and
only loaded after after a reset.

Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
@ogayot ogayot force-pushed the no-probert-during-partitioning branch from cf4e4fc to 9e4cd77 Compare April 18, 2023 09:03
@ogayot
Copy link
Member Author

ogayot commented Apr 18, 2023

I think at this point it's ready to review, but I want to run manual tests.

@ogayot ogayot marked this pull request as ready for review April 18, 2023 09:09
@dbungert dbungert changed the base branch from main to ubuntu/lunar April 18, 2023 17:45
@dbungert dbungert changed the base branch from ubuntu/lunar to main April 18, 2023 17:45
Copy link

@sil2100 sil2100 left a comment

Choose a reason for hiding this comment

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

Just a small completely irrelevant inline comment, but still browsing through the rest of the logic.

Copy link

@sil2100 sil2100 left a comment

Choose a reason for hiding this comment

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

A very uh, non-expert approve here. But the logic feels fine as well.

@dbungert dbungert merged commit 5586382 into canonical:main Apr 18, 2023
@ogayot ogayot deleted the no-probert-during-partitioning branch April 19, 2023 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants