Hello Aaron, I read all your comments and most of them seem reasonable. I guess the only one that I could argue against is the one on patch 9 that I've already replied upon. This means that Alex will be happy to do all the changes you propose. The only problem is that Alex will be out of work until the end of the month and won't be able to reply to your comments (that's why I'm doing it :) ). So would it be possible to have the second reviewer review the patch now, so that Alex can incorporate the comments of both once he gets back? I think we'll save a lot of time this way.
Thanks a lot, Constantinos On 11/10/14, 17:48 PM, 'Aaron Karper' via ganeti-devel wrote: > On Thu, Nov 06, 2014 at 12:30:34AM +0200, Alex Pyrgiotis wrote: > > Hi everybody, > > > > This patch series implements the necessary logic for the attach/detach > > operation on disks. This functionality will allow users to remove a disk > > from an instance and attach it later on to another one, or keep it for > > forensic reasons. The latter part means that the Ganeti configuration > > now supports having detached disks. > > > > In order to document this new functionality, we have extended the man > > page for gnt-instance, as well as the design doc for disks. Also, to > > make sure that we don't introduce any new bugs, we have added many new > > unit tests and extended older ones. Moreover, we have extended the > > burnin tool in order to live-test this new functionality on an existing > > cluster. > > > > Finally, a related patch about file-based instances is in the works and > > will be sent in a couple of days. > > > > Cheers, > > Alex > > > > Alex Pyrgiotis (19): > > config: Use disk name to get its info > > config: Fix docstring of _UnlockedGetInstanceNodes > > cmdlib: Turn index code to functions > > cmdlib: Return RPC result in AssembleInstanceDisks > > config: Create wrappers for attach/detach ops > > constants: Add attach/detach and UUID constant > > cmdlib: Add checks for attach/detach > > cmdlib: Add core attach/detach logic > > config: Remove checks for detached disks > > burnin: Test if attach/detach works properly > > tests.config: Add test for GetDiskInfoByName > > tests.cmdlib: Test index operations > > tests.opcodes/client: Add test for constants > > tests.config: Test attach/detach wrappers > > tests.cmdlib: Check if attach/detach checks work > > tests.cmdlib: Extend tests for _ApplyContainerMods > > tests.cluster: Add test for VerifyConfig > > doc: Extend the design document for disks > > man: Update the gnt-instance man page > > > > Ilias Tsitsimpis (2): > > Implement GetAllDisksInfo in config > > AllocateDRBDMinor per disk, not per instance > > > > doc/design-disks.rst | 49 ++- > > lib/client/gnt_instance.py | 54 +++- > > lib/cmdlib/cluster.py | 35 ++- > > lib/cmdlib/instance.py | 10 +- > > lib/cmdlib/instance_create.py | 3 +- > > lib/cmdlib/instance_migration.py | 4 +- > > lib/cmdlib/instance_set_params.py | 185 +++++++++-- > > lib/cmdlib/instance_storage.py | 47 +-- > > lib/cmdlib/instance_utils.py | 97 ++++-- > > lib/config.py | 138 ++++++-- > > lib/tools/burnin.py | 122 ++++++-- > > man/gnt-instance.rst | 25 +- > > src/Ganeti/Config.hs | 1 + > > src/Ganeti/Constants.hs | 8 +- > > src/Ganeti/Types.hs | 4 + > > src/Ganeti/WConfd/Core.hs | 14 +- > > src/Ganeti/WConfd/TempRes.hs | 40 +-- > > test/py/cmdlib/cluster_unittest.py | 6 + > > test/py/cmdlib/instance_unittest.py | 399 > ++++++++++++++++++++---- > > test/py/cmdlib/testsupport/config_mock.py | 18 +- > > test/py/ganeti.client.gnt_instance_unittest.py > <http://ganeti.client.gnt_instance_unittest.py> | 20 +- > > test/py/ganeti.config_unittest.py > <http://ganeti.config_unittest.py> | 96 ++++++ > > test/py/ganeti.opcodes_unittest.py > <http://ganeti.opcodes_unittest.py> | 3 + > > 23 files changed, 1097 insertions(+), 281 deletions(-) > > > > -- > > 1.7.10.4 > > > > Hi Alex, > Thanks for adding this feature. I will be doing a first pass of review, > but since I don't have push rights, there will be a second reviewer. > > > -- > Google Germany GmbH > Dienerstr. 12 > 80331 München > > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Geschäftsführer: Graham Law, Christine Elizabeth Flores
