Change in vdsm[master]: blockSD: Avoid stale lvs

nsoffer at redhat.com nsoffer at redhat.com
Mon May 2 17:07:29 UTC 2016


Nir Soffer has posted comments on this change.

Change subject: blockSD: Avoid stale lvs
......................................................................


Patch Set 3:

(3 comments)

https://gerrit.ovirt.org/#/c/56876/3/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 653:     for vg in _lvminfo.getAllVgs():
Line 654:         deactivateUnusedLVs(vg.name, refreshlvs=refreshlvs)
Line 655: 
Line 656: 
Line 657: def deactivateUnusedLVs(vgname, refreshlvs=()):
> refreshlvs is a confusing parameter name.  To me it suggests that these lvs
This is the same name used in bootrap(), I'm reusing both the code and the interface.

I agree that the name does not explain that these lvs are not deactivated, but excludelvs or keeplvs does not explain that the lvs will be refreshed, so we better keep the current interface for now. If we find a better name, we should change both functions.

I will document this function (same as bootstrap) to make this clear:

    Ensure that all unused lvs are deactivated, except lvs
    matching refreshlvs, which are refreshed instead.
Line 658:     deactivate = []
Line 659:     refresh = []
Line 660: 
Line 661:     for lv in _lvminfo.getLv(vgname):


Line 672:         try:
Line 673:             _setLVAvailability(vgname, deactivate, "n")
Line 674:         except se.CannotDeactivateLogicalVolume:
Line 675:             log.error("Error deactivating lvs: vg=%s lvs=%s", vgname,
Line 676:                       deactivate)
> Since stale lvs could lead to data corruption, should this cause a failure 
stale lvs can lead to data corruption if lv was removed on storage, and we start a vm or do write into it on the host where it was active. Since we do not do that, I don't think we should go that far and disable a domain because of temporary lvm failure.

You can use the same argument for bootstrap - when we added it we intentionally logged error and continue if some lvs could not be deactivated or refreshed.
Line 677:         # Some lvs are inactive now
Line 678:         _lvminfo._invalidatelvs(vgname, deactivate)
Line 679: 
Line 680:     if refresh:


https://gerrit.ovirt.org/#/c/56876/3/vdsm/storage/sd.py
File vdsm/storage/sd.py:

Line 503:     # Life cycle
Line 504: 
Line 505:     def setup(self):
Line 506:         """
Line 507:         Called when after storage domain is produced in the storage domain
> Called after
Thanks, will fix.
Line 508:         monitor.
Line 509:         """
Line 510: 
Line 511:     def teardown(self):


-- 
To view, visit https://gerrit.ovirt.org/56876
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7227bb43c2e1ee67a6239956aae48173a27f566e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Ala Hino <ahino at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Simone Tiraboschi <stirabos at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list