engine: do not stall the host command queue on a failed send in Agent…

[nagaboinaramgopal]

Description

When sending the next queued request threw AgentUnavailableException, the request
was cancelled but _currentSequence was still set to that dead sequence. No answer
ever arrives for a cancelled command, so sendNext was never driven again and every
later in-sequence command to the host queued behind it and timed out, until the
attache was rebuilt.

Advance _currentSequence only on a successful send, and on failure move on to the
next queued request.

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Feature/Enhancement Scale or Bug Severity

Bug Severity

• Minor

How Has This Been Tested?

Added a unit test where the first queued command fails to send and asserts the
next queued command is dispatched and becomes the current sequence, instead of
stalling on the failed one. Also built the standard packages and deployed on a KVM
advanced zone.

[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.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 17.21%. Comparing base (2cd8c5e) to head (4d8e427).

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20   #14040      +/-   ##
============================================
+ Coverage     16.34%   17.21%   +0.86%     
- Complexity    13574    13580       +6     
============================================
  Files          5669     5264     -405     
  Lines        501368   468343   -33025     
  Branches      60903    55005    -5898     
============================================
- Hits          81964    80632    -1332     
+ Misses       410219   378694   -31525     
+ Partials       9185     9017     -168     

Flag	Coverage Δ
uitests	?
unittests	17.21% <100.00%> (+<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.

[blueorangutan]

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 19095

[DaanHoogland]

@nagaboinaramgopal , an compile error in your test code:

09:17:39 [ERROR] /jenkins/workspace/acs-centos8-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.20.4.0-SNAPSHOT/engine/orchestration/src/test/java/com/cloud/agent/manager/AgentAttacheSendNextTest.java:[38,12] error: constructor AgentAttache in class AgentAttache cannot be applied to given types;
09:17:39   required: AgentManagerImpl,long,String,String,boolean
09:17:39   found: <null>,long,String,String,<null>,boolean
09:17:39   reason: actual and formal argument lists differ in length
09:17:39 [INFO] 1 error

can you have a look?

[nagaboinaramgopal]

@nagaboinaramgopal , an compile error in your test code:

09:17:39 [ERROR] /jenkins/workspace/acs-centos8-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.20.4.0-SNAPSHOT/engine/orchestration/src/test/java/com/cloud/agent/manager/AgentAttacheSendNextTest.java:[38,12] error: constructor AgentAttache in class AgentAttache cannot be applied to given types;
09:17:39   required: AgentManagerImpl,long,String,String,boolean
09:17:39   found: <null>,long,String,String,<null>,boolean
09:17:39   reason: actual and formal argument lists differ in length
09:17:39 [INFO] 1 error

can you have a look?

fixed and pushed, Thanks Daan

[blueorangutan]

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 19123

[blueorangutan]

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

[DaanHoogland]

@nagaboinaramgopal , as you are not referring an issue, can you add a reporduction scheme/scenario?

[nagaboinaramgopal]

@nagaboinaramgopal , as you are not referring an issue, can you add a reporduction scheme/scenario?

No linked issue on this one, it came from following the in-sequence command path in AgentAttache, so here is the scenario, and it reproduces deterministically as a fail-before / pass-after test.

For a given host, in-sequence commands run one at a time, and _currentSequence holds the sequence that is currently in flight. While it is non-null, any newly arriving in-sequence command is queued in _requests rather than sent (the executeInSequence() && _currentSequence != null branch in send(...)).

Sequence that stalls the queue:

1. Command A is in flight to the host, so _currentSequence = A. Commands B and C arrive meanwhile and queue behind it.
2. A completes, so the answer path calls sendNext(...), which pops B and calls send(B).
3. send(B) throws AgentUnavailableException. This is the transient case the catch block exists for: the agent link is briefly down or reconnecting, or a socket write fails as the command goes out.
4. Before this change, sendNext cancelled B but then still ran _currentSequence = B at the end of the method. B was just cancelled, so no answer will ever arrive for it.
5. _currentSequence now points at a dead sequence. C, and every later in-sequence command to this host, takes the _currentSequence != null path and waits for B. The queue only unwedges when the outer send(req, wait) hits its full timeout and clears _currentSequence, so in the meantime each in-sequence command to that host eats a timeout.

AgentAttacheSendNextTest.sendNextAdvancesPastAFailedCommandToTheNextQueued pins this: it queues a command whose send() fails followed by a good one, then asserts sendNext dispatches the good command and leaves _currentSequence on it.

On the unpatched code the test fails, which is exactly the stall:

java.lang.AssertionError: the next queued command should have been dispatched expected:<200> but was:<null>
Tests run: 1, Failures: 1

With the change it passes (the queued command is dispatched instead of the queue parking on the cancelled one):

Tests run: 1, Failures: 0

I could not tie it to a specific cluster incident, since it needs a send failure at the exact moment the queue is drained, but that is the path and the test reproduces it deterministically.