Alon Bar-Lev has posted comments on this change. Change subject: vdsm-imaged: Support random io to oVirt disks ......................................................................
Patch Set 2: (10 comments) it is so far from the methods I use to write software, so I won't be able to help much. I added few general comments. https://gerrit.ovirt.org/#/c/41824/2/vdsm-imaged/README File vdsm-imaged/README: Line 14: available via HTTP on port 54322. Line 15: Line 16: - ticket service manage session tickets authorizing image service Line 17: operations. This service is available via HTTP over Line 18: unix domain socket. what about reporting back statistics to vdsm so it can expose them to engine? Line 19: Line 20: Transfer session flow Line 21: Line 22: - Client starts an engine transfer session using oVirt REST API. Line 22: - Client starts an engine transfer session using oVirt REST API. Line 23: - Engine creates session tickets Line 24: - Engine ask Vdsm to start a transfer session with a ticket. Line 25: - Vdsm prepares an image, and add the session ticket to vdsm-imaged. Line 26: - Engine returns signed session tickets to client. I guess this is the proxy sequence (interactive), so engine returns two tickets to client: 1. the access ticket 2. the transfer ticket (vdsm visible) for this specific image transaction. there should be also direct interaction with the daemon, in which application (not a user) communicates, in this case the access ticket is not required. Line 27: - Client perform io operations with oVirt transfer proxy using the Line 28: signed session ticket. Line 29: - oVirt transfer proxy performs random io operations with vdsm-imaged Line 30: using the session ticket uuid. Line 33: - Vdsm deletes session ticket from vdsm-imaged. Line 34: Line 35: Session tickets are ephemeral; A client needs to request Engine to renew Line 36: the ticket from time to time, otherwise a ticket will expire and the Line 37: transfer session will be aborted. the "session" ticket (or access token) is checked once at interactive mode when user is redirected to the proxy. as long as transaction is in progress there is no expiration as http session with proxy is kept alive. Line 38: Line 39: Session tickets are not persisted. In case of vdsm-imaged crash or Line 40: reboot, Engine will provide a new session ticket and possibly point https://gerrit.ovirt.org/#/c/41824/2/vdsm-imaged/imaged.py File vdsm-imaged/imaged.py: Line 45: try: Line 46: while running: Line 47: time.sleep(30) Line 48: finally: Line 49: stop() please use python-daemon, you can see an example at engine. examples: packaging/pythonlib/ovirt_engine/service.py packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py you need to do much more in order to detach from controlling terminal. also, I am not fund of waisting CPU time to wakeup every 30 seconds, this is bad practice you should be able to signal the main process when stopping. Line 50: Line 51: Line 52: def terminate(signo, frame): Line 53: global running Line 49: stop() Line 50: Line 51: Line 52: def terminate(signo, frame): Line 53: global running why do you use global variables? just create a simple object and put all as properties. Line 54: running = False Line 55: Line 56: Line 57: def start(config): Line 91: server.serve_forever(poll_interval=config.poll_interval) Line 92: Line 93: t = threading.Thread(target=run, name=name) Line 94: t.daemon = True Line 95: t.start() are you sure you must have a thread to handle this server? Line 96: Line 97: Line 98: class Config(object): Line 99: Line 102: port = 54322 Line 103: repo_dir = "/var/run/vdsm/storage" Line 104: poll_interval = 1.0 Line 105: buffer_size = 64 * 1024 Line 106: socket = "/var/run/vdsm/imaged.socket" please let autoconf to set these locations. Line 107: Line 108: @property Line 109: def key_file(self): Line 110: return os.path.join(self.pki_dir, "keys", "vdsmkey.pem") Line 103: repo_dir = "/var/run/vdsm/storage" Line 104: poll_interval = 1.0 Line 105: buffer_size = 64 * 1024 Line 106: socket = "/var/run/vdsm/imaged.socket" Line 107: please use conf.d config file structure (unlike vdsm) to ease configuration. Line 108: @property Line 109: def key_file(self): Line 110: return os.path.join(self.pki_dir, "keys", "vdsmkey.pem") Line 111: https://gerrit.ovirt.org/#/c/41824/2/vdsm-imaged/pki/certs/vdsmcert.pem File vdsm-imaged/pki/certs/vdsmcert.pem: should probably go into tests Line 1: -----BEGIN CERTIFICATE----- Line 2: MIIDEjCCAfoCCQCNg3LPIL2ZWTANBgkqhkiG9w0BAQUFADBLMQswCQYDVQQGEwJV Line 3: UzEMMAoGA1UECAwDRm9vMQwwCgYDVQQHDANCYXIxDDAKBgNVBAoMA0RpczESMBAG Line 4: A1UEAwwJMTI3LjAuMC4xMB4XDTE1MDUxOTE4MTEzM1oXDTE2MDUxODE4MTEzM1ow https://gerrit.ovirt.org/#/c/41824/2/vdsm-imaged/pki/keys/vdsmkey.pem File vdsm-imaged/pki/keys/vdsmkey.pem: should probably go into tests Line 1: -----BEGIN RSA PRIVATE KEY----- Line 2: MIIEpAIBAAKCAQEAzD4rYpybBU5XR3pcIZYL3Xn7AEUZg3H+/CIOT0JHiNKYY0C5 Line 3: 9ZXEHrao6X75YR6d9UxKAz1lwbuPpvuzUe8+fOmPiI2t+K6AALy0MK6R05lrrQPF Line 4: Fysy211cUCrsxzA7bPuSVL6gWSMxnlwMigZKx/aHB4Y8N+t8pasY8Ek7idKcDpxQ -- To view, visit https://gerrit.ovirt.org/41824 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If3339fa94ef8464228cd036f4fe8eea61887e337 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Amit Aviram <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Greg Padgett <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
