Nir Soffer has uploaded a new change for review.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
vm: Fix attribute error when accessing drive in sampling method
Du to race when migration is finished and monitoring, drive may not have a format attribute when accessing it from the monitor. This patch use getattr to log spam.
Change-Id: Ia50e8af94b9c9b54332066a3f30999ce73e7a56f Signed-off-by: Nir Soffer nsoffer@redhat.com --- M vdsm/vm.py 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/18/22518/1
diff --git a/vdsm/vm.py b/vdsm/vm.py index bb4a7ec..7e2d220 100644 --- a/vdsm/vm.py +++ b/vdsm/vm.py @@ -506,7 +506,8 @@ return
for vmDrive in self._vm._devices[DISK_DEVICES]: - if not vmDrive.blockDev or vmDrive.format != 'cow': + # Note: drive may not have a format attribute during migration + if not vmDrive.blockDev or getattr(vmDrive, 'format', None) != 'cow': continue
capacity, alloc, physical = \
Nir Soffer has posted comments on this change.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
Patch Set 1:
Patch for testing this issue on a user machine. Patch for upstream and ovirt current release will be little different since that code move to another function.
Vinzenz Feenstra has posted comments on this change.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
Patch Set 1: Code-Review+1
Federico Simoncelli has posted comments on this change.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
Patch Set 1:
Is this a backport or is it a patch needed only in ovirt-3.3?
Sergey Gotliv has posted comments on this change.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
I assume that this is an upstream patch, right?
.................................................... Commit Message Line 5: CommitDate: 2013-12-18 13:52:35 +0200 Line 6: Line 7: vm: Fix attribute error when accessing drive in sampling method Line 8: Line 9: Du to race when migration is finished and monitoring, drive may not have Due Line 10: a format attribute when accessing it from the monitor. This patch use Line 11: getattr to log spam. Line 12: Line 13: Change-Id: Ia50e8af94b9c9b54332066a3f30999ce73e7a56f
Nir Soffer has posted comments on this change.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
Patch Set 1:
This is a patch for a user running 4.13.0. Patch for upstream will be in another function.
But there is another patch that seems better, fixing the root cause: http://gerrit.ovirt.org/21036
Dan Kenigsberg has posted comments on this change.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
Patch Set 1:
(1 comment)
.................................................... Commit Message Line 6: Line 7: vm: Fix attribute error when accessing drive in sampling method Line 8: Line 9: Du to race when migration is finished and monitoring, drive may not have Line 10: a format attribute when accessing it from the monitor. This patch use Could you elaborate on the nature of this race? Is it complex to solve (and hence better be left for a future patch)? Line 11: getattr to log spam. Line 12: Line 13: Change-Id: Ia50e8af94b9c9b54332066a3f30999ce73e7a56f
Dan Kenigsberg has posted comments on this change.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
Patch Set 1:
Ah sorry, Nir, I did not see your latest comment. How does the __slots__ patch fix the race?
Nir Soffer has posted comments on this change.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
Patch Set 1:
(1 comment)
.................................................... Commit Message Line 6: Line 7: vm: Fix attribute error when accessing drive in sampling method Line 8: Line 9: Du to race when migration is finished and monitoring, drive may not have Line 10: a format attribute when accessing it from the monitor. This patch use No, I don't know why format is sometimes missing. This patch assume that it is ok that it is missing, since user only complain is about log spamming. Line 11: getattr to log spam. Line 12: Line 13: Change-Id: Ia50e8af94b9c9b54332066a3f30999ce73e7a56f
Nir Soffer has posted comments on this change.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
Patch Set 1:
The race is not the problem. The problem is that the code treat attributes as dicts keys, creating them dynamically, but does not check them as dict keys, assuming that they are not there. The __slots__ patch solve this.
If there is another problem hiding behind this, it will be easier to debug when we have a cleaner code. This patch is only the first step in understanding this issue, and user is ready running with this path and sending us logs.
Nir Soffer has posted comments on this change.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
Patch Set 1:
Note also that in the context of _highwrite, we don't care if the vmDrive has a format or not. We care only about vmDrive when vmDrive.format == 'cow' - so any other case is not interesting.
If the code was sane, wmDrive was None in this case, and vmDrive.format == 'cow' was not making any unwanted noise.
Nir Soffer has posted comments on this change.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
Patch Set 1: Verified+1
Verified on user machine.
Nir Soffer has posted comments on this change.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
Patch Set 1:
For reference: http://lists.ovirt.org/pipermail/users/2013-December/018905.html
Dan Kenigsberg has posted comments on this change.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
Patch Set 1: Code-Review-1
(2 comments)
.................................................... Commit Message Line 6: Line 7: vm: Fix attribute error when accessing drive in sampling method Line 8: Line 9: Du to race when migration is finished and monitoring, drive may not have Line 10: a format attribute when accessing it from the monitor. This patch use I think that this commit message is misleading, then. It's not a fix, it's a silent ignoring of a possibly-broken condition. We are not sure which race is involved here. Would a subject line of "Use getattr to avoid log spamming" be more precise?
Now I do not mind loosing _highWrite events on this broken temporal occasion. And we can hide this condition, as it confuses users and ourselves. Posting this patch for testing on ovirt-3.3 branch was fine, too.
However, we should not take it in without a matching patch to master (even if the code moved), or else we'd forget the patch, and see the problem popping again on ovirt-3.4. Would you use the same ChangeID for a similar tweak of the parallel code in master? Thanks!
The existing mangling of attributes is horrible, for sure. Line 11: getattr to log spam. Line 12: Line 13: Change-Id: Ia50e8af94b9c9b54332066a3f30999ce73e7a56f
.................................................... File vdsm/vm.py Line 506: return Line 507: Line 508: for vmDrive in self._vm._devices[DISK_DEVICES]: Line 509: # Note: drive may not have a format attribute during migration Line 510: if not vmDrive.blockDev or getattr(vmDrive, 'format', None) != 'cow': how come pep8 has not cried about this glitch? Line 511: continue Line 512: Line 513: capacity, alloc, physical = \ Line 514: self._vm._dom.blockInfo(vmDrive.path, 0)
Sergey Gotliv has posted comments on this change.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
Sorry, I changed my mind either, please see my comment.
.................................................... File vdsm/vm.py Line 506: return Line 507: Line 508: for vmDrive in self._vm._devices[DISK_DEVICES]: Line 509: # Note: drive may not have a format attribute during migration Line 510: if not vmDrive.blockDev or getattr(vmDrive, 'format', None) != 'cow': This is not the right place to do that change. It should be in ctor of the Drive.
Let's follow the hotplugDisk flow.
Engine sends to VDSM the map with all disk params including format:
drive.put(VdsProperties.Format, diskImage.getVolumeFormat().toString().toLowerCase());
VDSM in vm.py hotplugDisk() creating a Drive which later added to the disk device list of the VM:
drive = Drive(self.conf, self.log, **diskParams),
but I don't see that __init__ of Drive does something with the format. Line 511: continue Line 512: Line 513: capacity, alloc, physical = \ Line 514: self._vm._dom.blockInfo(vmDrive.path, 0)
Nir Soffer has posted comments on this change.
Change subject: vm: Fix attribute error when accessing drive in sampling method ......................................................................
Patch Set 1:
(1 comment)
.................................................... File vdsm/vm.py Line 506: return Line 507: Line 508: for vmDrive in self._vm._devices[DISK_DEVICES]: Line 509: # Note: drive may not have a format attribute during migration Line 510: if not vmDrive.blockDev or getattr(vmDrive, 'format', None) != 'cow': Drive.__init__ invokes VmDevice.__init__(self, conf, log, **kwargs), which is Device.__init__ (because VmDevice did not override it):
def __init__(self, conf, log, **kwargs): for attr, value in kwargs.iteritems(): try: setattr(self, attr, value) except AttributeError: # skip read-only properties pass
So if we get a format argument in kwargs, we will have format attribute. The problem is what happen when we don't get a format parameter. Line 511: continue Line 512: Line 513: capacity, alloc, physical = \ Line 514: self._vm._dom.blockInfo(vmDrive.path, 0)
Nir Soffer has posted comments on this change.
Change subject: vm: Avoid log spamming when drive format is undefined ......................................................................
Patch Set 2:
Address Dan comments.
Sergey Gotliv has posted comments on this change.
Change subject: vm: Avoid log spamming when drive format is undefined ......................................................................
Patch Set 2: Code-Review+1
Now I like it again! It seems like "format" is just lost somewhere :-)
Federico Simoncelli has posted comments on this change.
Change subject: vm: Avoid log spamming when drive format is undefined ......................................................................
Patch Set 2:
It's late so I won't -1 but it seems to me this is making the "format" attribute optional. I definitely prefer to explicitly explode (forever) rather than keep running without extensions and have no feedback whatsoever. If "format" got lost we have to find it back (meaning that we have to understand in what flow it gets lost and fix it).
Nir Soffer has posted comments on this change.
Change subject: vm: Avoid log spamming when drive format is undefined ......................................................................
Patch Set 2:
Federico, I suggest to move the discussion to http://gerrit.ovirt.org/22551.
Yaniv Bronhaim has posted comments on this change.
Change subject: vm: Avoid log spamming when drive format is undefined ......................................................................
Patch Set 2:
(1 comment)
.................................................... Commit Message Line 20: the code more clear by extracting the check to new isExtendable method. Line 21: Line 22: This patch is for ovirt-3.3.1 only - master patch must be different Line 23: because of recent refactoring in this area. Line 24: please put link to master branch patch Line 25: Change-Id: Ia50e8af94b9c9b54332066a3f30999ce73e7a56f
Allon Mureinik has posted comments on this change.
Change subject: vm: Avoid log spamming when drive format is undefined ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
.................................................... Commit Message Line 20: the code more clear by extracting the check to new isExtendable method. Line 21: Line 22: This patch is for ovirt-3.3.1 only - master patch must be different Line 23: because of recent refactoring in this area. Line 24: +1. Line 25: Change-Id: Ia50e8af94b9c9b54332066a3f30999ce73e7a56f
Itamar Heim has posted comments on this change.
Change subject: vm: Avoid log spamming when drive format is undefined ......................................................................
Patch Set 2:
ping
Yaniv Bronhaim has posted comments on this change.
Change subject: vm: Avoid log spamming when drive format is undefined ......................................................................
Patch Set 2: Code-Review-1
please add bug url and verify
Itamar Heim has posted comments on this change.
Change subject: vm: Avoid log spamming when drive format is undefined ......................................................................
Patch Set 2:
ping
Nir Soffer has posted comments on this change.
Change subject: vm: Avoid log spamming when drive format is undefined ......................................................................
Patch Set 2:
Yaniv: what bug url?
Itamar Heim has posted comments on this change.
Change subject: vm: Avoid log spamming when drive format is undefined ......................................................................
Patch Set 2:
ping
Nir Soffer has posted comments on this change.
Change subject: vm: Avoid log spamming when drive format is undefined ......................................................................
Patch Set 2:
The issue is not fixed yet - we will an update version of this patch.
Nir Soffer has abandoned this change.
Change subject: vm: Avoid log spamming when drive format is undefined ......................................................................
Abandoned
Not needed now, this issue is fixed in master.
automation@ovirt.org has posted comments on this change.
Change subject: vm: Avoid log spamming when drive format is undefined ......................................................................
Patch Set 2:
* Update tracker::IGNORE, no Bug-Url found
vdsm-patches@lists.fedorahosted.org