[PATCH proxmox] resource-scheduling: topsis: add context to 'unwraps'

[PATCH proxmox] resource-scheduling: topsis: add context to 'unwraps'

[Dominik Rusovac]

Adds context as to why particular 'unwraps' panic.

Signed-off-by: Dominik Rusovac <d.rusovac@proxmox.com>
---
 proxmox-resource-scheduling/src/topsis.rs | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/proxmox-resource-scheduling/src/topsis.rs b/proxmox-resource-scheduling/src/topsis.rs
index 6d078aa6..25934f5b 100644
--- a/proxmox-resource-scheduling/src/topsis.rs
+++ b/proxmox-resource-scheduling/src/topsis.rs
@@ -145,8 +145,10 @@ impl<const N: usize> IdealAlternatives<N> {
             let min = fixed_criterion
                 .clone()
                 .min_by(|a, b| a.total_cmp(b))
-                .unwrap();
-            let max = fixed_criterion.max_by(|a, b| a.total_cmp(b)).unwrap();
+                .expect("zero alternatives");
+            let max = fixed_criterion
+                .max_by(|a, b| a.total_cmp(b))
+                .expect("zero alternatives");
 
             (best[n], worst[n]) = match criteria[n].maximize {
                 true => (max, min),
@@ -234,8 +236,7 @@ macro_rules! criteria_struct {
                     $(
                         $crate::topsis::Criterion::new($crit_name.to_string(), $crit_weight),
                     )*
-                ])
-                .unwrap()
+                ]).unwrap_or_else(|err| panic!("constructing criteria failed: {err}"))
             });
 
         impl From<$name> for [f64; $count_name] {
-- 
2.47.3

Re: [PATCH proxmox] resource-scheduling: topsis: add context to 'unwraps'

[Daniel Kral]

Thanks for the patch!

On Wed Feb 25, 2026 at 2:06 PM CET, Dominik Rusovac wrote:
> Adds context as to why particular 'unwraps' panic.
>
> Signed-off-by: Dominik Rusovac <d.rusovac@proxmox.com>
> ---
>  proxmox-resource-scheduling/src/topsis.rs | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/proxmox-resource-scheduling/src/topsis.rs b/proxmox-resource-scheduling/src/topsis.rs
> index 6d078aa6..25934f5b 100644
> --- a/proxmox-resource-scheduling/src/topsis.rs
> +++ b/proxmox-resource-scheduling/src/topsis.rs
> @@ -145,8 +145,10 @@ impl<const N: usize> IdealAlternatives<N> {
>              let min = fixed_criterion
>                  .clone()
>                  .min_by(|a, b| a.total_cmp(b))
> -                .unwrap();
> -            let max = fixed_criterion.max_by(|a, b| a.total_cmp(b)).unwrap();
> +                .expect("zero alternatives");
> +            let max = fixed_criterion
> +                .max_by(|a, b| a.total_cmp(b))
> +                .expect("zero alternatives");

Hm, it might make sense to fall back to sensible defaults here or return
a Result<> (as this is only used internally in score_alternatives(...))
and return either an empty vec there or an error as well.

>  
>              (best[n], worst[n]) = match criteria[n].maximize {
>                  true => (max, min),
> @@ -234,8 +236,7 @@ macro_rules! criteria_struct {
>                      $(
>                          $crate::topsis::Criterion::new($crit_name.to_string(), $crit_weight),
>                      )*
> -                ])
> -                .unwrap()
> +                ]).unwrap_or_else(|err| panic!("constructing criteria failed: {err}"))
>              });
>  
>          impl From<$name> for [f64; $count_name] {

Re: [PATCH proxmox] resource-scheduling: topsis: add context to 'unwraps'

[Dominik Rusovac]

I agree, unwrapping the option is indeed questionable. Yet these unwraps
seemed good enough and I didn't want to touch the error handling itself,
as it would entail more changes. Atm, we have no designated Error types
to map to and imo just bail!-ing on a Result in this place adds no
reasonable value. Consistently propagating errors from start to end
would be more appropriate, I suppose.

For now, I just added some context as to what must have gone wrong in
this very situation, because it changes just a tiny bit of code, while
adding reasonable information. 

I could send a v2 that touches error handling in general, though.

Re: [PATCH proxmox] resource-scheduling: topsis: add context to 'unwraps'

[Daniel Kral]

On Thu Feb 26, 2026 at 10:28 AM CET, Dominik Rusovac wrote:
> I agree, unwrapping the option is indeed questionable. Yet these unwraps
> seemed good enough and I didn't want to touch the error handling itself,
> as it would entail more changes. Atm, we have no designated Error types
> to map to and imo just bail!-ing on a Result in this place adds no
> reasonable value. Consistently propagating errors from start to end
> would be more appropriate, I suppose.
>
> For now, I just added some context as to what must have gone wrong in
> this very situation, because it changes just a tiny bit of code, while
> adding reasonable information. 
>
> I could send a v2 that touches error handling in general, though.

Currently, the only user of score_alternatives(...) is
score_nodes_to_start_service(...), where it's enough that nodes is an
empty slice to cause a panic. This isn't a problem now because the
function is unlikely to be called as there's always at least one node
available.

If we're going to use these for the load balancer, it could be way
easier to run into this, e.g. there is only one service restricted to a
single node, i.e., no migration candidates. Of course we could catch
this early, but returning a generic Error here and then handling that in
score_alternatives(...) to return an empty vec would be an improvement.

Having error types would be plus here of course, but needs some
consideration that it doesn't break building packages that use this
crate.