Dan Kenigsberg has posted comments on this change. Change subject: vdsm-tool: Add register verb ......................................................................
Patch Set 1: Code-Review-1 (10 comments) https://gerrit.ovirt.org/#/c/40966/1//COMMIT_MSG Commit Message: Line 8: Line 9: The new verb register will make the registration Line 10: from a node into the Engine. This verb supports Line 11: old Engine registration schema and the new one as Line 12: service. This commit it's an alternative for It's not clear which discussion you refer to. Line 13: the registration discussions. Line 14: Line 15: Supported Engine: >= 3.3 Line 16: https://gerrit.ovirt.org/#/c/40966/1/lib/vdsm/tool/register.py File lib/vdsm/tool/register.py: Line 16: # also available at http://www.gnu.org/copyleft/gpl.html. Line 17: import argparse Line 18: import getpass Line 19: import logging Line 20: import M2Crypto M2Crypto and selinux are not part of python's stdlib. import them in another block. Line 21: import os Line 22: import pwd Line 23: import requests Line 24: import selinux Line 177: Print and log a message Line 178: """ Line 179: print(msg) Line 180: Line 181: if level == "error": logging.log(level, msg) accepts a (numeric) level, which makes this long if-elif-end block redundant. Just pass logging.INFO, logging.CRITICAL etc. But more importantly: you can add a log handler that spits to stderr on top of the one going to the log file. The logging module has this print-and-log logic implemented. Line 182: self.logger.error(msg, exc_info=True) Line 183: elif level == "debug": Line 184: self.logger.debug(msg) Line 185: elif level == "warning": Line 188: self.logger.critical(msg) Line 189: elif level == "info": Line 190: self.logger.info(msg) Line 191: Line 192: def __set_logger(self): we don't use these uber-private double-underscore in new vdsm code. It's not like anyone is going to inherit from this class to care about it. Line 193: """ Line 194: The logging settings Line 195: Saving log in: /var/log/vdsm/register.log Line 196: """ Line 193: """ Line 194: The logging settings Line 195: Saving log in: /var/log/vdsm/register.log Line 196: """ Line 197: # Avoid node side effects of logging module not been working could you explain this a bit better? what's the node issue you are trying to avoid? How does a late reload help? using "reload" in production is a very disturbing sign. Line 198: logging.shutdown() Line 199: reload(logging) Line 200: Line 201: logging.basicConfig( Line 204: format='%(asctime)s %(message)s', Line 205: datefmt='%m/%d/%Y %I:%M:%S %p', Line 206: ) Line 207: Line 208: if sys.version_info >= (2, 7): we now support only python>=2.7, so this can be dropped. Line 209: logging.captureWarnings(True) Line 210: Line 211: return logging.getLogger(__name__) Line 212: Line 262: with open(cert, 'r') as f: Line 263: cert = f.read() Line 264: Line 265: x509 = M2Crypto.X509.load_cert_string(cert, M2Crypto.X509.FORMAT_PEM) Line 266: fp = x509.get_fingerprint('sha1') please use hashlib.sha1(ssl.PEM_cert_to_DER_cert(cert)).hexdigest() as it's in the stdlib Line 267: fp = ':'.join(fp[pos:pos + 2] for pos in range(0, len(fp), 2)) Line 268: Line 269: return fp Line 270: Line 271: def download_ca(self): Line 272: """ Line 273: Download CA from Engine and save self.ca_engine Line 274: """ Line 275: if self.reg_protocol == "legacy": vdsm_id-handling code must not in its own function - we "download ca" should not have side-effects on /etc/vdsm/vdsm.id. Line 276: # The legacy version uses the format: UUID_MACADDRESS Line 277: self.uuid = getHostUUID(legacy=True) Line 278: self.url_reg += "&vds_unique_id={u}".format(u=self.uuid) Line 279: else: Line 488: - In case, there is no UUID, use auxiliary function from VDSM Line 489: to generate it and store in /etc/vdsm/vdsm.id Line 490: Line 491: Legacy reg: Line 492: ============ Which Engine versions require this Legacy reg? If engin-3.3 can handle the service, we can (and should) drop this code. Otherwise, please add # REQUIRED_FOR: engine-3.3 so we can tell easily when this can be thrown away. Line 493: - Process UUID Line 494: Line 495: - Download CA via Line 496: https://ENGINE_FQDN Line 499: https://ENGINE_FQDN/engine.ssh.key.txt Line 500: Line 501: - Register via URL: Line 502: (Original .NET version and earlier Linux versions) Line 503: https://ENGINE_FQDN/RHEVManagerWeb/VdsAutoRegistration.aspx?vds_ip=NODE_FQDN_OR_IP&vds_name=NODE_NAME&vds_unique_id=NODE_UUID&port=54321 Please break this line. pep8-1.4.6 does not like it, and frankly - neither do I. It's not clickable anyway, it's just an example. So it's better broken by the ? and & chars. Line 504: Line 505: or Line 506: Line 507: https://ENGINE_FQDN/OvirtEngineWeb/register?vds_ip=NODE_FQDN_OR_IP&vds_name=NODE_NAME&vds_unique_id=NODE_UUID&port=54321 -- To view, visit https://gerrit.ovirt.org/40966 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ica800027beec1e5a20165bb5e1e78baf9283c1da Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
