[pve-devel] [PATCH v2 00/18] lxc pending changes

[Fabian Grünbichler]

On September 30, 2019 2:44 pm, Oguz Bektas wrote:
> this series makes it possible to add/delete/revert pending changes in
> the backend for containers.
> 
> this v2 took longer than expected, mainly because there were small bugs
> popping up everywhere, everytime i tried to change anything :)
> 
> big thanks to fabian for the extensive review on v1, and for putting up
> with me :D

thanks for the v2! some detailed feedback on invidivual patches. except 
for the naming/order/splitting of commits (see below) and some open 
questions, this already looks quite good!

I'll give v3 a more detailed test run again, since some stuff will 
probably be moved/refactored/rewritten again.

> 
> 
> v1 -> v2:
> * better refactoring into guest-common, as suggested by fabian and
> thomas
> * fixed up some bugs (probably added some too):
>     * backup/restore
>     * cloning
>     * unlimited swap bug
>     * mountpoint handling (more on that)
>     * other stuff that i can't remember right now :D
> * small changes with style
> * in v1, mountpoints were a special-special case, since they were
> handled earlier in code before going into the hotplug/apply pending
> routines. after much discussion, they're now created/deleted in these
> phases instead of update_pct_config. unused disks can still be removed
> directly.
> * some other small changes around helper functions, mainly adding them
> support for handling $conf->{pending}
> 
> pve-container:
> 
> Oguz Bektas (11):
>   add lxc/pending API path
>   add 'pct pending'
>   adapt CT config parser for pending changes
>   use load_current_config for config GET call
>   skip pending changes while cloning
>   skip pending changes while taking backup
>   apply pending changes during container start
>   add revert parameter to config PUT
>   adapt config PUT method for the new update_pct_config
>   rework update_pct_config to write into pending section
>   add vmconfig_hotplug_pending and vmconfig_apply_pending

the order here is a bit wrong IMHO

#12 and #13 can go further up (they just make the later commits safe, 
and don't change anything without the later commits - good for bisecting 
;))

#14 relies on a method only introduced in #18
#15 adds an API parameter, but no code to handle it (I already told you 
this in v1 ;))
#16 reworks that API call further to adapt for changes that only come in 
patch #17
#17 relies on methods only introduced in #18

so, #18 needs to come first (if nobody calls those methods yet, it is 
clear that that change alone cannot break anything!)
#14 can come after, since it only has an effect if pending changes can 
exist (which, barring manual editing, is impossible with just the 
patches up to this point)
#15-17 are a single logical change (switching from "directly apply what 
is possible, die otherwise" to "write to pending, and either apply or 
hotplug depending on whether the CT is running"). no part of that works 
without the other, so just squash them into a single commit unless you 
find a *meaningful* way to split them up. sometimes commits are big, and 
that is okay as long as they are not big because lots of un- or 
semi-related stuff got thrown together.

> 
>  src/PVE/API2/LXC.pm        |  89 ++++++
>  src/PVE/API2/LXC/Config.pm |  82 ++----
>  src/PVE/CLI/pct.pm         |   3 +
>  src/PVE/LXC.pm             |  21 +-
>  src/PVE/LXC/Config.pm      | 566 +++++++++++++++++++++----------------
>  src/PVE/VZDump/LXC.pm      |   1 +
>  6 files changed, 457 insertions(+), 305 deletions(-)
> 
> qemu-server:
> 
> Oguz Bektas (4):
>   overwrite load_current_config from AbstractConfig
>   use load_current_config for config GET call
>   refactor pending changes related code
>   use format_pending from GuestHelpers for 'qm pending' command

those could almost be one commit, since they all do the same:
code moved to AbstractConfig/GuestHelpers, drop own copy and use shared 
one. at least the first two should be squashed for sure.

> 
>  PVE/API2/Qemu.pm  | 49 +++++------------------------
>  PVE/CLI/qm.pm     | 28 +----------------
>  PVE/QemuConfig.pm | 14 +++++++++
>  PVE/QemuServer.pm | 79 +++++------------------------------------------
>  4 files changed, 31 insertions(+), 139 deletions(-)
> 
> pve-guest-common:
> Oguz Bektas (3):
>   refactor pending changes related config code into AbstractConfig
>   refactor method used by config GET calls into AbstractConfig
>   refactor code from qm/pct 'pending' call into AbstractConfig
> 
>  PVE/AbstractConfig.pm | 103 ++++++++++++++++++++++++++++++++++++++++++
>  PVE/GuestHelpers.pm   |  26 +++++++++++
>  2 files changed, 129 insertions(+)
> 
> -- 
> 2.20.1
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>