Change in vdsm[master]: utils: build cert paths in single place
by Piotr Kliczewski
Piotr Kliczewski has posted comments on this change.
Change subject: utils: build cert paths in single place
......................................................................
Patch Set 7:
OK, let me check hard coding the path. It should simplify bunch of code we have.
--
To view, visit https://gerrit.ovirt.org/52354
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I58dd3a5f7c1503fc38b6c6a204c036c06d09941b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
7 years, 10 months
Change in vdsm[master]: utils: build cert paths in single place
by Nir Soffer
Nir Soffer has posted comments on this change.
Change subject: utils: build cert paths in single place
......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/vdscli.py
File lib/vdsm/vdscli.py:
Line 103
Line 104
Line 105
Line 106
Line 107
> We are already hard coding this value in tool.certificates.PKI_DIR, so this
Additionally, ovirt-imageio-daemon is using the /etc/pki/vdsm/ - we cannot support configurable settings here. If you change this value, you break image upload and download in 4.0.
--
To view, visit https://gerrit.ovirt.org/52354
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I58dd3a5f7c1503fc38b6c6a204c036c06d09941b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 10 months
Change in vdsm[master]: utils: build cert paths in single place
by Nir Soffer
Nir Soffer has posted comments on this change.
Change subject: utils: build cert paths in single place
......................................................................
Patch Set 7:
(2 comments)
https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/sslutils.py
File lib/vdsm/sslutils.py:
Line 263:
Line 264: def create_ssl_context():
Line 265: sslctx = None
Line 266: if config.getboolean('vars', 'ssl'):
Line 267: path = config.get('vars', 'trust_store_path')
> As you can see here we are using it. Do you suggest to hard code this value
It is already hard coded in certificates.py and in libvirt.py (via import). Can you explain how this path will work with the hard coded values?
Line 268: protocol = (
Line 269: ssl.PROTOCOL_SSLv23
Line 270: if config.get('vars', 'ssl_protocol') == 'sslv23'
Line 271: else ssl.PROTOCOL_TLSv1
https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/vdscli.py
File lib/vdsm/vdscli.py:
Line 103
Line 104
Line 105
Line 106
Line 107
> In this code we need it. We would need to explore whether we could hard cod
We are already hard coding this value in tool.certificates.PKI_DIR, so this means that the current code does not support dynamic pki dir.
Your patch is not a refactoring but adding features that we don't need. I don't want to maintain features that nobody asked for. When we find broken feature that is not needed we remove it.
If you think this feature is needed and the fact that it does not work is a bug, open a bug.
--
To view, visit https://gerrit.ovirt.org/52354
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I58dd3a5f7c1503fc38b6c6a204c036c06d09941b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 10 months
Change in vdsm[master]: utils: build cert paths in single place
by Piotr Kliczewski
Piotr Kliczewski has posted comments on this change.
Change subject: utils: build cert paths in single place
......................................................................
Patch Set 7:
(4 comments)
https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:
Line 157: # localtion of the certificates
Line 158: def get_cert_paths(path):
Line 159: return (os.path.join(path, 'keys', 'vdsmkey.pem'),
Line 160: os.path.join(path, 'certs', 'vdsmcert.pem'),
Line 161: os.path.join(path, 'certs', 'cacert.pem'))
> This module is for constants, not for functions.
We need it.
https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/sslutils.py
File lib/vdsm/sslutils.py:
Line 263:
Line 264: def create_ssl_context():
Line 265: sslctx = None
Line 266: if config.getboolean('vars', 'ssl'):
Line 267: path = config.get('vars', 'trust_store_path')
> We can remove this configuration, it does not work anyway - all the code us
As you can see here we are using it. Do you suggest to hard code this value?
Line 268: protocol = (
Line 269: ssl.PROTOCOL_SSLv23
Line 270: if config.get('vars', 'ssl_protocol') == 'sslv23'
Line 271: else ssl.PROTOCOL_TLSv1
https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/tool/configurators/certific...
File lib/vdsm/tool/configurators/certificates.py:
Line 30
Line 31
Line 32
Line 33
Line 34
> Please move the 4 lines above to constants, and use constants.CA_FILE, con
we need path to build the constants.
https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/vdscli.py
File lib/vdsm/vdscli.py:
Line 103
Line 104
Line 105
Line 106
Line 107
> Do we use tsPath? Do we need to support this?
In this code we need it. We would need to explore whether we could hard code the path. I am not really sure whether it is good idea.
--
To view, visit https://gerrit.ovirt.org/52354
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I58dd3a5f7c1503fc38b6c6a204c036c06d09941b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 10 months
Change in vdsm[master]: utils: build cert paths in single place
by Nir Soffer
Nir Soffer has posted comments on this change.
Change subject: utils: build cert paths in single place
......................................................................
Patch Set 7:
(3 comments)
I think that the issue is trying to support dynamic pki dir, while some of the code is using static one (/etc/pki/vdsm). Lets simplify and support only static pki dir and use simple constants in constants.py.
https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/sslutils.py
File lib/vdsm/sslutils.py:
Line 263:
Line 264: def create_ssl_context():
Line 265: sslctx = None
Line 266: if config.getboolean('vars', 'ssl'):
Line 267: path = config.get('vars', 'trust_store_path')
We can remove this configuration, it does not work anyway - all the code using certificates.XXX ignores this value.
Line 268: protocol = (
Line 269: ssl.PROTOCOL_SSLv23
Line 270: if config.get('vars', 'ssl_protocol') == 'sslv23'
Line 271: else ssl.PROTOCOL_TLSv1
https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/tool/configurators/certific...
File lib/vdsm/tool/configurators/certificates.py:
Line 30
Line 31
Line 32
Line 33
Line 34
Please move the 4 lines above to constants, and use constants.CA_FILE, constants.CERT_FILE and constants.KEY_FILE in the code.
https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/vdscli.py
File lib/vdsm/vdscli.py:
Line 103
Line 104
Line 105
Line 106
Line 107
Do we use tsPath? Do we need to support this?
--
To view, visit https://gerrit.ovirt.org/52354
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I58dd3a5f7c1503fc38b6c6a204c036c06d09941b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 10 months
Change in vdsm[master]: utils: build cert paths in single place
by Nir Soffer
Nir Soffer has posted comments on this change.
Change subject: utils: build cert paths in single place
......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/52354/7/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:
Line 157: # localtion of the certificates
Line 158: def get_cert_paths(path):
Line 159: return (os.path.join(path, 'keys', 'vdsmkey.pem'),
Line 160: os.path.join(path, 'certs', 'vdsmcert.pem'),
Line 161: os.path.join(path, 'certs', 'cacert.pem'))
This module is for constants, not for functions.
Why do we need the path argument?
If you need to access temporary certificates (e.g. for testing), you can create your certificates in a temporary directory and monkeypatch the constants so the code accessing them will see the fake certificates.
--
To view, visit https://gerrit.ovirt.org/52354
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I58dd3a5f7c1503fc38b6c6a204c036c06d09941b
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 10 months
Change in vdsm[master]: hostdev: move scsi device code to separate class
by mzamazal＠redhat.com
Milan Zamazal has posted comments on this change.
Change subject: hostdev: move scsi device code to separate class
......................................................................
Patch Set 8:
(1 comment)
With the exception of the comment below it looks OK to me.
https://gerrit.ovirt.org/#/c/57960/8/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:
Line 447:
Line 448: return hostdev
Line 449:
Line 450: @classmethod
Line 451: def update_device_info(cls, vm, device_conf, x):
The method and `x' argument names as usually. :-)
Line 452: alias = x.getElementsByTagName('alias')[0].getAttribute('name')
Line 453: host_address = vmxml.device_address(x)
Line 454:
Line 455: # The routine is quite unusual because we cannot directly
--
To view, visit https://gerrit.ovirt.org/57960
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I784c158192433bba1ea4dd330f8a633024b9fec2
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 10 months
Change in vdsm[master]: hostdev: move usb device code to separate class
by mzamazal＠redhat.com
Milan Zamazal has posted comments on this change.
Change subject: hostdev: move usb device code to separate class
......................................................................
Patch Set 8:
(1 comment)
With the exception of the comment below it looks OK to me.
https://gerrit.ovirt.org/#/c/57958/8/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:
Line 354:
Line 355: return hostdev
Line 356:
Line 357: @classmethod
Line 358: def update_device_info(cls, vm, device_conf, x):
The same about the method name and `x' argument name as in the preceding patch.
Line 359: alias = x.getElementsByTagName('alias')[0].getAttribute('name')
Line 360: host_address = vmxml.device_address(x)
Line 361:
Line 362: # The routine is quite unusual because we cannot directly
--
To view, visit https://gerrit.ovirt.org/57958
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I481a966776089f37971a27637e06d21eafabd05c
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 10 months
Change in vdsm[master]: hostdev: use specific device classes in HostDevice
by fromani＠redhat.com
Francesco Romani has posted comments on this change.
Change subject: hostdev: use specific device classes in HostDevice
......................................................................
Patch Set 8:
(1 comment)
https://gerrit.ovirt.org/#/c/57963/8/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:
PS8, Line 320: @classmethod
: def update_device_info(cls, vm, device_conf):
: for device_xml in vm.domain.get_device_elements('hostdev'):
: device_type = device_xml.getAttribute('type')
: cls._DEVICE_MAPPING[device_type].update_device_info(
: vm, device_conf, device_xml)
> I also prefer not having special cases when creating device classes. Using
OK, since indeed I don't have any good reason against __new__ and we're explored a good number of alternatives, I'm now convinced __new__ is the right approach here.
--
To view, visit https://gerrit.ovirt.org/57963
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2734b7ec789c0ce57b64d7699cbe967c774bb608
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 10 months
Change in vdsm[master]: hostdev: move pci device code to separate class
by mzamazal＠redhat.com
Milan Zamazal has posted comments on this change.
Change subject: hostdev: move pci device code to separate class
......................................................................
Patch Set 8:
(1 comment)
https://gerrit.ovirt.org/#/c/57956/8/vdsm/virt/vmdevices/hostdevice.py
File vdsm/virt/vmdevices/hostdevice.py:
Line 264:
Line 265: return hostdev
Line 266:
Line 267: @classmethod
Line 268: def update_device_info(cls, vm, device_conf, x):
> I don't like having the extra `x' argument here, this very confusing wrt. o
Also, please use a better argument name than `x'.
Line 269: alias = x.getElementsByTagName('alias')[0].getAttribute('name')
Line 270: address = vmxml.device_address(x)
Line 271: source = x.getElementsByTagName('source')[0]
Line 272: device = pci_address_to_name(**vmxml.device_address(source))
--
To view, visit https://gerrit.ovirt.org/57956
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6bceb93c2434ff827406bbf4ee0af30f5726f6af
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Tomas Golembiovsky <tgolembi(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 10 months