Use a status struct for RegionReplacementDetector by jmpesp · Pull Request #6564 · oxidecomputer/omicron

jmpesp · 2024-09-12T18:39:15Z

Like the other replacement related background tasks, use a struct for the return value of the RegionReplacementDetector background task.

hawkw

Looks good to me overall! I suggested some very unimportant improvements, but none of them really matter --- if you don't care that much, feel free to ignore me! :)

hawkw · 2024-09-12T18:58:18Z

                println!(
-                    "    number of region replacements started ok: {}",
-                    success.region_replacement_started_ok
+                    "    number of region replacement requests created ok: \


nit: maybe this line shouldn't begin with "number of" anymore, since it's now serving double-duty as showing the count and also as the header for the list of requests created?

Suggested change

" number of region replacement requests created ok: \

" region replacement requests created ok: \

I like this, done in 831848c

hawkw · 2024-09-12T18:58:40Z

                println!(
-                    "    number of region replacement start errors: {}",
-                    success.region_replacement_started_err
+                    "    number of region replacement start sagas started \


similarly, maybe:

Suggested change

" number of region replacement start sagas started \

" region replacement start sagas started \

also 831848c

hawkw · 2024-09-12T18:58:52Z

+                    println!("    > {line}");
+                }
+
+                println!("    number of errors: {}", status.errors.len());


and again:

Suggested change

println!(" number of errors: {}", status.errors.len());

println!(" errors: {}", status.errors.len());

also 831848c

hawkw · 2024-09-12T18:59:26Z

+                    "    number of region replacement drive sagas started ok: \
+                    {}",
                    status.drive_invoked_ok.len()
                );
                for line in &status.drive_invoked_ok {
                    println!("    > {line}");
                }

                println!(
-                    "    number of region replacement finish sagas started ok: {}",
+                    "    number of region replacement finish sagas started \
+                    ok: {}",


similarly, these lines now serve as the header for the list of entries invoked OK/errored, I wonder if we should remove the "number of"?

also 831848c

hawkw · 2024-09-12T19:03:20Z

+    number of region replacement requests created ok: 0
+    number of region replacement start sagas started ok: 0
+    number of errors: 0


take it or leave it: if you want to be super-duper extra fancy and align the number columns programmatically, you could use the approach I took in #6541, using const_max_len to calculate the longest line to align to:

omicron/dev-tools/omdb/src/bin/omdb/nexus.rs

Lines 1193 to 1225 in a3917ea

const VMMS_FOUND: &'static str = "total abandoned VMMs found:";

const VMMS_DELETED: &'static str = " VMM records deleted:";

const VMMS_ALREADY_DELETED: &'static str =

" VMMs already deleted by another Nexus:";

const SLED_RESERVATIONS_DELETED: &'static str =

"sled resource reservations deleted:";

// To align the number column, figure out the length of the

// longest line of text and add one (so that there's a space).

//

// Yes, I *could* just count the number of characters in each

// line myself, but why do something by hand when you could make

// the computer do it for you? And, this way, if we change the

// text, we won't need to figure it out again.

const WIDTH: usize = const_max_len(&[

VMMS_FOUND,

VMMS_DELETED,

VMMS_ALREADY_DELETED,

SLED_RESERVATIONS_DELETED,

]) + 1;

const NUM_WIDTH: usize = 3;

println!(" {VMMS_FOUND:<WIDTH$}{vmms_found:>NUM_WIDTH$}");

println!(

" {VMMS_DELETED:<WIDTH$}{vmms_deleted:>NUM_WIDTH$}"

);

println!(

" {VMMS_ALREADY_DELETED:<WIDTH$}{:>NUM_WIDTH$}",

vmms_already_deleted

);

println!(

" {SLED_RESERVATIONS_DELETED:<WIDTH$}{:>NUM_WIDTH$}",

sled_reservations_deleted,

);

totally up to you, though --- this is probably just me being a perfectionist.

in this case, I'm going to leave it the way it is

hawkw · 2024-09-12T19:05:08Z

-                        &log,
+                    let s = format!(
                        "find_regions_on_expunged_physical_disks failed: {e}"
                    );
-                    err += 1;
+                    error!(&log, "{s}");
+                    status.errors.push(s);


take it or leave it: it might be worth making the error be a structured field on the log entry, so that it can be easily searched with looker. maybe consider something like:

let message = "find_regions_on_expunged_physical_disks failed"; error!(&log, "{message}"; "error" => e); status.errors.push(format!("{message}: {e}");

but, it's not a huge deal.

in this case, I'm also going to leave it the way it is - I noodled a bit with this suggestion but didn't like the repetition.

cool, fine with me!

hawkw · 2024-09-12T19:05:57Z

+                        let s = format!(
                            "error looking for existing region replacement \
                             requests for {}: {e}",
                            region.id(),
                        );
+                        error!(&log, "{s}");


similarly, it might be nice to make the IDs be structured fields in the log entry and only interpolate them into the error message when formatting the string for the status? not a big deal.

(similar feedback applies to the other log messages here, of course)

see above, going to skip this suggestion

hawkw

👍

Like the other replacement related background tasks, use a struct for the return value of the RegionReplacementDetector background task.

jmpesp requested a review from hawkw September 12, 2024 18:39

hawkw reviewed Sep 12, 2024

View reviewed changes

hawkw approved these changes Sep 12, 2024

View reviewed changes

jmpesp enabled auto-merge (squash) September 12, 2024 21:24

jmpesp merged commit 887b3f0 into oxidecomputer:main Sep 12, 2024

jmpesp deleted the use_task_result_type branch September 13, 2024 01:00

jmpesp added 3 commits September 13, 2024 04:55

Use a status struct for RegionReplacementDetector

82903ed

Like the other replacement related background tasks, use a struct for the return value of the RegionReplacementDetector background task.

remove "number of" prefix

831848c

update expectorate output

a84a89f

	" number of region replacement requests created ok: \
	" region replacement requests created ok: \

	" number of region replacement start sagas started \
	" region replacement start sagas started \

	println!(" number of errors: {}", status.errors.len());
	println!(" errors: {}", status.errors.len());

	const VMMS_FOUND: &'static str = "total abandoned VMMs found:";
	const VMMS_DELETED: &'static str = " VMM records deleted:";
	const VMMS_ALREADY_DELETED: &'static str =
	" VMMs already deleted by another Nexus:";
	const SLED_RESERVATIONS_DELETED: &'static str =
	"sled resource reservations deleted:";
	// To align the number column, figure out the length of the
	// longest line of text and add one (so that there's a space).
	//
	// Yes, I could just count the number of characters in each
	// line myself, but why do something by hand when you could make
	// the computer do it for you? And, this way, if we change the
	// text, we won't need to figure it out again.
	const WIDTH: usize = const_max_len(&[
	VMMS_FOUND,
	VMMS_DELETED,
	VMMS_ALREADY_DELETED,
	SLED_RESERVATIONS_DELETED,
	]) + 1;
	const NUM_WIDTH: usize = 3;

	println!(" {VMMS_FOUND:<WIDTH$}{vmms_found:>NUM_WIDTH$}");
	println!(
	" {VMMS_DELETED:<WIDTH$}{vmms_deleted:>NUM_WIDTH$}"
	);
	println!(
	" {VMMS_ALREADY_DELETED:<WIDTH$}{:>NUM_WIDTH$}",
	vmms_already_deleted
	);
	println!(
	" {SLED_RESERVATIONS_DELETED:<WIDTH$}{:>NUM_WIDTH$}",
	sled_reservations_deleted,
	);

Uh oh!

Conversation

jmpesp commented Sep 12, 2024

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants