[pve-devel] [PATCH container 0/9] lxc pending changes

[Fabian Grünbichler]

On September 5, 2019 4:11 pm, Oguz Bektas wrote:
> this series makes it possible to add/revert/delete pending changes in
> the backend.
> 
> it depends on my previous patch for pve-guest-common, for enabling
> inheritance of pending changes related methods into PVE::LXC::Config

thanks for these patches! this functionality is a long-time overdue..

detailed comments/questions on individual patches (it's a lot, but I am 
also on vacation next week so you have plenty of time to prepare a v2 ;)).

the big picture already looks mostly good (there are some refactoring 
choices that I would do differently), but with such functionality the 
devil is always in the details/edge cases.

please test your v2 thoroughly, in particular regarding

- mountpoint handling
- backup/restore
- custom lxc.foo options
- clones
- migration
- various ways to restart containers (from within, stop/start, 
  systemctl, ..)

also, please think about which parts can be unit-tested and write tests for 
them!

if you find stuff about the interface or semantics from Qemu that is 
sub-optimal / unclear, don't just copy it, but try to understand and 
improve it.. duplicate or very similar code is already bad enough, 
two or three forks of BAD code is even worse ;) I haven't checked for 
everything whether particular lines are originally from you, or based on 
qemu-server - some / a lot of my comments might apply to qemu-server's 
implementation as well..

> 
> some notes or to-be-fixed-soon-after-review points:
>     [1]. gui patches are coming soon
>     [2]. right now pending changes are applied __only__ at vm_start.
>     [3]. --delete $opt --force is not implemented (is in schema, but has no effect atm)

unless you plan to implement this in this series, I'd drop it from the 
schema/API at least.

>     [4]. clones will __keep__ pending changes in config (for now)
>     [5]. there are a couple TODO/FIXME's inside, since they were extra
> features or unrelated bugs (which i think should be fixed in separate
> commits later since they don't directly affect pending changes functionality)

for small, well-understood stuff it often makes sense to fix it in 
separate commits up front. who knows when the next person takes a 
closer look at that particular code - a fix done now is done, a FIXME 
added now might be ignored forever.

> 
> comments:
> 
> [2]: i thought it was best to keep it like this for the first version,
> until we decide what's the best way to go about it.
> 
> [3]: it seemed quite tricky to implement live force-delete of mounpoints
> because of a few reasons, like unmount not being allowed or mp being on
> network storage etc.
> 
> [4]: will fix this in v2 after the first review
> 
> [5]: one bug, where if swap option is deleted while ct is running,
> memory.memsw.limit_in_bytes is set to infinite instead of zero. the
> other one is the --force implementation.

backup is also missing pending changes skipping. some more stuff in the 
review itself..

> Oguz Bektas (9):
>   add pending section to lxc config parser/writer
>   adapt config GET call for taking pending changes
>   adapt config PUT to use 'revert' and 'force' parameters
>   remove trailing whitespace
>   add 'pending' API method to LXC
>   add 'pct pending' command
>   add vmconfig_hotplug_pending and vmconfig_apply_pending
>   rework update_pct_config to write and apply pending changes
>   apply pending changes when container is started
> 
>  src/PVE/API2/LXC.pm        |  90 +++++-
>  src/PVE/API2/LXC/Config.pm |  88 +++---
>  src/PVE/CLI/pct.pm         |  27 ++
>  src/PVE/LXC.pm             |   7 +
>  src/PVE/LXC/Config.pm      | 549 ++++++++++++++++++++++++-------------
>  5 files changed, 520 insertions(+), 241 deletions(-)
> 
> -- 
> 2.20.1
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>