Nir Soffer has posted comments on this change. Change subject: vdsm-imaged: Support random io to oVirt disks ......................................................................
Patch Set 2: (8 comments) Thanks for reviewing Alon! 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 engin We considered two options: - vdsm report progress to engine, report to engine, report to clients via rest api - proxy get progress from imaged, report to clients - client perform multiple request to proxy, no progress needed. This is how the ui will send data, since streaming data from browser is broken. This is not implemented yet since we did not decided yet what is the way we want to do that. 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 tic I'll update the text to reflect the difference between the tickets. What kind of direct interaction is needed? Do you mean local interaction for debugging? 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 Even when the user is redirected to the proxy, the ui will have to do multiple requests, since browsers do not support streaming (practically broken). So the proxy may have to validate the session ticket multiple times for one session, or validate once and then share a session key with the client. We assume that the transfer ticket will expire automatically, so you cannot reuse an old transfer ticket. Greg mentioned expire time of 5 minutes. 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. I plan to run via systemd, so no demonizing is needed. The 30 seconds wake up here is not the real issue - serve_forever() will wake every 1 second!. We can solve this by adding a shutdown pipe, and make all servers wait on the listen fd and the shutdown pipe fd. But I don't plan to address this before the functionality is finished. 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 This is just another way to use module global variable without the global keyword. Why do you think it is better? 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? I must start 2 servers, each listening to different kind of socket, so I must start the first one in its own thread. I prefer to start both in a separate thread, and use the main thread only for signal handling. This way the other thread never receive any signal except SIGCHLD (if the thread was running a child process). 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. Ok 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 What do you mean by conf.d config file structure? Line 108: @property Line 109: def key_file(self): Line 110: return os.path.join(self.pki_dir, "keys", "vdsmkey.pem") Line 111: -- 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
