From 2f7931da637a98995fe001097490ad288c3a728e Mon Sep 17 00:00:00 2001 From: Cole Robinson Date: Sun, 20 Sep 2020 16:44:52 -0400 Subject: [PATCH] createvol: Drop Allocation field in favor of checkbox Inspired by some discussion from here: https://bugzilla.redhat.com/show_bug.cgi?id=1759454 Most libvirt storage volume creation doesn't actually do anything with allocation, besides interpreting cap == alloc and cap != alloc. The exceptions are zfs volumes, and raw file volumes. But it's unclear what the usecase is for the latter at all. This drops the allocation spinner and adds checkbox in its place 'Allocate entire volume now'. When enabled, it sets cap == alloc. We only show this for file volumes. For qcow2 it defaults to unselected (sparse), for all others it defaults to selected. If it's not showing, it defaults to selected. Bundled with this change is showing this field for qcow2, where we previously only allowed nonsparse here. Libvirt and qemu-img support non-sparse qcow2 these days. Signed-off-by: Cole Robinson --- tests/uitests/test_createvol.py | 27 ++++-------- ui/createvol.ui | 74 ++++++++++++++------------------ virtManager/createvol.py | 76 +++++++++------------------------ 3 files changed, 59 insertions(+), 118 deletions(-) diff --git a/tests/uitests/test_createvol.py b/tests/uitests/test_createvol.py index 251efb543..6be015866 100644 --- a/tests/uitests/test_createvol.py +++ b/tests/uitests/test_createvol.py @@ -28,15 +28,15 @@ def testCreateVolDefault(app): name = win.find("Name:", "text") # Create a default qcow2 volume - lib.utils.check(lambda: name.text == "vol") - newname = "a-newvol" - name.set_text(newname) - win.find("Max Capacity:", "spin button").set_text("10.5") + newname = "vol" + lib.utils.check(lambda: name.text == newname) + sparse = win.find("Allocate", "check box") + lib.utils.check(lambda: not sparse.checked) finish.click() # Delete it, clicking 'No' first volcell = vollist.find(newname + ".qcow2") - volcell.click() + volcell.bring_on_screen() hostwin.find("vol-refresh", "push button").click() hostwin.find("vol-delete", "push button").click() app.click_alert_button("permanently delete the volume", "No") @@ -92,20 +92,8 @@ def testCreateVolMisc(app): # Using previous name so we collide name.set_text(newname) win.combo_select("Format:", "raw") - cap = win.find("Max Capacity:", "spin button") - alloc = win.find("Allocation:", "spin button") - alloc.set_text("50.0") - alloc.click() - app.rawinput.pressKey("Enter") - lib.utils.check(lambda: cap.text == "50.0") - cap.set_text("1.0") - cap.click() - app.rawinput.pressKey("Enter") - lib.utils.check(lambda: alloc.text == "1.0") - alloc.set_text("0.5") - alloc.click() - app.rawinput.pressKey("Enter") - lib.utils.check(lambda: cap.text == "1.0") + sparse = win.find("Allocate", "check box") + lib.utils.check(lambda: sparse.checked) finish.click() app.click_alert_button("Error validating volume", "Close") @@ -122,6 +110,7 @@ def testCreateVolMisc(app): win.find("Backing store").click_expander() win.find("Browse...").click() app.select_storagebrowser_volume("disk-pool", "diskvol7") + sparse.check_not_onscreen() finish.click() vollist.find(newname) diff --git a/ui/createvol.ui b/ui/createvol.ui index a77048f4b..d136d1da2 100644 --- a/ui/createvol.ui +++ b/ui/createvol.ui @@ -142,7 +142,7 @@ True False True - 4 + 6 6 @@ -284,8 +284,7 @@ True False - 8 - 4 + 6 6 @@ -297,42 +296,12 @@ 10 1 1 - 1 0 - - - True - True - ● - 1.0 - adjustment1 - 10 - 1 - 1 - - - - 1 - 1 - - - - - True - False - start - GiB - - - 2 - 1 - - True @@ -350,7 +319,7 @@ True False end - Max Ca_pacity: + Ca_pacity: True vol-capacity @@ -360,13 +329,27 @@ - + + _Allocate entire volume now + True + True + False + True + True + + + 1 + 1 + 2 + + + + True False - end - _Allocation: - True - vol-allocation + + + 0 @@ -391,16 +374,20 @@ True True False + True True False + 4 6 True False - Path: + Pa_th: + True + backing-store 0 @@ -412,7 +399,6 @@ True True True - backing-store @@ -426,11 +412,12 @@ - Browse... + _Browse... True True True image2 + True @@ -445,7 +432,8 @@ True False False - Backing store + _Backing store + True diff --git a/virtManager/createvol.py b/virtManager/createvol.py index 2715237ff..584530386 100644 --- a/virtManager/createvol.py +++ b/virtManager/createvol.py @@ -41,9 +41,6 @@ class vmmCreateVolume(vmmGObjectUI): "on_vol_name_changed": self._vol_name_changed_cb, "on_vol_format_changed": self._vol_format_changed_cb, - "on_backing_store_changed": self._backing_storage_changed_cb, - "on_vol_allocation_value_changed": self._vol_allocation_changed_cb, - "on_vol_capacity_value_changed": self._vol_capacity_changed_cb, "on_backing_browse_clicked": self._browse_backing_clicked_cb, }) self.bind_escape_key_close() @@ -127,22 +124,17 @@ class vmmCreateVolume(vmmGObjectUI): self.widget("vol-name").grab_focus() self.widget("vol-name").emit("changed") - default_alloc = 0 - default_cap = 20 - self.widget("backing-store").set_text("") - alloc = default_alloc - if not self._can_alloc(): - alloc = default_cap - self._show_alloc() + self.widget("vol-nonsparse").set_active( + not self._should_default_sparse()) + self._show_sparse() self._show_backing() self.widget("backing-expander").set_expanded(False) - self.widget("vol-allocation").set_range(0, - int(self._parent_pool.get_available() / 1024 / 1024 / 1024)) - self.widget("vol-allocation").set_value(alloc) + pool_avail = int(self._parent_pool.get_available() / 1024 / 1024 / 1024) + default_cap = 20 self.widget("vol-capacity").set_range(0.1, 1000000) - self.widget("vol-capacity").set_value(default_cap) + self.widget("vol-capacity").set_value(min(default_cap, pool_avail)) self.widget("vol-parent-info").set_markup( _("%(volume)s's available space: %(size)s") % { @@ -183,25 +175,16 @@ class vmmCreateVolume(vmmGObjectUI): return StorageVolume.get_file_extension_for_format( self._get_config_format()) - def _can_only_sparse(self): - if self._get_config_format() == "qcow2": - return True - if (self.widget("backing-store").is_visible() and - self.widget("backing-store").get_text()): - return True - return False + def _should_default_sparse(self): + return self._get_config_format() == "qcow2" - def _can_alloc(self): - if self._can_only_sparse(): - return False - if self._parent_pool.get_type() == "logical": - # Sparse LVM volumes don't auto grow, so alloc=0 is useless - return False - return True + def _can_sparse(self): + dtype = self._parent_pool.xmlobj.get_disk_type() + return dtype == StorageVolume.TYPE_FILE - def _show_alloc(self): + def _show_sparse(self): uiutil.set_grid_row_visible( - self.widget("vol-allocation"), self._can_alloc()) + self.widget("vol-nonsparse"), self._can_sparse()) def _can_backing(self): if self._parent_pool.get_type() == "logical": @@ -252,13 +235,13 @@ class vmmCreateVolume(vmmGObjectUI): suffix = self.widget("vol-name-suffix").get_text() volname = name + suffix fmt = self._get_config_format() - alloc = self.widget("vol-allocation").get_value() cap = self.widget("vol-capacity").get_value() + nonsparse = self.widget("vol-nonsparse").get_active() backing = self.widget("backing-store").get_text() - if not self.widget("vol-allocation").get_visible(): + + alloc = 0 + if nonsparse: alloc = cap - if self._can_only_sparse(): - alloc = 0 vol = self._make_stub_vol() vol.name = volname @@ -341,28 +324,12 @@ class vmmCreateVolume(vmmGObjectUI): self._xmleditor.set_xml(xmlobj and xmlobj.get_xml() or "") def _vol_format_changed_cb(self, src): - self._show_alloc() + self._show_sparse() + self.widget("vol-nonsparse").set_active( + not self._should_default_sparse()) self._show_backing() self.widget("vol-name").emit("changed") - def _vol_capacity_changed_cb(self, src): - alloc_widget = self.widget("vol-allocation") - - cap = src.get_value() - alloc = self.widget("vol-allocation").get_value() - - if cap < alloc: - alloc_widget.set_value(cap) - - def _vol_allocation_changed_cb(self, src): - cap_widget = self.widget("vol-capacity") - - alloc = src.get_value() - cap = cap_widget.get_value() - - if alloc > cap: - cap_widget.set_value(alloc) - def _vol_name_changed_cb(self, src): text = src.get_text() @@ -374,8 +341,5 @@ class vmmCreateVolume(vmmGObjectUI): def _browse_backing_clicked_cb(self, src): self._browse_file() - def _backing_storage_changed_cb(self, src): - self._show_alloc() - def _create_clicked_cb(self, src): self._finish()