Yedidyah Bar David has posted comments on this change. Change subject: packaging: setup: deploy Cinder and Glance Docker containers ......................................................................
Patch Set 35: (16 comments) https://gerrit.ovirt.org/#/c/39519/35/packaging/setup/ovirt_engine_setup/dockerc/constants.py File packaging/setup/ovirt_engine_setup/dockerc/constants.py: Line 30: return gettext.dgettext(message=m, domain='ovirt-engine-setup') Line 31: Line 32: Line 33: @util.export Line 34: class Const(object): Most of these should not be Const but random Line 35: C_IMAGE_RABBITMQ = 'kollaglue/centos-rdo-rabbitmq:latest' Line 36: C_NAME_RABBITMQ = 'rabbitmq' Line 37: C_IMAGE_MARIADBDATA = 'kollaglue/centos-rdo-mariadb-data:latest' Line 38: C_NAME_MARIADBDATA = 'mariadbdata' https://gerrit.ovirt.org/#/c/39519/35/packaging/setup/plugins/ovirt-engine-common/dockerc/core.py File packaging/setup/plugins/ovirt-engine-common/dockerc/core.py: Line 55: osetupcons.CoreEnv.SETUP_ATTRS_MODULES Line 56: ].append(odockerccons) Line 57: Line 58: @plugin.event( Line 59: stage=plugin.Stages.STAGE_LATE_SETUP, Condition=? Most/all current plugins are optional even if installed Line 60: ) Line 61: def _late_setup(self): Line 62: if not self.environment[ Line 63: osetupcons.CoreEnv.DEVELOPER_MODE https://gerrit.ovirt.org/#/c/39519/35/packaging/setup/plugins/ovirt-engine-remove/dockerc/misc.py File packaging/setup/plugins/ovirt-engine-remove/dockerc/misc.py: Line 48: self._dcli = docker.Client(base_url='unix://var/run/docker.sock') Line 49: Line 50: @plugin.event( Line 51: stage=plugin.Stages.STAGE_CUSTOMIZATION, Line 52: name=osetupcons.Stages.REMOVE_CUSTOMIZATION_COMMON, How come we already have 3 with this name? and now you add another one? Never noticed this. Sounds to me like a rather hard-to-debug bug. Line 53: condition=lambda self: not self.environment[ Line 54: osetupcons.RemoveEnv.REMOVE_ALL Line 55: ], Line 56: ) Line 78: stage=plugin.Stages.STAGE_MISC, Line 79: condition=lambda self: ( Line 80: ( Line 81: self.environment[osetupcons.RemoveEnv.REMOVE_ALL] or Line 82: self.environment[odockerccons.RemoveEnv.REMOVE_DOCKERC] I'd personally check only REMOVE_DOCKERC, and always set it in previous function. I know we have many such places, I'd change them all :-) Line 83: ) and not self.environment[osetupcons.CoreEnv.DEVELOPER_MODE] Line 84: ), Line 85: ) Line 86: def _misc(self): Line 101: self.logger.info(_('Stopping {cname}').format(cname=cont)) Line 102: try: Line 103: self._dcli.stop( Line 104: container=cont, Line 105: timeout=60, Did this ever fail for you during testing? Perhaps set a shorter timeout and loop (and log)? Line 106: ) Line 107: except docker.errors.APIError as ex: Line 108: if ex.response.status_code == 404: Line 109: self.logger.warning( Line 132: else: Line 133: raise ex Line 134: Line 135: if self.services.exists( Line 136: name=odockerccons.Const.DOCKER_SERVICE_NANE Perhaps add some other condition? What if user already had it enabled and up before setup? Line 137: ): Line 138: self.services.startup( Line 139: name=odockerccons.Const.DOCKER_SERVICE_NANE, Line 140: state=False, Line 137: ): Line 138: self.services.startup( Line 139: name=odockerccons.Const.DOCKER_SERVICE_NANE, Line 140: state=False, Line 141: ) You don't stop? Only disable? Line 142: self.environment[ Line 143: odockerccons.RemoveEnv.REMOVE_DCLIST Line 144: ] = '' Line 145: https://gerrit.ovirt.org/#/c/39519/35/packaging/setup/plugins/ovirt-engine-setup/dockerc/config.py File packaging/setup/plugins/ovirt-engine-setup/dockerc/config.py: Line 65: super(Plugin, self).__init__(context=context) Line 66: self._enabled = True Line 67: self._dimages = [] Line 68: self._already_deployed_by_me = [] Line 69: self._dcli = docker.Client(base_url='unix://var/run/docker.sock') Does this try to connect? I'd put this later, if enabled Line 70: Line 71: @plugin.event( Line 72: stage=plugin.Stages.STAGE_INIT, Line 73: ) Line 205: ]) Line 206: self.environment[ Line 207: osetupcons.NetEnv.FIREWALLD_SUBST Line 208: ].update({ Line 209: '@OVIRT_CINDER_PORT@': odockerccons.Const.CINDER_SERVICE_PORT Perhaps put this in env too? So user can config with answer file? Line 210: }) Line 211: if self.environment[ Line 212: odockerccons.ConfigEnv.DOCKERC_GLANCE Line 213: ]: Line 219: ]) Line 220: self.environment[ Line 221: osetupcons.NetEnv.FIREWALLD_SUBST Line 222: ].update({ Line 223: '@OVIRT_GLANCE_PORT@': odockerccons.Const.GLANCE_SERVICE_PORT same Line 224: }) Line 225: Line 226: @plugin.event( Line 227: stage=plugin.Stages.STAGE_VALIDATION, Line 243: ) Line 244: ) Line 245: Line 246: if self.environment[ Line 247: odockerccons.RemoveEnv.REMOVE_DCLIST Is this the list of containers deployed by us? I'd move it to some other class in odockerccons and give it a different name. Was quite hard to understand like that. For me. Line 248: ]: Line 249: self._already_deployed_by_me = [ Line 250: x.strip() Line 251: for x in self.environment[ Line 261: # suffix to the container name. Line 262: if already_existing_not_by_me: Line 263: self.dialog.note( Line 264: text=_( Line 265: 'The following container was found:\n' containers were Line 266: ' {found}\n' Line 267: 'Please remove or rename and ' Line 268: 'than execute the setup again\n' Line 269: ).format( Line 264: text=_( Line 265: 'The following container was found:\n' Line 266: ' {found}\n' Line 267: 'Please remove or rename and ' Line 268: 'than execute the setup again\n' then execute Setup again See also [1] :-) [1] https://gerrit.ovirt.org/#/q/Ic2a50fdb2e3b2281fafa894b8e95c7902ba24380,n,z Line 269: ).format( Line 270: found=', '.join(already_existing), Line 271: ), Line 272: ) Line 326: hostname = osetuphostname.Hostname(plugin=self) Line 327: dnsresolved = hostname.isResolvedByDNS(fqdn) Line 328: # TODO: check if we also need to force container DNS Line 329: for cont in self._dimages: Line 330: self.logger.info(_('Pulling {cname}').format(cname=cont['name'])) No idea what this does. Is any of it configurable? Outside? E.g. what if a user wants to use a proxy/mirror/whatever? Line 331: for line in self._dcli.pull(cont['image'], stream=True): Line 332: jline = json.loads(line) Line 333: self.logger.debug(json.dumps(jline, indent=4)) Line 334: if 'error' in jline: Line 457: """, Line 458: args=dict( Line 459: provider_id=gpUUID, Line 460: provider_name="local-glance-image-repository", Line 461: provider_description="Local Glance repository for oVirt", perhaps localized? No idea if that's common Line 462: provider_url=( Line 463: 'http://' + Line 464: self.environment[osetupcons.ConfigEnv.FQDN] + Line 465: ':' + Line 497: self.logger.info(_('Enabling Docker')) Line 498: self.services.startup( Line 499: name=odockerccons.Const.DOCKER_SERVICE_NANE, Line 500: state=True, Line 501: ) Don't start? Line 502: Line 503: -- To view, visit https://gerrit.ovirt.org/39519 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id192819a95343df9d9d035f8632d17ac76d2d82f Gerrit-PatchSet: 35 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Lev Veyde <lve...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
