Changes since V2: - Rename mountToRemotePath() to getRealPath() and define it in each child class.
Changes since V1: - Derive the remotePath from self.mountpoint instead of using the metadata
The current method for gathering a LOCALFS Storage Domain's remotePath property does not work because these domains are connected with a symlink, not a mount. Fix up the current code so that it handles links and mountpoints.
In the code I have noticed some sentiments that path information should be removed from the storage domain metadata. I strongly disagree with this idea. The path is a critical piece of information. End users will care a lot about the path because it is where their images are located. This informaton is also useful for calling connectStorageServer() and disconnectStorageServer().
Signed-off-by: Adam Litke agl@us.ibm.com --- vdsm/storage/fileSD.py | 16 ++++++++-------- vdsm/storage/localFsSD.py | 3 +++ vdsm/storage/nfsSD.py | 6 ++++++ 3 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/vdsm/storage/fileSD.py b/vdsm/storage/fileSD.py index 35f7ab3..5058b89 100644 --- a/vdsm/storage/fileSD.py +++ b/vdsm/storage/fileSD.py @@ -24,7 +24,6 @@ import logging import glob
import sd -import fileUtils import storage_exception as se import fileVolume import image @@ -224,6 +223,13 @@ class FileStorageDomain(sd.StorageDomain): def getRemotePath(self): return self.remotePath
+ def getRealPath(self): + """ + Return the actual path to the underlying storage. + This function needs to be overloaded by the child classes. + """ + return "" + def getInfo(self): """ Get storage domain info @@ -232,13 +238,7 @@ class FileStorageDomain(sd.StorageDomain): # First call parent getInfo() - it fills in all the common details info = sd.StorageDomain.getInfo(self) # Now add fileSD specific data - info['remotePath'] = '' - mounts = fileUtils.getMounts() - for mount in mounts: - if self.mountpoint == mount[1]: - info['remotePath'] = mount[0] - break - + info['remotePath'] = self.getRealPath() return info
def getStats(self): diff --git a/vdsm/storage/localFsSD.py b/vdsm/storage/localFsSD.py index 6a61979..e07426e 100644 --- a/vdsm/storage/localFsSD.py +++ b/vdsm/storage/localFsSD.py @@ -93,5 +93,8 @@ class LocalFsStorageDomain(fileSD.FileStorageDomain):
raise se.StorageDomainDoesNotExist(sdUUID)
+ def getRealPath(self): + return os.readlink(self.mountpoint) + def findDomain(sdUUID): return LocalFsStorageDomain(LocalFsStorageDomain.findDomainPath(sdUUID)) diff --git a/vdsm/storage/nfsSD.py b/vdsm/storage/nfsSD.py index 51dc31f..d4b2759 100644 --- a/vdsm/storage/nfsSD.py +++ b/vdsm/storage/nfsSD.py @@ -138,6 +138,12 @@ class NfsStorageDomain(fileSD.FileStorageDomain):
raise se.StorageDomainDoesNotExist(sdUUID)
+ def getRealPath(self): + for mount in fileUtils.getMounts(): + if self.mountpoint == mount[1]: + return mount[0] + return "" + def findDomain(sdUUID): return NfsStorageDomain(NfsStorageDomain.findDomainPath(sdUUID))
Looks good to me. +1
----- Original Message -----
Changes since V2:
- Rename mountToRemotePath() to getRealPath() and define it in each
child class.
Changes since V1:
- Derive the remotePath from self.mountpoint instead of using the
metadata
The current method for gathering a LOCALFS Storage Domain's remotePath property does not work because these domains are connected with a symlink, not a mount. Fix up the current code so that it handles links and mountpoints.
In the code I have noticed some sentiments that path information should be removed from the storage domain metadata. I strongly disagree with this idea. The path is a critical piece of information. End users will care a lot about the path because it is where their images are located. This informaton is also useful for calling connectStorageServer() and disconnectStorageServer().
Signed-off-by: Adam Litke agl@us.ibm.com
vdsm/storage/fileSD.py | 16 ++++++++-------- vdsm/storage/localFsSD.py | 3 +++ vdsm/storage/nfsSD.py | 6 ++++++ 3 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/vdsm/storage/fileSD.py b/vdsm/storage/fileSD.py index 35f7ab3..5058b89 100644 --- a/vdsm/storage/fileSD.py +++ b/vdsm/storage/fileSD.py @@ -24,7 +24,6 @@ import logging import glob
import sd -import fileUtils import storage_exception as se import fileVolume import image @@ -224,6 +223,13 @@ class FileStorageDomain(sd.StorageDomain): def getRemotePath(self): return self.remotePath
- def getRealPath(self):
"""
Return the actual path to the underlying storage.
This function needs to be overloaded by the child classes.
"""
return ""
- def getInfo(self): """ Get storage domain info
@@ -232,13 +238,7 @@ class FileStorageDomain(sd.StorageDomain): # First call parent getInfo() - it fills in all the common details info = sd.StorageDomain.getInfo(self) # Now add fileSD specific data
info['remotePath'] = ''
mounts = fileUtils.getMounts()
for mount in mounts:
if self.mountpoint == mount[1]:
info['remotePath'] = mount[0]
break
info['remotePath'] = self.getRealPath() return info
def getStats(self):
diff --git a/vdsm/storage/localFsSD.py b/vdsm/storage/localFsSD.py index 6a61979..e07426e 100644 --- a/vdsm/storage/localFsSD.py +++ b/vdsm/storage/localFsSD.py @@ -93,5 +93,8 @@ class LocalFsStorageDomain(fileSD.FileStorageDomain):
raise se.StorageDomainDoesNotExist(sdUUID)
- def getRealPath(self):
return os.readlink(self.mountpoint)
def findDomain(sdUUID): return LocalFsStorageDomain(LocalFsStorageDomain.findDomainPath(sdUUID)) diff --git a/vdsm/storage/nfsSD.py b/vdsm/storage/nfsSD.py index 51dc31f..d4b2759 100644 --- a/vdsm/storage/nfsSD.py +++ b/vdsm/storage/nfsSD.py @@ -138,6 +138,12 @@ class NfsStorageDomain(fileSD.FileStorageDomain):
raise se.StorageDomainDoesNotExist(sdUUID)
- def getRealPath(self):
for mount in fileUtils.getMounts():
if self.mountpoint == mount[1]:
return mount[0]
return ""
def findDomain(sdUUID): return NfsStorageDomain(NfsStorageDomain.findDomainPath(sdUUID))
-- 1.7.6
-- Adam Litke agl@us.ibm.com IBM Linux Technology Center _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Well done Adam. ACK,
Dan, this patch also solve our: Bug 735621 - getStorageDomainInfo for local domains doesn't return 'remote path ' So, maybe we need mention it in the commit message
Igor
----- Original Message ----- From: "Adam Litke" agl@us.ibm.com To: vdsm-patches@lists.fedorahosted.org Sent: Friday, September 30, 2011 12:20:22 AM Subject: [PATCH] fileSD: Fix remotePath in SD metadata (V3)
Changes since V2: - Rename mountToRemotePath() to getRealPath() and define it in each child class.
Changes since V1: - Derive the remotePath from self.mountpoint instead of using the metadata
The current method for gathering a LOCALFS Storage Domain's remotePath property does not work because these domains are connected with a symlink, not a mount. Fix up the current code so that it handles links and mountpoints.
In the code I have noticed some sentiments that path information should be removed from the storage domain metadata. I strongly disagree with this idea. The path is a critical piece of information. End users will care a lot about the path because it is where their images are located. This informaton is also useful for calling connectStorageServer() and disconnectStorageServer().
Signed-off-by: Adam Litke agl@us.ibm.com --- vdsm/storage/fileSD.py | 16 ++++++++-------- vdsm/storage/localFsSD.py | 3 +++ vdsm/storage/nfsSD.py | 6 ++++++ 3 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/vdsm/storage/fileSD.py b/vdsm/storage/fileSD.py index 35f7ab3..5058b89 100644 --- a/vdsm/storage/fileSD.py +++ b/vdsm/storage/fileSD.py @@ -24,7 +24,6 @@ import logging import glob
import sd -import fileUtils import storage_exception as se import fileVolume import image @@ -224,6 +223,13 @@ class FileStorageDomain(sd.StorageDomain): def getRemotePath(self): return self.remotePath
+ def getRealPath(self): + """ + Return the actual path to the underlying storage. + This function needs to be overloaded by the child classes. + """ + return "" + def getInfo(self): """ Get storage domain info @@ -232,13 +238,7 @@ class FileStorageDomain(sd.StorageDomain): # First call parent getInfo() - it fills in all the common details info = sd.StorageDomain.getInfo(self) # Now add fileSD specific data - info['remotePath'] = '' - mounts = fileUtils.getMounts() - for mount in mounts: - if self.mountpoint == mount[1]: - info['remotePath'] = mount[0] - break - + info['remotePath'] = self.getRealPath() return info
def getStats(self): diff --git a/vdsm/storage/localFsSD.py b/vdsm/storage/localFsSD.py index 6a61979..e07426e 100644 --- a/vdsm/storage/localFsSD.py +++ b/vdsm/storage/localFsSD.py @@ -93,5 +93,8 @@ class LocalFsStorageDomain(fileSD.FileStorageDomain):
raise se.StorageDomainDoesNotExist(sdUUID)
+ def getRealPath(self): + return os.readlink(self.mountpoint) + def findDomain(sdUUID): return LocalFsStorageDomain(LocalFsStorageDomain.findDomainPath(sdUUID)) diff --git a/vdsm/storage/nfsSD.py b/vdsm/storage/nfsSD.py index 51dc31f..d4b2759 100644 --- a/vdsm/storage/nfsSD.py +++ b/vdsm/storage/nfsSD.py @@ -138,6 +138,12 @@ class NfsStorageDomain(fileSD.FileStorageDomain):
raise se.StorageDomainDoesNotExist(sdUUID)
+ def getRealPath(self): + for mount in fileUtils.getMounts(): + if self.mountpoint == mount[1]: + return mount[0] + return "" + def findDomain(sdUUID): return NfsStorageDomain(NfsStorageDomain.findDomainPath(sdUUID))
On Sun, Oct 02, 2011 at 02:00:04AM -0400, Igor Lvovsky wrote:
Well done Adam. ACK,
Dan, this patch also solve our: Bug 735621 - getStorageDomainInfo for local domains doesn't return 'remote path ' So, maybe we need mention it in the commit message
Speaking about commit message, the following part of the message should be dropped. I think we agreed that the path is a critical piece of information that should be reported, but should not be recorded in the domain metadata.
In the code I have noticed some sentiments that path information should be removed from the storage domain metadata. I strongly disagree with this idea. The path is a critical piece of information. End users will care a lot about the path because it is where their images are located. This informaton is also useful for calling connectStorageServer() and disconnectStorageServer().
Fine by you, Adam?
Dan.
On Sun, Oct 02, 2011 at 09:55:47AM +0200, Dan Kenigsberg wrote:
On Sun, Oct 02, 2011 at 02:00:04AM -0400, Igor Lvovsky wrote:
Well done Adam. ACK,
Dan, this patch also solve our: Bug 735621 - getStorageDomainInfo for local domains doesn't return 'remote path ' So, maybe we need mention it in the commit message
Speaking about commit message, the following part of the message should be dropped. I think we agreed that the path is a critical piece of information that should be reported, but should not be recorded in the domain metadata.
In the code I have noticed some sentiments that path information should be removed from the storage domain metadata. I strongly disagree with this idea. The path is a critical piece of information. End users will care a lot about the path because it is where their images are located. This informaton is also useful for calling connectStorageServer() and disconnectStorageServer().
Fine by you, Adam?
Yes, agreed.
On Mon, Oct 03, 2011 at 07:58:35AM -0500, Adam Litke wrote:
On Sun, Oct 02, 2011 at 09:55:47AM +0200, Dan Kenigsberg wrote:
On Sun, Oct 02, 2011 at 02:00:04AM -0400, Igor Lvovsky wrote:
Well done Adam. ACK,
Dan, this patch also solve our: Bug 735621 - getStorageDomainInfo for local domains doesn't return 'remote path ' So, maybe we need mention it in the commit message
Speaking about commit message, the following part of the message should be dropped. I think we agreed that the path is a critical piece of information that should be reported, but should not be recorded in the domain metadata.
In the code I have noticed some sentiments that path information should be removed from the storage domain metadata. I strongly disagree with this idea. The path is a critical piece of information. End users will care a lot about the path because it is where their images are located. This informaton is also useful for calling connectStorageServer() and disconnectStorageServer().
Fine by you, Adam?
Yes, agreed.
Thanks, pushed.
vdsm-patches@lists.fedorahosted.org