Change in vdsm[master]: Don't ignore nfs_mount_options in vdsm.conf

Greg Padgett has uploaded a new change for review.

Change subject: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Don't ignore nfs_mount_options in vdsm.conf

The nfs_mount_options configuration setting in vdsm.conf was ignored.
The options will now be processed, but options passed to VDSM from the
backend will take priority.

Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Signed-off-by: Greg Padgett 
---
M vdsm/storage/fileUtils.py
M vdsm/storage/storageServer.py
2 files changed, 7 insertions(+), 4 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/68/5368/1
--
To view, visit http://gerrit.ovirt.org/5368
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File vdsm/storage/fileUtils.py
Line 44
lovely. a never-used constant. there must be a python tool to find those.

....................................................
File vdsm/storage/storageServer.py
Line 256:         for opt in confOptions:
blindly adding conf options on top of DEFAULT_OPTIONS does not sound right (they may be in compatible).

maybe *replace*  DEFAULT_OPTIONS with NFS_OPTIONS from the conf file.

but please talk to Saggi/Ayal

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Saggi Mizrahi

Greg Padgett has posted comments on this change.

Change subject: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 2:

Dan Kenigsberg write:
> blindly adding conf options on top of DEFAULT_OPTIONS does
> not sound right (they may be in compatible).
>
> maybe *replace* DEFAULT_OPTIONS with NFS_OPTIONS from the 
> conf file.
>
> but please talk to Saggi/Ayal

Patch set 2 overrides options from engine if the nfs_mount_options parameter is supplied in vdsm.conf.  I removed the config object's default initialization of this option so that config.has_option() would reflect the status in the .conf file for nfs_mount_options (otherwise it would always return true).  Alternatively, we could add a new option in vdsm.conf, a boolean specifying whether or not to override.  (I'm sure there are more possibilities, this is just one idea...)

Saggi/Ayal/Dan, any thoughts on the best approach or if this patch set is suitable?  Thanks.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi

Ayal Baron has posted comments on this change.

Change subject: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 2: Looks good to me, approved

I'm not crazy about the fact that the config takes precedence over the engine and not the other way around, however, engine does not support per-host config so this does enable users to do this.
Problem is that users changing params in engine also need to remove params from vdsm.conf.

Anyway, acking since I don't feel strongly enough about it.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 2: I would prefer that you didn't submit this

having the static config file option override run-time, server-specific (though not host-specific) option is a very peculiar semantics.

Do you really want this behavior?
Why is this patch needed? to override timeout, retrans, and version passed by Engine? or to add more exotic options?

I'm not nacking it, just trying to understand the use case.

Maybe you would like to add/override mount options specifically.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi

Greg Padgett has posted comments on this change.

Change subject: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 2:

IIUC, the original BZ was written on overriding the option 'vers=4' to instead be 'vers=3'.  It seemed to me to be a general request to make the nfs_mount_options field in vdsm.conf function again.

Of course today you can change this in Engine.  I would think that customizing options on a per-storage-target basis would be needed much more often than changing it for a particular host.  I don't know the rationale behind host-based customization except possibly very rare corner cases.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi

Ayal Baron has posted comments on this change.

Change subject: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 2: I would prefer that you didn't submit this

I take it back. engine run time per domain configuration should take precedence over config.
Reason we need config support to begin with is backwards compatibility (user set config, did not set in engine, now it no longer works).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi

Saggi Mizrahi has posted comments on this change.

Change subject: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 2: Looks good to me, but someone else must approve

(1 inline comment)

Please add the comment if you do another iteration.

....................................................
File vdsm/storage/storageServer.py
Line 246:         # Options defined in vdsm.conf override those from engine
please add
FIXME: Remove when deprecated

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

Greg Padgett has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 3:

This is certainly a more involved set of changes, but it follows Ayal's suggestion about how to prioritize the settings (engine first, then vdsm.conf, then defaults).  If the engine passes some--but not all--options, the defaults will "fill in the blanks" just like the existing code does.

The code tries to grab the values for retrans, timeo, and vers from the vdsm.conf setting (if used) and store the values in the class.  If any are missing, the respective class properties will be 'None'.

Thanks in advance for any feedback.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

Saggi Mizrahi has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 3: Looks good to me, but someone else must approve

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

Saggi Mizrahi has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 4: Looks good to me, but someone else must approve

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

Shu Ming has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 4: Looks good to me, but someone else must approve

I think it will better to have some code to check the version of VDSM like:

