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@pve.proxmox.com > https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
