Change in vdsm[master]: cdrom: API change: require index
Martin Polednik has posted comments on this change. Change subject: cdrom: API change: require index .. Patch Set 1: (1 comment) https://gerrit.ovirt.org/#/c/56805/1/lib/api/vdsmapi-schema.json File lib/api/vdsmapi-schema.json: Line 6497: # @vmID:The UUID of the VM Line 6498: # Line 6499: # @interface: The bus used for cdrom Line 6500: # Line 6501: # @index: Index on the bus used > BTW looked into it further, unawesome thing is that changeCD actually expec Or to phrase it better, engine sends string to changeCD as of current masters. Line 6502: # Line 6503: # @driveSpec: The specification of the new CD Line 6504: # Line 6505: # Returns: -- To view, visit https://gerrit.ovirt.org/56805 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I178c1a02bbad962f9dc9b67bed7691cf170ee896 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: cdrom: API change: require index
Martin Polednik has posted comments on this change. Change subject: cdrom: API change: require index .. Patch Set 1: (1 comment) https://gerrit.ovirt.org/#/c/56805/1/lib/api/vdsmapi-schema.json File lib/api/vdsmapi-schema.json: Line 6497: # @vmID:The UUID of the VM Line 6498: # Line 6499: # @interface: The bus used for cdrom Line 6500: # Line 6501: # @index: Index on the bus used > Ok, good plan BTW looked into it further, unawesome thing is that changeCD actually expects drivespec to be string... Line 6502: # Line 6503: # @driveSpec: The specification of the new CD Line 6504: # Line 6505: # Returns: -- To view, visit https://gerrit.ovirt.org/56805 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I178c1a02bbad962f9dc9b67bed7691cf170ee896 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: cdrom: API change: require index
Francesco Romani has posted comments on this change. Change subject: cdrom: API change: require index .. Patch Set 1: (1 comment) https://gerrit.ovirt.org/#/c/56805/1/lib/api/vdsmapi-schema.json File lib/api/vdsmapi-schema.json: Line 6497: # @vmID:The UUID of the VM Line 6498: # Line 6499: # @interface: The bus used for cdrom Line 6500: # Line 6501: # @index: Index on the bus used > I would keep the interface the same and add optional arguments to DriveSpec Ok, good plan Line 6502: # Line 6503: # @driveSpec: The specification of the new CD Line 6504: # Line 6505: # Returns: -- To view, visit https://gerrit.ovirt.org/56805 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I178c1a02bbad962f9dc9b67bed7691cf170ee896 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: cdrom: API change: require index
Nir Soffer has posted comments on this change. Change subject: cdrom: API change: require index .. Patch Set 1: (1 comment) https://gerrit.ovirt.org/#/c/56805/1/vdsm/API.py File vdsm/API.py: Line 112: def __init__(self, UUID): Line 113: APIBase.__init__(self) Line 114: self._UUID = UUID Line 115: Line 116: def changeCD(self, interface, index, driveSpec): > This was my initial idea, there is one annoying problem though: driveSpec i I'm not sure that we actually support all these. - DriveSpecVolume - vdsm image (PDIV) - DriveSpecGUID - direct lun - DriveSpecPayload - used for cdrom/floppy (not storage) - DriveSpecPath - we have support for vdsm, but engine does not used this (not storage). I think we should remove this. - DriveSpecUUID was never used by engine, and the support in vdsm was removed recently https://gerrit.ovirt.org/56478. This type is removed by https://gerrit.ovirt.org/56992. These parameters should appear in the all the relevant types. We probably should find a better way to specify this without duplicating the schema. Line 117: """ Line 118: Change the CD in the specified VM. Line 119: Line 120: :param vmId: uuid of specific VM. -- To view, visit https://gerrit.ovirt.org/56805 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I178c1a02bbad962f9dc9b67bed7691cf170ee896 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: cdrom: API change: require index
Martin Polednik has posted comments on this change. Change subject: cdrom: API change: require index .. Patch Set 1: (1 comment) https://gerrit.ovirt.org/#/c/56805/1/vdsm/API.py File vdsm/API.py: Line 112: def __init__(self, UUID): Line 113: APIBase.__init__(self) Line 114: self._UUID = UUID Line 115: Line 116: def changeCD(self, interface, index, driveSpec): > Why not keep single parameter (driveSpec), and include the interface and th This was my initial idea, there is one annoying problem though: driveSpec is union and contains ['DriveSpecVolume', 'DriveSpecGUID', 'DriveSpecUUID', 'DriveSpecPayload', 'DriveSpecPath']. That is kind of unfortunate in this case as we'd have to add both to all of these - if you're fine with that, OK but your future-ack is needed first. Line 117: """ Line 118: Change the CD in the specified VM. Line 119: Line 120: :param vmId: uuid of specific VM. -- To view, visit https://gerrit.ovirt.org/56805 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I178c1a02bbad962f9dc9b67bed7691cf170ee896 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: cdrom: API change: require index
Nir Soffer has posted comments on this change. Change subject: cdrom: API change: require index .. Patch Set 1: (1 comment) https://gerrit.ovirt.org/#/c/56805/1/lib/api/vdsmapi-schema.json File lib/api/vdsmapi-schema.json: Line 6497: # @vmID:The UUID of the VM Line 6498: # Line 6499: # @interface: The bus used for cdrom Line 6500: # Line 6501: # @index: Index on the bus used > not optional, so how do we handle the backward compatibility? I would keep the interface the same and add optional arguments to DriveSpec. Line 6502: # Line 6503: # @driveSpec: The specification of the new CD Line 6504: # Line 6505: # Returns: -- To view, visit https://gerrit.ovirt.org/56805 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I178c1a02bbad962f9dc9b67bed7691cf170ee896 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: cdrom: API change: require index
Nir Soffer has posted comments on this change. Change subject: cdrom: API change: require index .. Patch Set 1: (1 comment) https://gerrit.ovirt.org/#/c/56805/1/vdsm/API.py File vdsm/API.py: Line 112: def __init__(self, UUID): Line 113: APIBase.__init__(self) Line 114: self._UUID = UUID Line 115: Line 116: def changeCD(self, interface, index, driveSpec): Why not keep single parameter (driveSpec), and include the interface and the index inside it? This way the api remain backward compatible - old vdsm will ignore the new parameters, and new vdsm will use them. And we don't have to change so many layers, only the definition of drivespec will change. Line 117: """ Line 118: Change the CD in the specified VM. Line 119: Line 120: :param vmId: uuid of specific VM. -- To view, visit https://gerrit.ovirt.org/56805 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I178c1a02bbad962f9dc9b67bed7691cf170ee896 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: cdrom: API change: require index
Nir Soffer has posted comments on this change. Change subject: cdrom: API change: require index .. Patch Set 1: (1 comment) https://gerrit.ovirt.org/#/c/56805/1/vdsm/API.py File vdsm/API.py: Line 124 Line 125 Line 126 Line 127 Line 128 You forgot to use the new arguments -- To view, visit https://gerrit.ovirt.org/56805 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I178c1a02bbad962f9dc9b67bed7691cf170ee896 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: cdrom: API change: require index
Francesco Romani has posted comments on this change. Change subject: cdrom: API change: require index .. Patch Set 1: Code-Review-1 (1 comment) -1 for backward compatibility concerns https://gerrit.ovirt.org/#/c/56805/1/lib/api/vdsmapi-schema.json File lib/api/vdsmapi-schema.json: PS1, Line 6499: # @interface: The bus used for cdrom : # : # @index: Index on the bus used not optional, so how do we handle the backward compatibility? -- To view, visit https://gerrit.ovirt.org/56805 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I178c1a02bbad962f9dc9b67bed7691cf170ee896 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: cdrom: API change: require index
Martin Polednik has uploaded a new change for review.
Change subject: cdrom: API change: require index
..
cdrom: API change: require index
Previously, changeCD call did not require anything but path to an
image. This works fine as long as there is only single type of
cdrom for one architecture (and set number of cdroms & architectures).
As we slowly move towards using q35 machine type, we have to plan how
to fix the cdrom since q35 does not support IDE bus. Solution that
allows different cdrom buses
Change-Id: I178c1a02bbad962f9dc9b67bed7691cf170ee896
Signed-off-by: Martin Polednik
---
M client/vdsClient.py
M lib/api/vdsmapi-schema.json
M lib/vdsm/rpc/bindingxmlrpc.py
M tests/vmTests.py
M vdsm/API.py
M vdsm/virt/vm.py
6 files changed, 20 insertions(+), 16 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/05/56805/1
diff --git a/client/vdsClient.py b/client/vdsClient.py
index 6ea0283..dabf23f 100755
--- a/client/vdsClient.py
+++ b/client/vdsClient.py
@@ -359,8 +359,9 @@
def do_changeCD(self, args):
vmId = args[0]
-file = self._parseDriveSpec(args[1])
-return self.ExecAndExit(self.s.changeCD(vmId, file))
+interface, index = args[1], args[2]
+file = self._parseDriveSpec(args[3])
+return self.ExecAndExit(self.s.changeCD(interface, index, vmId, file))
def do_changeFloppy(self, args):
vmId = args[0]
@@ -2106,7 +2107,7 @@
'o optional: True|False'
)),
'changeCD': (serv.do_changeCD,
- (' ',
+ (' ',
'Changes the iso image of the cdrom'
)),
'changeFloppy': (serv.do_changeFloppy,
diff --git a/lib/api/vdsmapi-schema.json b/lib/api/vdsmapi-schema.json
index 4153f86..f875b6d 100644
--- a/lib/api/vdsmapi-schema.json
+++ b/lib/api/vdsmapi-schema.json
@@ -6494,9 +6494,13 @@
#
# Change the CD in the VM's CD-ROM device.
#
-# @vmID: The UUID of the VM
+# @vmID:The UUID of the VM
#
-# @driveSpec: The specification of the new CD
+# @interface: The bus used for cdrom
+#
+# @index: Index on the bus used
+#
+# @driveSpec: The specification of the new CD
#
# Returns:
# The VM definition, as updated
@@ -6504,7 +6508,8 @@
# Since: 4.10.0
##
{'command': {'class': 'VM', 'name': 'changeCD'},
- 'data': {'vmID': 'UUID', 'driveSpec': 'DriveSpec'},
+ 'data': {'vmID': 'UUID', 'interface': 'str', 'index': 'int',
+ 'driveSpec': 'DriveSpec'},
'returns': 'VmDefinition'}
##
diff --git a/lib/vdsm/rpc/bindingxmlrpc.py b/lib/vdsm/rpc/bindingxmlrpc.py
index 9dc6d35..b88525d 100644
--- a/lib/vdsm/rpc/bindingxmlrpc.py
+++ b/lib/vdsm/rpc/bindingxmlrpc.py
@@ -425,9 +425,9 @@
vm = API.VM(vmId)
return vm.setTicket(password, ttl, existingConnAction, params)
-def vmChangeCD(self, vmId, driveSpec):
+def vmChangeCD(self, vmId, interface, index, driveSpec):
vm = API.VM(vmId)
-return vm.changeCD(driveSpec)
+return vm.changeCD(interface, index, driveSpec)
def vmChangeFloppy(self, vmId, driveSpec):
vm = API.VM(vmId)
diff --git a/tests/vmTests.py b/tests/vmTests.py
index 1e45fda..6a514b7 100644
--- a/tests/vmTests.py
+++ b/tests/vmTests.py
@@ -1497,7 +1497,7 @@
fakevm._dom = fake.Domain(
virtError=libvirt.VIR_ERR_GET_FAILED)
-res = fakevm.changeCD({})
+res = fakevm.changeCD('ide', 2, {})
expected_status = define.errCode['changeDisk']['status']
self.assertEqual(res['status'], expected_status)
diff --git a/vdsm/API.py b/vdsm/API.py
index d474c26..a0fbcbe 100644
--- a/vdsm/API.py
+++ b/vdsm/API.py
@@ -113,12 +113,14 @@
APIBase.__init__(self)
self._UUID = UUID
-def changeCD(self, driveSpec):
+def changeCD(self, interface, index, driveSpec):
"""
Change the CD in the specified VM.
:param vmId: uuid of specific VM.
:type vmId: UUID
+:param interface: cdrom bus interface
+:param index: index of cdrom on the interface
:param driveSpec: specification of the new CD image. Either an
image path or a `storage`-centric quartet.
"""
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 6140baa..a165537 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -3750,12 +3750,8 @@
"Action: %s", self.name,
actionToString(action))
-def changeCD(self, drivespec):
-if cpuarch.is_ppc(self.arch):
-blockdev = 'sda'
-else:
-blockdev = 'hdc'
-
+def changeCD(self, interface, index, drivespec):
+blockdev = vmdevices.storage.makeName(interface, index)
return self._changeBlockDev('cdrom', blockdev, drivespec)
def changeFlopp
Change in vdsm[master]: cdrom: API change: require index
gerrit-hooks has posted comments on this change. Change subject: cdrom: API change: require index .. Patch Set 1: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6']) -- To view, visit https://gerrit.ovirt.org/56805 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I178c1a02bbad962f9dc9b67bed7691cf170ee896 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
