Ala Hino has posted comments on this change. Change subject: gluster: Handle missing glusterfs-cli package ......................................................................
Patch Set 10: (5 comments) https://gerrit.ovirt.org/#/c/50363/10/vdsm/storage/storageServer.py File vdsm/storage/storageServer.py: Line 320 Line 321 Line 322 Line 323 Line 324 > check here if we have gluster cli - if we don't need to call _get_backup_se Done Line 321: self.log.warning("Required glusterfs-cli package is missing " Line 322: "on this host. Note that no backup " Line 323: "servers will be used! Please install the " Line 324: "missing package in order to use gluster storage " Line 325: "backup servers") > One issue with warning here, is this message is relevant only for connect, Moved to validate Line 326: Line 327: @property Line 328: def options(self): Line 329: if "backup-volfile-servers" in self._options: Line 336: Line 337: @property Line 338: def volinfo(self): Line 339: if not self._have_gluster_cli: Line 340: return None > Nobody should call volinfo if we don't have a gluster cli, don't protect th Why not to protect? My unit test checked that volinfo is None and if I remove this if, the test fails Line 341: Line 342: if self._volinfo is None: Line 343: self._volinfo = self._get_gluster_volinfo() Line 344: return self._volinfo Line 344: return self._volinfo Line 345: Line 346: def validate(self): Line 347: if not self._have_gluster_cli: Line 348: return > I think warning here is better since this is used only during connect, and Done Line 349: Line 350: replicaCount = self.volinfo['replicaCount'] Line 351: if replicaCount not in self.ALLOWED_REPLICA_COUNTS: Line 352: self.log.warning("Unsupported replica count (%s) for volume %r, " Line 354: replicaCount, self._volname) Line 355: Line 356: def _get_backup_servers_option(self): Line 357: if not self._have_gluster_cli: Line 358: return "" > Why do we need to protect this? check if we have gluster cli in the code th Done Line 359: Line 360: servers = utils.unique(brick.split(":")[0] for brick Line 361: in self.volinfo['bricks']) Line 362: self.log.debug("Using bricks: %s", servers) -- To view, visit https://gerrit.ovirt.org/50363 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9c33aa33e4ffe6a382d40e1bc63f6735efcfcd1f Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@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/mailman/listinfo/vdsm-patches
