Change in vdsm[master]: vdsm-imaged: Support random io to oVirt disks

Alon Bar-Lev alonbl at redhat.com
Tue Jun 2 10:56:59 UTC 2015


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 <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Daniel Erez <derez at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Greg Padgett <gpadgett at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list