Simone Tiraboschi has posted comments on this change. Change subject: Avoiding legacy health servlet usage ......................................................................
Patch Set 3: (10 comments) http://gerrit.ovirt.org/#/c/26878/3/src/ovirt_hosted_engine_setup/check_liveliness.py File src/ovirt_hosted_engine_setup/check_liveliness.py: Line 43: TIMEOUT = 20 Line 44: Line 45: def __init__(self): Line 46: super(LivelinessChecker, self).__init__() Line 47: self.cacert = None > _cacert? Done Line 48: Line 49: def _getPKICert(self, fqdn): Line 50: def check_ignore(*args, **kw): Line 51: return True Line 56: sock.set_post_connection_check_callback(check_ignore) Line 57: sock.connect(fqdn) Line 58: sock.shutdown(socket.SHUT_RDWR) Line 59: c_c = [c.as_pem() for c in sock.get_peer_cert_chain()] Line 60: if c_c[-1]: > if you need only -1, then get it from the sock.get... directly, no? Done Line 61: self.logger.debug(content) Line 62: fd, self.cacert = tempfile.mkstemp( Line 63: prefix='engine-ca', Line 64: suffix='.crt', Line 56: sock.set_post_connection_check_callback(check_ignore) Line 57: sock.connect(fqdn) Line 58: sock.shutdown(socket.SHUT_RDWR) Line 59: c_c = [c.as_pem() for c in sock.get_peer_cert_chain()] Line 60: if c_c[-1]: > Maybe better to store the CA in /etc/ovirt-hosted-engine on setup and use t It's probably a good idea to do it just one time and than store it but how we can do it in a secure way? the CA cert it's just on the hosted engine host. Should we ask the user to verify that signature? Now we are using it just for a kind of ping and, on my opinion, it's not that relevant to check it but if we are going to store it also for other future purposes we need to be sure that we can trust it. Line 61: self.logger.debug(content) Line 62: fd, self.cacert = tempfile.mkstemp( Line 63: prefix='engine-ca', Line 64: suffix='.crt', Line 58: sock.shutdown(socket.SHUT_RDWR) Line 59: c_c = [c.as_pem() for c in sock.get_peer_cert_chain()] Line 60: if c_c[-1]: Line 61: self.logger.debug(content) Line 62: fd, self.cacert = tempfile.mkstemp( > where do you delete it? on __exit__ Line 63: prefix='engine-ca', Line 64: suffix='.crt', Line 65: ) Line 66: with os.fdopen(fd, 'w') as fileobj: Line 86: ), Line 87: username='admin@internal', Line 88: password=self.environment[ Line 89: ohostedcons.EngineEnv.ADMIN_PASSWORD, Line 90: ], > you'll need to change also __main__ for asking the password. __main__ reads ohostedcons.FileLocations.OVIRT_HOSTED_ENGINE_SETUP_CONF isn't that password also there? if not, it's probably a good idea to store it there also for the HA daemons without the need to ask any question. I'll check Line 91: # it's simply a ping, we are basically Line 92: # trusting any certs Line 93: ca_file=self.cert, Line 94: ) Line 98: return isUp Line 99: Line 100: def __exit__(self, type, value, tb): Line 101: if self.cert is not None and os.path.exists(self.cert): Line 102: os.unlink(self.cert) > oh... shouldn't this be self.cacert? Done Line 103: Line 104: Line 105: if __name__ == "__main__": Line 106: import sys Line 129: ) Line 130: ) Line 131: sys.exit(2) Line 132: Line 133: with LivelinessChecker() as live_checker: > I think you're missing __enter__ and __exit__ in LivelinessChecker for usin Done Line 134: if not live_checker.isEngineUp(config['fqdn']): Line 135: print _('Hosted Engine is not up!') Line 136: sys.exit(1) Line 137: print _('Hosted Engine is up!') http://gerrit.ovirt.org/#/c/26878/3/src/plugins/ovirt-hosted-engine-setup/engine/health.py File src/plugins/ovirt-hosted-engine-setup/engine/health.py: Line 89 Line 90 Line 91 Line 92 Line 93 > why not raise as well? or keep asking? but how come we get response tha is all this code is under 'while poll:' so it keep asking until live_checker.isEngineUp(fqdn) says True or until the user aborts it with option 3 Line 83: '(2) Power off and restart the VM\n' Line 84: '(3) Abort setup\n\n(@VALUES@)[@DEFAULT@]: ' Line 85: ), Line 86: prompt=True, Line 87: validValues=(_('1'), _('2'), _('3')), > why don't you accept 'continue', 'poweroff', 'abort'? much more friendly an It was in the previous code and it's not strictly related to this patch but if we want we can also change it on another patch Line 88: default=_('1'), Line 89: caseSensitive=False) Line 90: if response == _('1').lower(): Line 91: if live_checker.isEngineUp(fqdn): Line 86: prompt=True, Line 87: validValues=(_('1'), _('2'), _('3')), Line 88: default=_('1'), Line 89: caseSensitive=False) Line 90: if response == _('1').lower(): > this is strange... and also stupid :-) Line 91: if live_checker.isEngineUp(fqdn): Line 92: poll = False Line 93: else: Line 94: self.dialog.note( -- To view, visit http://gerrit.ovirt.org/26878 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3522ccb82eee4bf7f04ded012d9badc97c55b5a0 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-hosted-engine-setup Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: David Caro <dcaro...@redhat.com> Gerrit-Reviewer: Lev Veyde <lve...@gmail.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
