Alon Bar-Lev has posted comments on this change. Change subject: WebSocketProxy on a separate host ......................................................................
Patch Set 29: (6 comments) well, this is a mixed patch of splitting engine and splitting websocket proxy. splitting websocket proxy is not critical and can wait (as written in the bug) until we split the engine out. this patch on its own is very complex to introduce before. also it is very complex by its own, I did not follow the entire sequence, but for sure: 1. ssl certificate enrolment should be provided, probably we need to think how to prompt user with certificate request and allow him to enroll and paste back certificate. 2. use of ssh should be removed, we should not rely on this protocol. As if we do require ssh, why do we need to run setup on remote machine, we can run setup on engine machine and setup the websocket proxy using ssh on another machine. http://gerrit.ovirt.org/#/c/26898/29/packaging/setup/plugins/ovirt-engine-setup/websocket_proxy/config.py File packaging/setup/plugins/ovirt-engine-setup/websocket_proxy/config.py: Line 139: default=22, Line 140: ) Line 141: transport = None Line 142: try: Line 143: transport = paramiko.Transport( I am unsure why ssh is required, and why should we have the engine fqdn and not ask the user. we can never determine it as there may be multiple. but I am unsure how this is related to websocket proxy, please explain. Line 144: ( Line 145: self.environment[ Line 146: osetupcons.ConfigEnv.REMOTE_ENGINE_HOST Line 147: ], Line 175: osetupcons.ConfigEnv.REMOTE_ENGINE_HOST Line 176: ], Line 177: error=e, Line 178: ) Line 179: ) pattern should always be: if fail: throw continue as usual Line 180: finally: Line 181: if transport is not None: Line 182: transport.close() Line 183: Line 178: ) Line 179: ) Line 180: finally: Line 181: if transport is not None: Line 182: transport.close() please use context lib and with Line 183: Line 184: @plugin.event( Line 185: stage=plugin.Stages.STAGE_CUSTOMIZATION, Line 186: name=osetupcons.Stages.CONFIG_WEBSOCKET_PROXY_CUSTOMIZATION, Line 251: else: Line 252: self.logger.info(_('Local engine, remote WebSocket Proxy')) Line 253: else: Line 254: if self._local_wsp: Line 255: self.logger.info(_('Remote engine, local WebSocket Proxy')) please display one message and put in variables. self.logger.info(_('Websocket proxy location: {}, Engine location {}") I still do not fully understand why the engine location is an issue. Line 256: Line 257: if self._local_wsp: Line 258: self.environment[ Line 259: osetupcons.ConfigEnv.WEBSOCKET_PROXY_HOST Line 316: osetupcons.ConfigEnv.WEBSOCKET_PROXY_PORT Line 317: ], Line 318: }) Line 319: Line 320: def _prepare_tran(self, content=''): not sure why this function is required. Line 321: self.environment[otopicons.CoreEnv.MAIN_TRANSACTION].append( Line 322: filetransaction.FileTransaction( Line 323: name=( Line 324: osetupcons.FileLocations. Line 491: '(@VALUES@)[@DEFAULT@]: ' Line 492: ), Line 493: prompt=True, Line 494: default=True, Line 495: ) Use /ovirt-engine/services//pki-resource?resource=engine-certificate to download certificate. Line 496: Line 497: if not self.environment[osetupcons.ConfigEnv.FETCH_REMOTE_CERTS]: Line 498: raise RuntimeError( Line 499: _( -- To view, visit http://gerrit.ovirt.org/26898 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I169604429e5a2d72573b05c0e5481306edfdd935 Gerrit-PatchSet: 29 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Lev Veyde <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Yedidyah Bar David <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
