Skip to content

Commit

Permalink
controller: Stop panicking when unable to create pod.
Browse files Browse the repository at this point in the history
Currently the controller panic if it encounters an error while trying to
create a pod/job for an instance, thus causing an impact far wider than
just the failing instance.

This should only have impact on the said instance, so as a first step,
this commit stop the controller from panicking in this case and just
output a log entry.

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
  • Loading branch information
diconico07 committed Jun 6, 2023
1 parent ce58a70 commit 313e757
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 8 deletions.
51 changes: 44 additions & 7 deletions controller/src/util/instance_action.rs
Original file line number Diff line number Diff line change
Expand Up @@ -419,20 +419,27 @@ pub async fn handle_instance_change(
}
};
if let Some(broker_spec) = &configuration.spec.broker_spec {
match broker_spec {
let instance_change_result = match broker_spec {
BrokerSpec::BrokerPodSpec(p) => {
handle_instance_change_pod(instance, p, action, kube_interface).await
}
BrokerSpec::BrokerJobSpec(j) => {
handle_instance_change_job(
instance.clone(),
instance,
*configuration.metadata.generation.as_ref().unwrap(),
j,
action,
kube_interface,
)
.await
}
};
match instance_change_result {
Ok(_) => Ok(()),
Err(e) => {
error!("Unable to handle Broker action: {:?}", e);
Ok(())
}
}
} else {
Ok(())
Expand All @@ -444,7 +451,7 @@ pub async fn handle_instance_change(
/// InstanceAction::Remove => Delete all Jobs labeled with the Instance name
/// InstanceAction::Update => No nothing
pub async fn handle_instance_change_job(
instance: Instance,
instance: &Instance,
config_generation: i64,
job_spec: &JobSpec,
action: &InstanceAction,
Expand Down Expand Up @@ -472,7 +479,7 @@ pub async fn handle_instance_change_job(
trace!("handle_instance_change_job - instance added");
let capability_id = format!("{}/{}", AKRI_PREFIX, instance_name);
let new_job = job::create_new_job_from_spec(
&instance,
instance,
OwnershipInfo::new(
OwnershipType::Instance,
instance_name.to_string(),
Expand Down Expand Up @@ -841,13 +848,15 @@ mod handle_instance_tests {
new_pod_names: Vec<&'static str>,
new_pod_instance_names: Vec<&'static str>,
new_pod_namespaces: Vec<&'static str>,
new_pod_error: Vec<bool>,
}

fn configure_add_shared_config_a_359973(pod_name: &'static str) -> HandleAdditionWork {
HandleAdditionWork {
new_pod_names: vec![pod_name],
new_pod_instance_names: vec!["config-a-359973"],
new_pod_namespaces: vec!["config-a-namespace"],
new_pod_error: vec![false],
}
}
fn get_config_work() -> HandleConfigWork {
Expand All @@ -857,11 +866,12 @@ mod handle_instance_tests {
find_config_result: "../test/json/config-a.json",
}
}
fn configure_add_local_config_a_b494b6() -> HandleAdditionWork {
fn configure_add_local_config_a_b494b6(error: bool) -> HandleAdditionWork {
HandleAdditionWork {
new_pod_names: vec!["config-a-b494b6-pod"],
new_pod_instance_names: vec!["config-a-b494b6"],
new_pod_namespaces: vec!["config-a-namespace"],
new_pod_error: vec![error],
}
}

Expand All @@ -873,6 +883,7 @@ mod handle_instance_tests {
work.new_pod_namespaces[i],
AKRI_INSTANCE_LABEL_NAME,
work.new_pod_instance_names[i],
work.new_pod_error[i],
);
}
}
Expand Down Expand Up @@ -944,7 +955,33 @@ mod handle_instance_tests {
find_pods_delete_start_time: false,
config_work: get_config_work(),
deletion_work: None,
addition_work: Some(configure_add_local_config_a_b494b6()),
addition_work: Some(configure_add_local_config_a_b494b6(false)),
},
);
run_handle_instance_change_test(
&mut mock,
"../test/json/local-instance.json",
&InstanceAction::Add,
)
.await;
}

#[tokio::test]
async fn test_handle_instance_change_for_add_new_local_instance_error() {
let _ = env_logger::builder().is_test(true).try_init();

let mut mock = MockKubeInterface::new();
configure_for_handle_instance_change(
&mut mock,
&HandleInstanceWork {
find_pods_selector: "akri.sh/instance=config-a-b494b6",
find_pods_result: "../test/json/empty-list.json",
find_pods_phase: None,
find_pods_start_time: None,
find_pods_delete_start_time: false,
config_work: get_config_work(),
deletion_work: None,
addition_work: Some(configure_add_local_config_a_b494b6(true)),
},
);
run_handle_instance_change_test(
Expand Down Expand Up @@ -1139,7 +1176,7 @@ mod handle_instance_tests {
find_pods_delete_start_time: false,
config_work: get_config_work(),
deletion_work: None,
addition_work: Some(configure_add_local_config_a_b494b6()),
addition_work: Some(configure_add_local_config_a_b494b6(false)),
},
);
run_handle_instance_change_test(
Expand Down
6 changes: 5 additions & 1 deletion controller/src/util/shared_test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ pub mod config_for_tests {
pod_namespace: &'static str,
label_id: &'static str,
label_value: &'static str,
error: bool,
) {
trace!("mock.expect_create_pod pod_name:{}", pod_name);
mock.expect_create_pod()
Expand All @@ -269,7 +270,10 @@ pub mod config_for_tests {
== label_value
&& namespace == pod_namespace
})
.returning(move |_, _| Ok(()));
.returning(move |_, _| match error {
false => Ok(()),
true => Err(anyhow::format_err!("create pod error")),
});
}

pub fn configure_remove_pod(
Expand Down

0 comments on commit 313e757

Please sign in to comment.