[WIP] [FEAT-18702] LMCT - Supporting Zone Based Conntrack Migration#17
[WIP] [FEAT-18702] LMCT - Supporting Zone Based Conntrack Migration#17abhishekgangwar-ntnx wants to merge 35 commits into
Conversation
mansish404
left a comment
There was a problem hiding this comment.
Review is mainly around code structuring, naming conventions, and coding style for now. Please address the necessary comments first.
Once the cleanup is done, I’ll review the deeper functionality and workflows.
One suggestion while making changes of this scale — try to keep variables and functions as close to the feature implementation as possible. Using generic terms like strides/offsets is fine where it makes sense, but avoid over-engineering the abstractions.
The code should be easy for someone new to understand and immediately identify what functionality is being implemented. Right now, because of the naming conventions and abstractions used, it’s difficult to map the code back to the actual feature being introduced.
Ideally, a developer should not need to refer to the design doc just to understand what functionality this patch is implementing.
…g properly , the second tuple's proto was not set: corrected it
- dmain: goto-cleanup; on error, call dbus_server_request_quit before pthread_join so the dbus thread doesn't block forever in g_main_loop_run. - dbus_server: add dbus_server_request_quit, plus loop/should_quit/ loop_mu on dbus_targs so the quit signal is race-free against a thread still on its way to g_main_loop_new. - apply_zone_rewrite: use g_hash_table_lookup_extended so an old_zone -> 0 remap isn't silently dropped. - conntrack_events_callback: nfct_destroy(ct) on all early returns (validate_ct_entry fail, zone-mismatch) via a single label. - append_ct_to_batch: return int; on nfct_nlmsg_build failure the caller drops the entry and skips mnl_nlmsg_batch_next so the partial slot is overwritten instead of sent to the kernel.
Adds ensure_cli_arg_is_int_at_least() in common.{h,c} — strtol-based,
errx's on empty/non-numeric/negative input and int overflow, base locked
to 10. Routes all 10 atoi() sites in main.c through it with per-arg
minimums as named constants (MIN_ACCEPTABLE_VALUE_FOR_{MODE,
NUM_ENTRIES, NUM_IPS}). dmain now also runs check_mode() after parsing
so out-of-set modes can't reach the post-fork grandchild.
The save_targets->ports_to_migrate hashtable (port_uuid -> CT zone) was built and freed but never read; every consumer of save_targets reads only ->kind, ->ips_to_migrate and ->zones_to_migrate. Removes the field and all the plumbing that fed it
Promote struct save_targets, struct ct_delete_args, and struct delete_ct_dump_cb_args from documented tagged unions to real anonymous unions over the active SAVE sub-mode. Anonymous inner structs preserve flat field access at call sites (e.g. cb_args->ips_migrated, targets->zones_to_migrate), while the two mode arms now share storage and are mutually exclusive at the type level.
dmain() so it stops re-walking argv after the double fork (the old "parent's stack is gone after fork" comment was wrong; fork copies the entire AS). detect_*_input_kind now publishes its already-parsed N via an out-param so check_args adds zero extra parses on argv[3].
based clear rpc invocation.
GDBus callbacks, post-fork dmain, libc-facing check_args) and at the helpers that take caller-supplied pointers or counts (hashtable builders, save_targets/load_targets allocators, conntrack_entry builders).
…locator independent of argv layout (mirroring the SAVE side). Optimize duplicate old_zone check via g_hash_table_lookup_extended() Tighten n_entries guard to <= 0 Replace magic +1/+2 offsets with named variables
…d helpers from start_in_save_mode, making it clean and readable plus ensuring that the events-create error path now correctly wakes and joins the delete thread rather than leaving it orphaned on clear_called_cond. Additionally, it replaces the hand-rolled UUID validator with uuid_parse() from libuuid and updates the Makefile and README to include the necessary -luuid flag and libuuid-devel build dependencies.
…d from future risks
… listen_for_conntrack_events (since the zone path never touches them). the LOAD-mode (old_zone -> new_zone) remap build is done while check_zone_load_args. This allows it to happen just once during pre-fork CLI validation
…attributes do not corrupt the migrator and changed data_template_new to create data template as per the requirement (if ip mode then 21 attr otherwise 25)
ffed51e to
e52301e
Compare
… it might lead to inconsistency and wrong result, because there is no separation of zones in our setup thus they are not even required.
…e save and load mode enums separate to keep the readability for parameters in several methods
Command-Line Interface Updates
Zone-Based Filtering & Entry Capture
Zone Remapping on Load
Manual Testing
Note