Change in vdsm[master]: Use shared lock for setStorageDomainDescription command

abaron at redhat.com abaron at redhat.com
Wed Jul 24 05:59:02 UTC 2013


Ayal Baron has posted comments on this change.

Change subject: Use shared lock for setStorageDomainDescription command
......................................................................


Patch Set 3:

^ This review is temporary in order to avoid the merge of this patch until some questions will be ellucidated.

This does not mean it should have a -2

^ 
^ By https://bugzilla.redhat.com/show_bug.cgi?id=819316#c2 this is and old issue. (Already known in 2009!)

That doesn't mean it shouldn't be fixed

^ 
^ The actual state is that this operation can be issued to a host which is not the SPM. In the block case this can lead to DL. 

Which in itself is a good enough reason to fix it

^ 
^ If such cases are not exist, we can deduce that the command was not issued very often and/or that engine sent this command to the SPM only.

The fact that engine doesn't call it as an HSM verb doesn't mean we should leave it as such.

^ 
^ The actual patch makes the command to fail if it was issued to a non SPM host, which is absolutely right, but this can lead to removal the engine protection, if it 
exists. In such case new engines sending this command to wrong hosts running the old vdsm (unprotected) code and producing DL.

This doesn't even make sense and we're wasting time here on not merging a patch that you agree is fixing the problem.
 
^ The SD description (name in the MD) is retrieved by getStorageDomainInfo command, unused by vdsm, and never passed as an indentifier, compared, etc.

The purpose of it is to be able to correlate what this domain is from vdsm side without needing to know what the UUID of the domain means.

^ 
^ In case that the engine issued the getStorageDomain command I'm dobious if this field content is used in any form.
^ 
^ The same info is already in the engine DB. 
^ 
^ Can this verb to be replaced by a NO OP in the vdsm side? When setStorageDomainDescription is issued by the engine?

No.

-- 
To view, visit http://gerrit.ovirt.org/17198
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idca7b7b6642c222d88a6cd2a94d4033c0c3ef70b
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgotliv at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list