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.

Sign up for GitHub

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

Jump to bottom

Refactor LiveMigrateDiskParameters & MoveOrCopyImageGroupParameters #623

Merged
merged 3 commits into from
Sep 18, 2022
Merged

Refactor LiveMigrateDiskParameters & MoveOrCopyImageGroupParameters #623

merged 3 commits into from
Sep 18, 2022

Conversation

barpavel
Copy link
Member

@barpavel barpavel commented Aug 31, 2022 •
edited
Loading

Commands performing actions on 2 Storage Domains (source & destination) use MoveOrCopyImageGroupParameters or its subclasses.
Until MoveOrCopyImageGroupParameters there was only 1 Storage Domain used - the one that the action was performed on.
The support for source Storage Domain ID was added in MoveOrCopyImageGroupParameters, while the already existing (in StorageDomainParametersBase superclass) Storage Domain ID was implicitly used as the destination Storage Domain ID.
Some additional support to easier reference source & destination Storage Domains' IDs was added in LiveMigrateDiskParameters.
Still there were a few issues - a missing clear destination Storage Domain ID setter and also inconsistent naming between the classes.

Refactoring the code to add the full and naming-consistent source & destination Storage Domains' IDs getters and setters in the MoveOrCopyImageGroupParameters class, which will be inherited by its subclasses (i.e., LiveMigrateDiskParameters).

@barpavel
Copy link
Member Author

/ost

ahadas
ahadas reviewed Sep 1, 2022
View reviewed changes
Copy link
Member

@ahadas ahadas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@barpavel did you check what would happen when trying to deserialize parameters that were persisted to command_entities in a previous version? theoretically such things should be cleared on engine upgrade but we saw it's not always the case. if that fails, we'd need to keep the old setters/getter as deprecated or specify somehow that the new setter/getter replaces the previous ones

@barpavel
Copy link
Member Author

barpavel commented Sep 1, 2022

@barpavel did you check what would happen when trying to deserialize parameters that were persisted to command_entities in a previous version? theoretically such things should be cleared on engine upgrade but we saw it's not always the case. if that fails, we'd need to keep the old setters/getter as deprecated or specify somehow that the new setter/getter replaces the previous ones

Hi @ahadas,
Thanks for the comment!
No I didn't checked it. Also had some concerns about it.

  1. Can you suggest a scenario for that? I mean from what I understand command_entities DB table are commands that are WIP, that means they are removed the moment they are finished.
    What commands can start before upgrade and "continue" after?
    Should I just run LSM, shut down the engine in the middle, update the the code (to simulate the upgrade) and turn engine again and expect LSM to finish successfully?
  2. Assuming there is an issue (or just to understand the concept) - what is important in such serialization+upgrade+deserialization scenario:
    a) getter & setter method names?
    or
    b) The private data member name that they (getter & setter) provide access to?
    or
    c) both (a) & (b)

@ahadas
Copy link
Member

ahadas commented Sep 1, 2022

Should I just run LSM, shut down the engine in the middle, update the the code (to simulate the upgrade) and turn engine again and expect LSM to finish successfully?

Yes

  1. Assuming there is an issue (or just to understand the concept) - what is important in such serialization+upgrade+deserialization scenario:
    a) getter & setter method names?
    or
    b) The private data member name that they (getter & setter) provide access to?
    or
    c) both (a) & (b)

I believe (b) but maybe (c)

@barpavel
Copy link
Member Author

barpavel commented Sep 1, 2022

/ost

@barpavel
Copy link
Member Author

barpavel commented Sep 2, 2022

@barpavel did you check what would happen when trying to deserialize parameters that were persisted to command_entities in a previous version? theoretically such things should be cleared on engine upgrade but we saw it's not always the case. if that fails, we'd need to keep the old setters/getter as deprecated or specify somehow that the new setter/getter replaces the previous ones

Hi @ahadas ,
I checked the following flows (twice):

  1. Regular LSM flow with the new code.

Results - all works as expected.

  1. LSM is interrupted due to engine shut down (1st time in LiveDiskMigrateStage.CLONE_IMAGE_STRUCTURE LSM phase, the 2nd time in LiveDiskMigrateStage.IMAGE_DATA_SYNC_EXEC_END in the middle - around 50% - of disk copy).
    a) Old code all the way.
    b) New code old the way.
    c) Simulating upgrade - LSM is started with the old code, command_entities DB table contains an old LiveMigrateDiskParameters structure, engine is turned off and restarted with the new code. After that LSM flow continues automatically. To be 100% sure that the new code is in effect I added a few logs (and verified that they are printed after restart) and checked DB command_parameters column of the LiveMigrateDiskParameters structure in command_entities DB table (and verified that the old targetStorageDomainId field changes after restart to a new destStorageDomainId field in the LiveMigrateDiskParameters structure in the command_entities DB table).

Results - in all 3 above cases I see the same behavior: LSM flow continues, disk is moved to a new Storage Domain, but the operation is stuck with an endless "Failed to acquire VM lock, will retry on the next polling cycle" log message and the snapshot is not cleaned-up due to a known bug (which is not related to this change): https://bugzilla.redhat.com/2110186

@barpavel barpavel force-pushed the refactor_lsm_parameters branch 3 times, most recently from bef9836 to 63ca05c Compare September 12, 2022 15:27
@barpavel barpavel marked this pull request as ready for review September 12, 2022 15:27
@barpavel
Copy link
Member Author

/ost

1 similar comment
@barpavel
Copy link
Member Author

/ost

@barpavel barpavel force-pushed the refactor_lsm_parameters branch 2 times, most recently from 3bacee7 to 506aa13 Compare September 13, 2022 11:00
@barpavel
Copy link
Member Author

/ost

@ahadas ahadas force-pushed the refactor_lsm_parameters branch 2 times, most recently from e6cb04b to 842069c Compare September 14, 2022 12:20
@oVirt oVirt deleted a comment from barpavel Sep 14, 2022
@ahadas
Copy link
Member

ahadas commented Sep 14, 2022

/ost

@barpavel
core: add a source Storage Domain ID support in "MoveOrCopyImageGroup…
99c763f 
…Parameters"

Add a clean and naming-consistent source Storage Domain ID support in
the "MoveOrCopyImageGroupParameters" class.
1. Removing the unnecessary "getSourceStorageDomainId()" getter from
"LiveMigrateDiskParameters" class and using "getSourceDomainId()"
from its parent's "MoveOrCopyImageGroupParameters" class.
2. Improving the "setSourceDomainId()" method's parameter name.

Background:
Commands performing actions on 2 Storage Domains (source & destination)
use "MoveOrCopyImageGroupParameters" or its subclasses.
Until "MoveOrCopyImageGroupParameters" there was only 1 Storage Domain
used - the one that the action was performed on.
The support for source Storage Domain ID was added in
"MoveOrCopyImageGroupParameters", while the already existing
(in "StorageDomainParametersBase" superclass) Storage Domain
ID was implicitly used as the destination Storage Domain ID.
Some additional support to easier reference source & destination Storage Domains
was added in "LiveMigrateDiskParameters".
Still there were a few issues - a missing clear destination
Storage Domain ID setter and also inconsistent naming between the classes.

Refactoring the code to add the full and naming-consistent source & destination
Storage Domains getters and setters in the "MoveOrCopyImageGroupParameters" class,
thus those will be inherited by its subclasses (i.e., "LiveMigrateDiskParameters").

Signed-off-by: Pavel Bar <pbar@redhat.com>
@barpavel
core: add a destination Storage Domain ID support in "MoveOrCopyImage…
aa6fe42 
…GroupParameters"

Add a clean and naming-consistent destination Storage Domain ID support in
the "MoveOrCopyImageGroupParameters" class.
Actually hiding the usage of the data member from "StorageDomainParametersBase"
superclass and wrapping it's getter & setter by the new ones.

1. Fix naming of the getter from "getTargetStorageDomainId()" to a more consistent
"getDestDomainId()" and move it from "LiveMigrateDiskParameters" class
to its parent's "MoveOrCopyImageGroupParameters".
2. Add a setter "setDestDomainId()" in "MoveOrCopyImageGroupParameters" class.
3. Refactoring code over multiple classes to use the above accessor & mutator.
4. Better and consistent names for "MoveOrCopyImageGroupParameters" Constructors.

Background:
Commands performing actions on 2 Storage Domains (source & destination)
use "MoveOrCopyImageGroupParameters" or its subclasses.
Until "MoveOrCopyImageGroupParameters" there was only 1 Storage Domain
used - the one that the action was performed on.
The support for source Storage Domain ID was added in
"MoveOrCopyImageGroupParameters", while the already existing
(in "StorageDomainParametersBase" superclass) Storage Domain
ID was implicitly used as the destination Storage Domain ID.
Some additional support to easier reference source & destination Storage Domains
was added in "LiveMigrateDiskParameters".
Still there were a few issues - a missing clear destination
Storage Domain ID setter and also inconsistent naming between the classes.

Refactoring the code to add the full and naming-consistent source & destination
Storage Domains getters and setters in the "MoveOrCopyImageGroupParameters" class,
thus those will be inherited by its subclasses (i.e., "LiveMigrateDiskParameters").

Signed-off-by: Pavel Bar <pbar@redhat.com>
@barpavel
core: final renamings for consistency
1fb92cc 
1. "sourceStorageDomainId" --> "sourceDomainId"
2. "targetStorageDomainId" --> "destDomainId"
3. "target" --> "dest" / "destination"

Signed-off-by: Pavel Bar <pbar@redhat.com>
@barpavel
Copy link
Member Author

/ost

@ahadas ahadas merged commit 41afbed into oVirt:master Sep 18, 2022
@barpavel barpavel deleted the refactor_lsm_parameters branch September 18, 2022 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bennyz bennyz bennyz approved these changes

@ahadas ahadas ahadas approved these changes

@ArtiomDivak ArtiomDivak Awaiting requested review from ArtiomDivak

@mkemel mkemel Awaiting requested review from mkemel

@smelamud smelamud Awaiting requested review from smelamud smelamud is a code owner

Assignees

@bennyz bennyz

Labels
storage verified
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@barpavel @ahadas @smelamud @bennyz