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

Fix flapping disk info due to timing differences #369

Merged
merged 1 commit into from
Jan 26, 2023

Conversation

dupondje
Copy link
Member

In commit 45903d0 there was code added to refresh the fs info earlier when a hotplug disk was added. Now this caused some side effect that when only that command was triggered, the info dict never contained 'disk.count', which caused us to change the diskMapping to the parsed data from fsinfo.

Some minutes later when the disk command needed to be refreshed, it changed the diskMapping again, because (sometimes) the disks call returns more disks in comparision to the fsinfo call (for ex unmounted disks).

Again a bit later, the fsinfo had to be refreshed, and the diskMapping was modified again.

This occurs continiously, causing the device hash to change every time, and causing an excessive amount of dumpxml's also due to that.

Fixed this by not setting the diskMapping when we have the _QEMU_DISKS_COMMAND in our caps. As this means somewhere else in time this command will refresh the disk info.
Also refresh the disk info and not only fs info after a disk hotplug.

Signed-off-by: Jean-Louis Dupond jean-louis@dupond.be
Change-Id: I27a30e020a5221237d9d4ae7835a1cc4e69c0c59

@dupondje
Copy link
Member Author

@nyoxi don't know if you want to check this also :)

Copy link
Member

@mz-pdm mz-pdm left a comment

Choose a reason for hiding this comment

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

@nyoxi, any objections to merging this? can anything get wrong?

Copy link
Member

@mz-pdm mz-pdm left a comment

Choose a reason for hiding this comment

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

/ost

nyoxi
nyoxi previously approved these changes Jan 25, 2023
@nyoxi
Copy link
Member

nyoxi commented Jan 25, 2023

Looks good to me.

In commit 45903d0 there was code added
to refresh the fs info earlier when a hotplug disk was added.
Now this caused some side effect that when only that command was
triggered, the info dict never contained 'disk.count', which caused us
to change the diskMapping to the parsed data from fsinfo.

Some minutes later when the disk command needed to be refreshed, it
changed the diskMapping again, because (sometimes) the disks call
returns more disks in comparision to the fsinfo call (for ex unmounted
disks).

Again a bit later, the fsinfo had to be refreshed, and the diskMapping
was modified again.

This occurs continiously, causing the device hash to change every time,
and causing an excessive amount of dumpxml's also due to that.

Fixed this by not setting the diskMapping when we have the
_QEMU_DISKS_COMMAND in our caps. As this means somewhere else in time
this command will refresh the disk info.
Also refresh the disk info and not only fs info after a disk hotplug.

Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
Change-Id: I27a30e020a5221237d9d4ae7835a1cc4e69c0c59
@dupondje
Copy link
Member Author

Fixed the test and lint :)

@mz-pdm
Copy link
Member

mz-pdm commented Jan 25, 2023

@dupondje, please fix the problems reported by the CI.

@mz-pdm
Copy link
Member

mz-pdm commented Jan 25, 2023

Ah, you were faster, great. :-)

@mz-pdm
Copy link
Member

mz-pdm commented Jan 25, 2023

/ost

1 similar comment
@mz-pdm
Copy link
Member

mz-pdm commented Jan 26, 2023

/ost

@mz-pdm
Copy link
Member

mz-pdm commented Jan 26, 2023

/ost

@mz-pdm mz-pdm merged commit 1bb7ee1 into oVirt:master Jan 26, 2023
@mz-pdm
Copy link
Member

mz-pdm commented Jan 26, 2023

Thanks for the fix!

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