Skip to content

Commit 2b830dc

Browse files
authored
[3/n] [installinator] make DiscoverPeersError::Abort test-only (#8037)
The production installinator must keep retrying until it succeeds. We use `DiscoverPeersError::Abort` for some of our tests, so gate it on `cfg(test)`.
1 parent 14b96b8 commit 2b830dc

File tree

3 files changed

+24
-8
lines changed

3 files changed

+24
-8
lines changed

installinator/src/errors.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,13 @@ pub(crate) enum DiscoverPeersError {
3333
#[allow(unused)]
3434
Retry(#[source] anyhow::Error),
3535

36-
#[error("failed to discover peers (no more retries left, will abort)")]
37-
#[allow(unused)]
36+
/// Abort further discovery.
37+
///
38+
/// The installinator must keep retrying until it has completed, which is why
39+
/// there's no abort case here in the not cfg(test) case. However, we test
40+
/// some abort-related functionality in tests.
41+
#[cfg(test)]
42+
#[error("failed to discover peers (will abort)")]
3843
Abort(#[source] anyhow::Error),
3944
}
4045

installinator/src/peers.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,9 @@ impl DiscoveryMechanism {
5353
) -> Result<Vec<PeerAddress>, DiscoverPeersError> {
5454
let peers = match self {
5555
Self::Bootstrap => {
56-
// XXX: consider adding aborts to this after a certain number of tries.
57-
56+
// Note: we do not abort this process and instead keep retrying
57+
// forever. This attempts to ensure that we'll eventually find
58+
// peers.
5859
let ddm_admin_client =
5960
DdmAdminClient::localhost(log).map_err(|err| {
6061
DiscoverPeersError::Retry(anyhow::anyhow!(err))
@@ -164,6 +165,7 @@ impl FetchedArtifact {
164165
tokio::time::sleep(RETRY_DELAY).await;
165166
continue;
166167
}
168+
#[cfg(test)]
167169
Err(DiscoverPeersError::Abort(error)) => {
168170
return Err(error);
169171
}

installinator/src/reporter.rs

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use display_error_chain::DisplayErrorChain;
1010
use futures::{Future, StreamExt};
1111
use installinator_common::{Event, EventBuffer};
1212
use tokio::{sync::mpsc, task::JoinHandle, time};
13+
use update_engine::AsError;
1314
use uuid::Uuid;
1415

1516
use crate::{errors::DiscoverPeersError, peers::Peers};
@@ -97,16 +98,24 @@ where
9798
tokio::spawn(async move {
9899
let peers = match (discover_fn)().await {
99100
Ok(peers) => peers,
100-
Err(error) => {
101-
// Ignore DiscoverPeersError::Abort here because the
102-
// installinator must keep retrying.
101+
Err(DiscoverPeersError::Retry(error)) => {
103102
slog::warn!(
104103
log,
105104
"failed to discover peers: {}",
106-
DisplayErrorChain::new(&error),
105+
DisplayErrorChain::new(error.as_error()),
107106
);
108107
return None;
109108
}
109+
#[cfg(test)]
110+
Err(DiscoverPeersError::Abort(_)) => {
111+
// This is not possible, since test implementations don't
112+
// currently generate Abort errors during reporter
113+
// discovery.
114+
unreachable!(
115+
"DiscoverPeersError::Abort is not generated for the \
116+
reporter by test implementations"
117+
)
118+
}
110119
};
111120

112121
// We could use StreamExt::any() here, but we're choosing to avoid

0 commit comments

Comments
 (0)