Skip to content

VR: fail the LB config apply when haproxy.cfg cannot be replaced - #14636

Open
bhouse-nexthop wants to merge 1 commit into
apache:4.22from
bhouse-nexthop:vr-haproxy-copy-failure
Open

bhouse-nexthop wants to merge 1 commit into
apache:4.22from
bhouse-nexthop:vr-haproxy-copy-failure

Conversation

@bhouse-nexthop

Copy link
Copy Markdown
Collaborator

Description

This PR makes the virtual router fail a load balancer config apply when it can't install the new HAProxy config, instead of reporting success.

CsLoadBalancer.process() installed haproxy.cfg.new over haproxy.cfg with CsHelper.copy(), which logs a failed copy and returns. If the file couldn't be replaced (an immutable or read-only file, or a full filesystem), the router reloaded HAProxy on the old file and the config run ended successfully. The management server then treated the change as applied and didn't roll it back.

The copy is now done with shutil.copy2() directly. When it fails, the error is logged and an exception is raised, in the same way as for a config that fails haproxy -c. HAProxy isn't reloaded, the config run fails, and the management server gets a failed answer. CsHelper.copy() itself is unchanged, since its other callers rely on it not raising.

What happens after the raise is the same as for an invalid config today:

  • The rest of the iptables pass in that config run, which comes after the load balancer in IpTablesExecutor.process(), is not applied.
  • haproxy.cfg.new is left in place. The next apply writes it again and retries the copy.

Fixes: #14633

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

How Has This Been Tested?

  • New systemvm/test/TestCsLoadBalancer.py, run with pytest on Python 3.12 (PYTHONPATH=../debian/opt/cloud/bin):
    • a changed config is installed, HAProxy is reloaded once, and the firewall rules are added;
    • when the copy raises PermissionError, process() raises, HAProxy isn't reloaded, no firewall rule is added, and haproxy.cfg keeps the old content.
  • The second test fails without this change, because the copy error is swallowed.
  • pycodestyle --max-line-length=179 is clean on the new test. On CsLoadBalancer.py it reports only an E302 that is already there before this change.
  • The bug was found in a lab: with chattr +i /etc/haproxy/haproxy.cfg on the router, updateLoadBalancerRule returned success, the router logged Could not copy ... and kept the old config. This change has not been run on a router yet. To test it live: repeat that step and check that the update fails, the rule keeps its old values, and HAProxy keeps running on the old config; then run chattr -i and check that the same update succeeds.

CsLoadBalancer installs the new HAProxy config with CsHelper.copy(),
which logs an IOError and returns. When /etc/haproxy/haproxy.cfg
cannot be written (read-only or immutable file, full filesystem), the
router logs "Could not copy ... haproxy.cfg.new to ... haproxy.cfg",
reloads HAProxy on the old file and reports success. The management
server then records the load balancer change as applied although the
router still runs the old config, and nothing is rolled back.

Copy the file directly and raise when the copy fails, as is already
done when the new config fails "haproxy -c". The apply then fails, the
router is not reloaded, and the management server sees the failure.

Signed-off-by: Brad House <bhouse@nexthop.ai>
@codecov

codecov Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 18.31%. Comparing base (d02adcd) to head (221db45).

Additional details and impacted files
@@             Coverage Diff              @@
##               4.22   #14636      +/-   ##
============================================
- Coverage     18.32%   18.31%   -0.01%     
+ Complexity    16644    16639       -5     
============================================
  Files          5939     5939              
  Lines        537768   537768              
  Branches      65929    65929              
============================================
- Hits          98551    98504      -47     
- Misses       428008   428063      +55     
+ Partials      11209    11201       -8     
Flag Coverage Δ
uitests 4.08% <ø> (ø)
unittests 19.40% <ø> (-0.01%) ⬇️

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.

@bhouse-nexthop

Copy link
Copy Markdown
Collaborator Author

Test report: load balancer change when the router can't replace haproxy.cfg

Summary: with /etc/haproxy/haproxy.cfg immutable on the router, updateLoadBalancerRule now fails, the rule keeps its old value, and HAProxy keeps running on the old config. After chattr -i, the same update succeeds and is applied.

Environment

  • A CloudStack 24.0 (main) build with this PR, alongside some of my other open PRs. Three KVM hosts, one management server, system VM template 4.22.0.
  • The PR targets 4.22; it wasn't run on a 4.22 build. 4.22's CsLoadBalancer.process() installs the file with CsHelper.copy(), as main did before this change.
  • A new isolated network with source NAT, its virtual router, and one VM. The router was created after the upgrade, so it runs this build's scripts: CsLoadBalancer.py on the router contains the new shutil.copy2 call.
  • A load balancer rule on a second public IP: port 80, roundrobin, with the VM assigned.

Steps and results

Step Result
A: chattr +i /etc/haproxy/haproxy.cfg on the router, then updateLoadBalancerRule algorithm=leastconn fails, 530
Rule after A algorithm: roundrobin, Active: rolled back
Router after A haproxy.cfg still has balance roundrobin; HAProxy still running with the same PID
B: chattr -i, then the same update succeeds
Rule and router after B algorithm: leastconn; haproxy.cfg has balance leastconn; HAProxy reloaded (new PID)

Evidence for A

  • The router's /var/log/cloud.log:
    21:38:47,515 INFO   Wrote edited file /etc/haproxy/haproxy.cfg.new
    21:38:47,515 INFO   Executing2: haproxy -c -f /etc/haproxy/haproxy.cfg.new
    21:38:47,543 ERROR  Could not copy /etc/haproxy/haproxy.cfg.new to /etc/haproxy/haproxy.cfg: [Errno 1] Operation not permitted: '/etc/haproxy/haproxy.cfg'
    
    There is no reload after the error.
  • Management server:
    21:38:47,751 WARN  LoadBalancingRulesManagerImpl  Unable to apply the load balancer config because resource is unavailable. com.cloud.exception.ResourceUnavailableException: ...
    
    The API answers Failed to update load balancer rule: ... with error code 530.
  • Before this PR, the same step returned success, listLoadBalancerRules showed leastconn, and the router kept roundrobin.

Result: pass. The management server now sees the failed install, rolls the rule back, and the router keeps serving the old, consistent config. Once the file is writable again, the change goes through.

Side note, not from this PR: the failed updateLoadBalancerRule is logged at ERROR as Unexpected exception while executing ... UpdateLoadBalancerRuleCmd (the CloudRuntimeException reaches the dispatcher). That is how this command reports any failed apply.

Not covered

  • A full filesystem or a read-only mount, the other causes the PR mentions: same code path, but not reproduced.
  • A VPC router: the same CsLoadBalancer.py runs there.
  • Adding a new rule, rather than changing one, while the file is immutable.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VR reports a load balancer change as applied when haproxy.cfg cannot be replaced

2 participants