if (get_vdsm_ver() < 4.0)
    self.log("Warning..."
else
    raise exception_not_supported

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

Ayal Baron has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 4: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File vdsm/storage/storageServer.py
Line 255:                     "4.0 of RHEV. Please upgrade domains to V2 or greater and "
s/V2/V3/

Line 265:                     self.__dict__[optMap[parts[0]]] = parts[1]
instead of messing around with the properties:
add this at start of method:
opts = {'retrans': None, 'version': None, 'timeout': None}
then:
if parts[0] in opts:
 opts[parts[0]] = parts[1]

then move assignment to self._timeout and friends to end of method
self._timeout = opts['timeout']...

Line 269:                 timeout = 600;
this would change to opts['timeout']
etc

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 5: I would prefer that you didn't submit this

(4 inline comments)

....................................................
File vdsm/storage/storageServer.py
Line 252:             options = ("".join(config.get('irs', 'nfs_mount_options').split())).split(",")
long line

Line 253:             self._log.warning("Using deprecated nfs_mount_options parameter "
a thought: instead of this long story in the log, report this fact as a new 'alert' for this storage domain.

See StoragePool.getInfo(). it may be a pain to implement, though.

Line 262:                 parts = opt.split("=")
this assumes that all nfs options look like key=value. `man nfs` claims this is not the case.

Line 278:         self._timeout = opts['timeo'];
*semicolon*!? are we C?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

Greg Padgett has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 5: (4 inline comments)

Dan, thanks.  I uploaded patch set 6, which addresses some of the issues.  I'll mark it -1 as a WIP while looking into a storage domain alert.

....................................................
File vdsm/storage/storageServer.py
Line 252:             options = ("".join(config.get('irs', 'nfs_mount_options').split())).split(",")
Done

Line 253:             self._log.warning("Using deprecated nfs_mount_options parameter "
Thanks for the idea, I am looking into it.

Line 262:                 parts = opt.split("=")
True, even the default options of "soft,nosharecache" do not follow the key=value convention.  However IIUC, Python's str.split() will happy return a one-element list if it splits a string that contains no separator.  (AFAICT, The documentation is silent on this particular point, but it is corroborated by the legality of maxsplit=0.)

Line 278:         self._timeout = opts['timeo'];
Yes, that reflex makes for some gross Python code, my apologies.  Fixed all 5 places.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

Greg Padgett has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 6: I would prefer that you didn't submit this

WIP, as per Dan's comments on patch 5, looking into adding a storage domain alert rather than the long warning in the log when the vdsm.conf nfs_mount_options are used.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 7:

Build Successful

http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/784/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

Greg Padgett has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 7: Verified

Rebased and verified that this works as it was written, with the message in the log.  A patch that sends an alert (and an accompanying engine patch) is forthcoming as soon as I clean it up a bit.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 8:

Build Successful

http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/797/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

Greg Padgett has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 8: Verified

This patch sends an alert to the engine, as suggested by Dan.

Any ideas on how to make it cleaner are welcome--it felt like a bit of a hack putting the code to propagate status from the NFS connection to the storage pools right in the middle of the HSM class.  However, I didn't see any other place with the ability to do this.  Also, I'm not sure if there are thread safety issues here as I don't know the architecture quite well enough to know what needs to be synchronized versus what already is.

Also see engine patch for alert text at http://gerrit.ovirt.org/7658

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

Greg Padgett has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 9: Verified

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 9:

Build Successful

http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/813/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

Ayal Baron has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 9: I would prefer that you didn't submit this

(4 inline comments)

....................................................
Commit Message
Line 7: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
Line 8: 
Line 9: The nfs_mount_options configuration setting in vdsm.conf was ignored.
Line 10: 
Line 11: If the engine doesn't pass any mount options, and nfs_mount_options are
Now that you've introduced the patch to make V3 the default, does engine not pass V3 by default? (effectively always overriding vdsm.conf options?)
Line 12: present (and not the defaults), then use the options from the config
Line 13: file.  Otherwise, use whatever the engine passes and/or defaults as was
Line 14: done previously.
Line 15:

....................................................
File vdsm/storage/hsm.py
Line 2143: 
Line 2144:     def _getNFSDomainToConInfoMap(self):
Line 2145:         dirConMap = {}  # directory -> connectionInfo
Line 2146:         for conInfo in [conInfo for conInfo in self._connectionList
Line 2147:                         if isinstance(conInfo, storageServer.NFSConnection)]:
this should be determined once when connecting, not every time getStoragePoolInfo is run (every 10s).
Value for getStoragePoolInfo should basically be static.
Line 2148:             path = conInfo.mountPath
Line 2149:             if path is not None:
Line 2150:                 dirConMap[path] = conInfo
Line 2151:

Line 2149:             if path is not None:
Line 2150:                 dirConMap[path] = conInfo
Line 2151: 
Line 2152:         sdConMap = {}  # sdUUID -> connectionInfo
Line 2153:         for sdUUID, domDir in fileSD.scanDomains():
running scanDomains every 10s is a catastrophe and totally redundant, as we already know what to check when connecting to the specific nfs server *and* only need to do so if the non default values in vdsm.conf are used...
Line 2154:             try:
Line 2155:                 dom = sdCache.produce(sdUUID=sdUUID)
Line 2156:                 if dom.getStorageType() == sd.NFS_DOMAIN:
Line 2157:                     domMount = domDir[:(domDir.rfind("/"))]  # strip off UUID

Line 2151: 
Line 2152:         sdConMap = {}  # sdUUID -> connectionInfo
Line 2153:         for sdUUID, domDir in fileSD.scanDomains():
Line 2154:             try:
Line 2155:                 dom = sdCache.produce(sdUUID=sdUUID)
producing a domain is a potentially heavy operation which can also hang in case of nfs problems and you already know if your mount is nfs or not when connecting.
Seeing as the configuration is not limited to a specific domain but rather to vdsm in general (any nfs domain would get these params) there is no need for all of this.
Line 2156:                 if dom.getStorageType() == sd.NFS_DOMAIN:
Line 2157:                     domMount = domDir[:(domDir.rfind("/"))]  # strip off UUID
Line 2158:                     if domMount in dirConMap:
Line 2159:                         sdConMap[sdUUID] = dirConMap[domMount]

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf

Ayal Baron has posted comments on this change.

Change subject: BZ#826921: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 9:

Come to think of it, this should probably not be reproted in getStoragePoolInfo at all, rather in getVdsCapabilities which is called once every restart of vdsm by engine.
This is about host configuration.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

Greg Padgett has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 10: Verified

This works alongside the changes here:
http://gerrit.ovirt.org/8798 (and its parents for bz855729)
http://gerrit.ovirt.org/7658 (engine changes for this bug)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

Shu Ming has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 10: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File vdsm/storage/storage_exception.py
Line 173:     code = 2008
Line 174:     message = "Directory cleanup failure"
Line 175: 
Line 176: 
Line 177: class DeprecatedNFSOptions(StorageException):
The error code "200x" should be misc exception".  To conform the style of the above,  It is better to name the class as "MiscDeprecatedNFSOptions".
Line 178:     def __init__(self, options):
Line 179:         self.value = "options=%s" % (options,)
Line 180:     code = 2009
Line 181:     message = "Using deprecated nfs_mount_options parameter from vdsm.conf."\

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

Greg Padgett has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 11: Verified

Updated name of exception class

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 11: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File vdsm/caps.py
Line 296:             config.getint('vars', 'host_mem_reserve') +
Line 297:             config.getint('vars', 'extra_mem_reserve'))
Line 298:     caps['guestOverhead'] = config.get('vars', 'guest_ram_overhead')
Line 299: 
Line 300:     caps['alerts'] = _getAlerts()
any change to the api must be accompanied with a compatible change to vdsm_api/vdsmapi-schema.json
Line 301: 
Line 302:     return caps
Line 303: 
Line 304:

Line 301: 
Line 302:     return caps
Line 303: 
Line 304: 
Line 305: def _getAlerts():
we tried hard to keep storage-specific know-how to the storage package. How about hiding this logic in a storage.getAlerts() functions? (some would say that it needs to be a declared hsm verb).
Line 306:     alerts = []
Line 307: 
Line 308:     # FIXME: Remove when nfs_mount_options is no longer supported
Line 309:     nfs_config = config.get('irs', 'nfs_mount_options')

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

Greg Padgett has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 12: Verified

Moved alert logic into vdsm/storage/alerts.py (new file).  If there is a better place to put it, I'll be happy to do so.

Also added new elements to json schema.

Still functionally the same as before.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

Shu Ming has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 12: Looks good to me, but someone else must approve

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 12: I would prefer that you didn't submit this

(6 inline comments)

only minor comments

....................................................
File vdsm/storage/alerts.py
Line 28:     alerts = []
Line 29: 
Line 30:     # FIXME: Remove when nfs_mount_options is no longer supported
Line 31:     nfs_config = config.get('irs', 'nfs_mount_options')
Line 32:     if nfs_config != ",".join(NFSConnection.DEFAULT_OPTIONS):
nit: I do not think the order of options matters, so a better approach would be to split nfs_config, cast into a frozenset, and then compare.
Line 33:         alert = se.MiscDeprecatedNFSOptions(nfs_config)
Line 34:         alerts.append({'code': alert.code,
Line 35:                        'message': alert.message,
Line 36:                        'value': alert.value})

Line 33:         alert = se.MiscDeprecatedNFSOptions(nfs_config)
Line 34:         alerts.append({'code': alert.code,
Line 35:                        'message': alert.message,
Line 36:                        'value': alert.value})
Line 37:         logging.warning("%s (%s)" % (alert.message, alert.value))
we try to avoid formatting if the log level is not low enough by

logging.warning("%s (%s)", alert.message, alert.value)
Line 38:

....................................................
File vdsm/storage/storageServer.py
Line 34: import fileSD
Line 35: import iscsi
Line 36: from sync import asyncmethod, AsyncCallStub
Line 37: from mount import MountError
Line 38: from vdsm.config import config
Saggi is going to be very angry about inroducing a new short-circuit into vdsm.config.

I suppose that I'm fine with this, as this is here only for backward compatibility of manually-configured hosts.
Line 39: import storage_exception as se
Line 40: 
Line 41: 
Line 42: class AliasAlreadyRegisteredError(RuntimeError):

Line 292:         if ((timeout, retrans, version) == (None, None, None) and
Line 293:                 conf_options != ",".join(options)):
Line 294:             # Overwrite defaults with config, then parse them and store in
Line 295:             # the opts dict so that they're stored as properties below.
Line 296:             self._log.warning("Using deprecated nfs_mount_options from"\
redundant trailing backslash (f18's pep8 hates this)
Line 297:                     " vdsm.conf to mount %s: %s", export, conf_options)
Line 298:             options = ("".join(conf_options.split())).split(",")
Line 299:             for opt in options:
Line 300:                 parts = opt.split("=")

Line 294:             # Overwrite defaults with config, then parse them and store in
Line 295:             # the opts dict so that they're stored as properties below.
Line 296:             self._log.warning("Using deprecated nfs_mount_options from"\
Line 297:                     " vdsm.conf to mount %s: %s", export, conf_options)
Line 298:             options = ("".join(conf_options.split())).split(",")
I find

conf_options.replace(' ', '')

or

opt = opt.strip()

as less confusing ways of removing spaces.
Line 299:             for opt in options:
Line 300:                 parts = opt.split("=")
Line 301:                 if parts[0] in opts:
Line 302:                     opts[parts[0]] = parts[1]

Line 312:             _addIntegerOption(options, "timeo", opts['timeo'])
Line 313:             _addIntegerOption(options, "retrans", opts['retrans'])
Line 314:             if opts['vers'] != 'auto':
Line 315:                 _addIntegerOption(options, "vers", opts['vers'])
Line 316:             self._deprecatedNFSOptions = None
is anyone using this "_deprecatedNFSOptions"?
Line 317: 
Line 318:         self._remotePath = normpath(export)
Line 319:         self._timeout = opts['timeo']
Line 320:         self._retrans = opts['retrans']

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

Saggi Mizrahi has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 12: I would prefer that you didn't submit this

You shouldn't read config in places other then hsm or clientif

This isn't integral to the infra but rather a compatibility feature so it should sit in the compatibility layer in hsm

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

Greg Padgett has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 13: Verified

This change was rebased on top of http://gerrit.ovirt.org/8573 -- changes to apply nfs_mount_options to the connection are all done in the hsm.py compatibility layer, using a posix connection which accepts arbitrary mount options.  Addressed other nits as well.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 13: I would prefer that you didn't submit this

(4 inline comments)

I have only one comment about set comparison, and one observations about the suggested implementation which requires closer scrutiny by someone familiar with nfs/posixfs differentiation.

....................................................
File vdsm/storage/alerts.py
Line 28:     alerts = []
Line 29: 
Line 30:     # FIXME: Remove when nfs_mount_options is no longer supported
Line 31:     nfs_config = config.get('irs', 'nfs_mount_options').replace(' ', '')
Line 32:     if len(frozenset(nfs_config.split(',')) ^
the equality sign could have been clearer.
Line 33:             frozenset(NFSConnection.DEFAULT_OPTIONS)) > 0:
Line 34:         alert = se.MiscDeprecatedNFSOptions(nfs_config)
Line 35:         alerts.append({'code': alert.code,
Line 36:                        'message': alert.message,

....................................................
File vdsm/storage/hsm.py
Line 159:                 conDict.get('retrans', None),
Line 160:                 conDict.get('timeout', None)) == (None, None, None):
Line 161:             conf_options = (config.get('irs', 'nfs_mount_options')
Line 162:                     .replace(' ', ''))
Line 163:             if len(frozenset(conf_options.split(',')) ^ frozenset(
==
Line 164:                     storageServer.NFSConnection.DEFAULT_OPTIONS)) > 0:
Line 165:                 logging.warning("Using deprecated nfs_mount_options from"
Line 166:                         " vdsm.conf to mount %s: %s",
Line 167:                         conDict.get('connection', '(unknown)'), conf_options)

Line 177:                 conDict.get('connection', None))
Line 178:     elif typeName == 'nfs':
Line 179:         params = tryDeprecatedNfsParams(conDict)
Line 180:         if params is not None:
Line 181:             # Hack to support vdsm.conf nfs_mount_options
oh, so using nfs with nfs_mount_options, is interpreted as posixfs. do we really want that? if I recalled correctly we had ideas that nfs would have specific stuff beyond posixfs.
Line 182:             typeName = 'posixfs'
Line 183:         else:
Line 184:             version = conDict.get('protocol_version', "3")
Line 185:             version = str(version)

....................................................
File vdsm/storage/storage_exception.py
Line 173:     code = 2008
Line 174:     message = "Directory cleanup failure"
Line 175: 
Line 176: 
Line 177: class MiscDeprecatedNFSOptions(StorageException):
Shu Ming, I believe that your comment was not correct. Misc* is related to exceptions raised in the misc.py module, unrelated to the error code.

I personally do not care much about the name, though.
Line 178:     def __init__(self, options):
Line 179:         self.value = "options=%s" % (options,)
Line 180:     code = 2009
Line 181:     message = "Using deprecated nfs_mount_options parameter from vdsm.conf."\

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

Greg Padgett has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 13: (4 inline comments)

Thanks Dan, replies inline, and agreed with respect to additional scrutiny needed for the posix/nfs issue.

....................................................
File vdsm/storage/alerts.py
Line 28:     alerts = []
Line 29: 
Line 30:     # FIXME: Remove when nfs_mount_options is no longer supported
Line 31:     nfs_config = config.get('irs', 'nfs_mount_options').replace(' ', '')
Line 32:     if len(frozenset(nfs_config.split(',')) ^
np, I'll change it to != when rebasing
Line 33:             frozenset(NFSConnection.DEFAULT_OPTIONS)) > 0:
Line 34:         alert = se.MiscDeprecatedNFSOptions(nfs_config)
Line 35:         alerts.append({'code': alert.code,
Line 36:                        'message': alert.message,

....................................................
File vdsm/storage/hsm.py
Line 159:                 conDict.get('retrans', None),
Line 160:                 conDict.get('timeout', None)) == (None, None, None):
Line 161:             conf_options = (config.get('irs', 'nfs_mount_options')
Line 162:                     .replace(' ', ''))
Line 163:             if len(frozenset(conf_options.split(',')) ^ frozenset(
Same as alerts.py, I'll change this one as well
Line 164:                     storageServer.NFSConnection.DEFAULT_OPTIONS)) > 0:
Line 165:                 logging.warning("Using deprecated nfs_mount_options from"
Line 166:                         " vdsm.conf to mount %s: %s",
Line 167:                         conDict.get('connection', '(unknown)'), conf_options)

Line 177:                 conDict.get('connection', None))
Line 178:     elif typeName == 'nfs':
Line 179:         params = tryDeprecatedNfsParams(conDict)
Line 180:         if params is not None:
Line 181:             # Hack to support vdsm.conf nfs_mount_options
It was between this or giving NFSConnection.__init__ an "options" param for general options (Saggi didn't like that too much).  I tested by switching options back and forth for the same sd, mounting on multiple hosts (one with sd mounted with nfs, one via posix), etc. and it seems to work /today/ ... realizing of course that the future compatibility may be less certain.
Line 182:             typeName = 'posixfs'
Line 183:         else:
Line 184:             version = conDict.get('protocol_version', "3")
Line 185:             version = str(version)

....................................................
File vdsm/storage/storage_exception.py
Line 173:     code = 2008
Line 174:     message = "Directory cleanup failure"
Line 175: 
Line 176: 
Line 177: class MiscDeprecatedNFSOptions(StorageException):
I looked for a "storage general exception" category, but misc was the best I could come up with (didn't know it was for misc.py).  If anyone is concerned enough about its naming/placement I'm open to suggestions of where to move it.
Line 178:     def __init__(self, options):
Line 179:         self.value = "options=%s" % (options,)
Line 180:     code = 2009
Line 181:     message = "Using deprecated nfs_mount_options parameter from vdsm.conf."\

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

Saggi Mizrahi has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 13: I would prefer that you didn't submit this

Please remove all the "alerts" bits.
It changes the interface and I'm not sure that we want to support alerts in that way.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 13:

Saggi, I agree that reporting alerts is something more general than solving this bug, and thus warrants a separate patch.

However, now is the time to express the reasons why you are ambivalent about the suggested API.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 13: (1 inline comment)

....................................................
File vdsm/storage/storage_exception.py
Line 173:     code = 2008
Line 174:     message = "Directory cleanup failure"
Line 175: 
Line 176: 
Line 177: class MiscDeprecatedNFSOptions(StorageException):
only now did I notice Shu Ming's http://gerrit.ovirt.org/8847 about exception naming. oh well, I might have been wrong. nm.
Line 178:     def __init__(self, options):
Line 179:         self.value = "options=%s" % (options,)
Line 180:     code = 2009
Line 181:     message = "Using deprecated nfs_mount_options parameter from vdsm.conf."\

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

Greg Padgett has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 14:

Removed the alert.  I'd like to come to a consensus on this before coding/testing/submitting more patch sets if we can.  For the history of the alert, see comments on storageServer.py in patch set 5, cover message comments to patch set 9, and caps.py comments on patch set 11.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 14: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File vdsm/storage/hsm.py
Line 159:                 conDict.get('retrans', None),
Line 160:                 conDict.get('timeout', None)) == (None, None, None):
Line 161:             conf_options = (config.get('irs', 'nfs_mount_options')
Line 162:                     .replace(' ', ''))
Line 163:             if len(frozenset(conf_options.split(',')) ^ frozenset(
Only now do I see that I was misunderstood. All I suggested was a clearer condition

if frozenset(conf_options.split(',')) != frozenset(storageServer.NFSConnection.DEFAULT_OPTIONS))

which I'd prefer over the xor.
Line 164:                     storageServer.NFSConnection.DEFAULT_OPTIONS)) != 0:
Line 165:                 logging.warning("Using deprecated nfs_mount_options from"
Line 166:                         " vdsm.conf to mount %s: %s",
Line 167:                         conDict.get('connection', '(unknown)'), conf_options)

Line 177:                 conDict.get('connection', None))
Line 178:     elif typeName == 'nfs':
Line 179:         params = tryDeprecatedNfsParams(conDict)
Line 180:         if params is not None:
Line 181:             # Hack to support vdsm.conf nfs_mount_options
still now word from the experts about this :-(
Line 182:             typeName = 'posixfs'
Line 183:         else:
Line 184:             version = conDict.get('protocol_version', "3")
Line 185:             version = str(version)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 15: Looks good to me, approved

I hope you do not mind that I've fixed the set comparison thingy myself, and documented in the commit message the fact that we use posixfs for the non-standard soon-to-be-removed vdsm.conf-modified options.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

Patch Set 15: Verified

I've verified the submitted code with several values for nfs_mount_options. Seems fine.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: storage: Don't ignore nfs_mount_options in vdsm.conf

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: storage: Don't ignore nfs_mount_options in vdsm.conf
......................................................................

storage: Don't ignore nfs_mount_options in vdsm.conf

The nfs_mount_options configuration setting in vdsm.conf was ignored.

If the engine doesn't pass any mount options, and nfs_mount_options are
present (and not the defaults), then use the options from the config
file, as options to 'posixfs' connection. Otherwise, use whatever the
engine passes and/or defaults as was done previously.

Bug-Url: https://bugzilla.redhat.com/826921
Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Signed-off-by: Greg Padgett 
---
M vdsm/storage/fileUtils.py
M vdsm/storage/hsm.py
2 files changed, 36 insertions(+), 14 deletions(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I3c1d13bfd98a9b41b9728bdcb7f01b3161c26bd8
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server