Skip to content

Commit b280ae5

Browse files
engine: do not stall the host command queue on a failed send in AgentAttache.sendNext
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.
1 parent 3d70ce4 commit b280ae5

2 files changed

Lines changed: 89 additions & 1 deletion

File tree

engine/orchestration/src/main/java/com/cloud/agent/manager/AgentAttache.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -532,11 +532,15 @@ protected synchronized void sendNext(final long seq) {
532532
logger.debug(LOG_SEQ_FORMATTED_STRING, req.getSequence(), "Sending now. is current sequence.");
533533
try {
534534
send(req);
535+
_currentSequence = req.getSequence();
535536
} catch (AgentUnavailableException e) {
536537
logger.debug(LOG_SEQ_FORMATTED_STRING, req.getSequence(), "Unable to send the next sequence");
537538
cancel(req.getSequence());
539+
// The failed command was cancelled, so no answer will ever drive sendNext again.
540+
// Move on to the next queued request instead of parking _currentSequence on the dead
541+
// sequence, which would stall every later in-sequence command to this host.
542+
sendNext(req.getSequence());
538543
}
539-
_currentSequence = req.getSequence();
540544
}
541545

542546
public void process(final Answer[] answers) {
Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
package com.cloud.agent.manager;
18+
19+
import org.junit.Assert;
20+
import org.junit.Test;
21+
import org.mockito.Mockito;
22+
23+
import com.cloud.agent.transport.Request;
24+
import com.cloud.exception.AgentUnavailableException;
25+
import com.cloud.host.Status;
26+
27+
public class AgentAttacheSendNextTest {
28+
29+
/**
30+
* Minimal concrete AgentAttache: send() fails for one designated sequence and succeeds otherwise,
31+
* recording the sequence that was actually dispatched.
32+
*/
33+
static class TestAgentAttache extends AgentAttache {
34+
Long sentSeq;
35+
final long failSeq;
36+
37+
TestAgentAttache(long failSeq) {
38+
super(null, 1L, "uuid-1", "host-1", null, false);
39+
this.failSeq = failSeq;
40+
}
41+
42+
@Override
43+
public void send(Request req) throws AgentUnavailableException {
44+
if (req.getSequence() == failSeq) {
45+
throw new AgentUnavailableException("simulated transient link failure", _id);
46+
}
47+
sentSeq = req.getSequence();
48+
}
49+
50+
@Override
51+
public void disconnect(Status state) {
52+
}
53+
54+
@Override
55+
protected boolean isClosed() {
56+
return false;
57+
}
58+
}
59+
60+
@Test
61+
public void sendNextAdvancesPastAFailedCommandToTheNextQueued() {
62+
long failSeq = 100L;
63+
long goodSeq = 200L;
64+
65+
Request failing = Mockito.mock(Request.class);
66+
Mockito.when(failing.getSequence()).thenReturn(failSeq);
67+
Request good = Mockito.mock(Request.class);
68+
Mockito.when(good.getSequence()).thenReturn(goodSeq);
69+
70+
TestAgentAttache attache = new TestAgentAttache(failSeq);
71+
attache._requests.add(failing);
72+
attache._requests.add(good);
73+
74+
attache.sendNext(1L);
75+
76+
// A command whose send() failed (and was cancelled) must NOT become _currentSequence: no answer
77+
// will ever arrive for it, so every later in-sequence command to this host would queue behind it
78+
// and time out. sendNext must move on and dispatch the next queued command instead.
79+
Assert.assertEquals("the next queued command should have been dispatched", Long.valueOf(goodSeq), attache.sentSeq);
80+
Assert.assertEquals("current sequence must be the successfully sent command, not the failed one",
81+
Long.valueOf(goodSeq), attache._currentSequence);
82+
Assert.assertTrue("the request queue should be drained", attache._requests.isEmpty());
83+
}
84+
}

0 commit comments

Comments
 (0)