Fix vm cleanup iptables/ipset misleading error logs (Fixes #12770)

[dheeraj12347]

Problem

During KVM VM cleanup, security_group.py tries to flush and delete iptables/ip6tables chains and ipsets unconditionally. If the chain or set has already been removed (for example, when cleanup is rerun or after partial failures), the underlying commands fail, and the agent logs misleading ERROR messages and stack traces, even though there is nothing left to clean up. This makes idempotent cleanup noisy and harder to debug. [web:2]

Changes

• Added helper functions:

  • iptables_chain_exists(chain)
  • ip6tables_chain_exists(chain)
  • ipset_exists(setname)

  These use the existing execute() wrapper to check whether a chain or ipset exists. Exit code 1 (not found) is treated as a normal condition: it is logged at DEBUG and returns False, while other non‑zero exit codes are still raised as real errors. [file:69]

• Updated destroy_network_rules_for_vm() to:

  • Check iptables and ip6tables chain existence via the new helpers before flushing and deleting them.
  • Check IPv4 and IPv6 ipset existence before flushing and deleting them.
  • Log missing chains and ipsets at DEBUG level as “does not exist, skipping”.
  • Keep logging real failures when flushing/deleting chains or ipsets at ERROR level.
  • Preserve existing behavior for ebtables cleanup, DNAT rule removal, and rule/log file cleanup. [file:69]

Result

• VM cleanup for KVM is now idempotent and no longer emits misleading ERROR logs when chains or ipsets are already gone.
• Real failures during cleanup are still surfaced as ERROR messages with details, improving signal‑to‑noise in agent logs. [file:69][web:2]

[Copilot]

Pull request overview

Improves KVM VM security-group cleanup in security_group.py by making iptables/ip6tables chain and ipset removal more idempotent, aiming to reduce misleading ERROR logs when cleanup is re-run and resources are already gone.

Changes:

• Added helper functions to detect existence of iptables/ip6tables chains and ipsets before attempting flush/delete.
• Updated destroy_network_rules_for_vm() to conditionally flush/delete chains and ipsets, and to log “missing” cases at DEBUG.
• Tightened exception handling in DNAT cleanup blocks (catch Exception explicitly).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 17.82%. Comparing base (27bce46) to head (2b9a340).
⚠️ Report is 286 commits behind head on 4.22.

Additional details and impacted files

@@             Coverage Diff              @@
##               4.22   #12850      +/-   ##
============================================
+ Coverage     17.61%   17.82%   +0.21%     
- Complexity    15662    16019     +357     
============================================
  Files          5917     5928      +11     
  Lines        531415   534404    +2989     
  Branches      64973    65399     +426     
============================================
+ Hits          93588    95266    +1678     
- Misses       427271   428319    +1048     
- Partials      10556    10819     +263     

Flag	Coverage Δ
uitests	3.82% <ø> (+0.11%)	⬆️
unittests	18.90% <ø> (+0.22%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[DaanHoogland]

@dheeraj12347 can you assess co-pilot’s comments?

[dheeraj12347]

Hi @DaanHoogland , I reviewed the Copilot comments and addressed all three.

• The existence checks now use check_output() directly, so expected return code 1 does not go through execute() and produce an ERROR traceback.
• ip6tables command-not-found (return code 127) is handled as a DEBUG/skip during cleanup.
• ipset command-not-found (return code 127) is handled similarly.

I also ran the Python syntax check and git diff --check locally.

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[DaanHoogland]

@weizhouapache , maybe this one is a good excuse to clean up the security groups smoke test set.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19126

[weizhouapache]

@weizhouapache , maybe this one is a good excuse to clean up the security groups smoke test set.

yes, the smoke test on advanced zone is useless for this PR

[DaanHoogland]

@blueorangutan help

[blueorangutan]

@DaanHoogland [SL] I understand these words: "help", "hello", "thanks", "package", "test", "sweep", "shutup"
Test command usage: test [mgmt os] [hypervisor] [keepEnv] [qemuEv] [basicZone|securityGroups]
Mgmt OS options: ['alma10', 'ol10', 'rocky10', 'ol8', 'ol9', 'debian12', 'alma8', 'alma9', 'suse15', 'ubuntu20', 'centos7', 'centos6', 'rocky8', 'ubuntu18', 'ubuntu22', 'rocky9', 'ubuntu26', 'ubuntu24']
Hypervisor options: ['kvm-centos6', 'kvm-centos7', 'kvm-rocky8', 'kvm-rocky9', 'kvm-rocky10', 'kvm-ol8', 'kvm-ol9', 'kvm-ol10', 'kvm-alma8', 'kvm-alma9', 'kvm-alma10', 'kvm-ubuntu18', 'kvm-ubuntu20', 'kvm-ubuntu22', 'kvm-ubuntu24', 'kvm-ubuntu26', 'kvm-debian12', 'kvm-suse15', 'vmware-55u3', 'vmware-60u2', 'vmware-65u2', 'vmware-67u3', 'vmware-70u1', 'vmware-70u2', 'vmware-70u3', 'vmware-80', 'vmware-80u1', 'vmware-80u2', 'vmware-80u3', 'vmware-80u3e', 'xenserver-65sp1', 'xenserver-71', 'xenserver-74', 'xenserver-84', 'xcpng74', 'xcpng76', 'xcpng80', 'xcpng81', 'xcpng82', 'xcpng83']
Note: when keepEnv is passed, you need to specify mgmt server os and hypervisor or use the matrix command.
when qemuEv is passed, it will deploy KVM hyperviosr hosts with qemu-kvm-ev, else it will default to stock qemu.
When basicZone and/or securityGroups are passed it will create a zone of the last type specified (default is Advanced)
Package command usage: package [all(default value),kvm,xen,vmware,hyperv,ovm] - a comma separated list can be passed with package command to bundle the required hypervisor's systemVM templates. Not passing any argument will bundle all - kvm,xen and vmware templates.
Sweep command usage: sweep - strips attached archive/file links from my own comments on this PR, marking them as removed.
Shutup command usage: shutup - deletes all of my comments on this PR.

Blessed contributors for kicking Trillian test jobs: ['shwstppr', 'Damans227', 'vishesh92', 'Pearl1594', 'harikrishna-patnala', 'nvazquez', 'DaanHoogland', 'weizhouapache', 'borisstoyanov', 'vladimirpetrov', 'kiranchavala', 'prashanthr2', 'ingox', 'NuxRo', 'rajujith', 'sureshanaparti', 'abh1sar', 'sudo87', 'RosiKyu']

[DaanHoogland]

@blueorangutan test securityGroups keepEnv

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests