Nir Soffer has posted comments on this change. Change subject: storage: add more info to NFS SD getInfo ......................................................................
Patch Set 2: Code-Review-1 (4 comments) https://gerrit.ovirt.org/#/c/63636/2//COMMIT_MSG Commit Message: Line 19: 'wsize=1048576', 'namlen=255', 'soft', 'nosharecache', Line 20: 'proto=tcp', 'timeo=600', 'retrans=6', 'sec=sys', Line 21: 'mountaddr=10.35.0.181', 'mountvers=3', Line 22: 'mountport=20048', 'mountproto=udp', Line 23: 'local_lock=none', 'addr=10.35.0.181'] This include options that vdsm (see stoageServer.py) adds and were not sent by engine. This may cause trouble if engine use this options to create a new storage domain. Since vdsm adds these options, I think it should also remove them when reporting mount options to engine. Otherwise, we will have to keep the list of default options in both vdsm and engine to create storage domain from existing mount. I think our goal is to restore a storage domain in engine from the existing mount in vdsm. Line 24: type = NFS Line 25: class = Data Line 26: pool = ['00000001-0001-0001-0001-000000000325'] Line 27: name = NFSSD https://gerrit.ovirt.org/#/c/63636/2/vdsm/storage/nfsSD.py File vdsm/storage/nfsSD.py: Line 128: # First call parent getInfo() - it fills in all the common details Line 129: info = fileSD.FileStorageDomain.getInfo(self) Line 130: # Now add nfsSD specific data Line 131: try: Line 132: mntrcd = mount.getMountFromTarget(self.mountpoint).getRecord() getMountFromTarget iterate over all records, getRecord will iterate again over all records so this code is not efficient. Even worse, you may get a Mont object that its getRecord will raise! since getRecord require that both fs_spec and fs_file match, but getMountFromTarget does not. We need to improve the mount infrastructure before we can depend on it for a new feature. "mntrcd" is too cryptic, "record" would be better, matching the api. Line 133: info['vfsType'] = mntrcd.fs_vfstype Line 134: info['mountOptions'] = mntrcd.fs_mntops Line 135: except mount.MountError: Line 136: return info Line 130: # Now add nfsSD specific data Line 131: try: Line 132: mntrcd = mount.getMountFromTarget(self.mountpoint).getRecord() Line 133: info['vfsType'] = mntrcd.fs_vfstype Line 134: info['mountOptions'] = mntrcd.fs_mntops These must be documented in the schema. Line 135: except mount.MountError: Line 136: return info Line 137: Line 138: return info Line 132: mntrcd = mount.getMountFromTarget(self.mountpoint).getRecord() Line 133: info['vfsType'] = mntrcd.fs_vfstype Line 134: info['mountOptions'] = mntrcd.fs_mntops Line 135: except mount.MountError: Line 136: return info Please log the error here instead of returning the info, the info is returned anyway below. Line 137: Line 138: return info Line 139: Line 140: -- To view, visit https://gerrit.ovirt.org/63636 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id54d735a43871f94684e94395b1569c54c03e8ce Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Fred Rolland <froll...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
