Alan/cmon polish - #1855
Open
leftwo wants to merge 71 commits into
Open
Conversation
Add a PID to upstairs_raw Make default cmon more like get_up_state.sh script, which is what I wanted this to be to begin with. Add more of the dtrace json blob to cmon
Add the missing test directory Include the dtrace default script in the binary. Help polish.
Found a bunch of places where we just panic and don't include session or client id. Started from here: #1924 But, then just found a bunch more. Co-authored-by: Alan Hanson <alan@oxide.computer>
`Volume::construct` was unintentionally adding another layer of Volume when constructing from a request. Remove this, ensuring that what is constructed matches the request.
Add a new VolumeInfo enum that will be returned and provide a richer tree of information that matches the shape of the Volume. The intended consumer of this is the control plane in a few areas: - when performing region replacement or region snapshot replacement, the control plane needs to know when to consider the live repair or reconciliation successful, ultimately proceeding to cleaning up the temporary resources and continuing with another replacement. The upstairs has always been the source of this answer but the plan is to move from using activation as this signal to using this introduced enum - when activating with only 2 out of 3 downstairs available, the control plane needs to know the difference between an unhealthy volume and one that activated early with 2 out of 3 but eventually had all 3 mirrors online. - in the future, the control plane could query for health when performing updates or sled reboots, pausing until impacted Volumes become healthy again The VolumeInfo enum already existed so this PR renames that to VolumeExtentInfo. Eventually we should probably combine the two.
Drift 0.1.4 includes oxidecomputer/drift#20 and oxidecomputer/drift#22 -- the latter is particularly important because it can lead to false negatives where actually-incompatible APIs are marked as compatible.
Now that Omicron uses reqwest 0.13, we can update omicron/oximeter, and remove the explicit dependency on reqwest 0.12.
This is required for the simulated Pantry embedded in omicron's simulated sled-agent.
Chasing down a cargo build failure in propolis caused by multiple versions of the same package lead to the discovery that rev of omicron that is in propolis' lock file is old, about 700 commits behind main. Once propolis was updated to the same rev of omicron that crucible uses, cargo no longer hit that same build error. Use explicit git revs in Cargo.toml: this doesn't change the version in the lock file but does make it explicit what is being used. Propolis will be updated shortly to pull this commit in (a no-op, as the lockfile version doesn't change), and will be updated to use explicit revs in the same way.
I'll continue work in other tests, but wanted to start with making crutest main use slog instead of println. It can be difficult to parse messages when println and slog are interleaved. I started to do this work in the 2/3 PR that Matt started, but better to split this out and not add extra noise to that PR.
Added the volume-info command to crutest cli. This will get the VolumeInfo, make it json, and send it over to the CLI.
Update Crucible's Omicron to oxidecomputer/omicron@5fd53a9. Also run `cargo update rand@0.8.5 daft` to resolve dependency conflicts.
This provides a pantry test program and a few basic commands to talk to a running pantry server. It allows some exercise of the pantry outside of omicron. Initial support for attach, detach, status, and volume-status. The pantest program uses dsc to get information and from that constructs a VCR to use with the pantry. This also adds a "readonly" endpoint to dsc. This allows us to get everything we need from the dsc server to create a volume and attach it to the pantry.
…1948) When a read-only upstairs is deactivating and a downstairs reconnects and negotiates to WaitQuorum, on_reconciliation_skipped() previously hit a catch-all arm that logged a warning but left the client stuck in WaitQuorum. Since ready_to_deactivate() only returns true for WaitConnect, deactivation could never complete. Deactivate any client in WaitQuorum when the upstairs is in the Deactivating state. This moves them through Stopping(Deactivated) and back to WaitConnect, allowing deactivation to finish. Fix for #1947
Holding the `entries` lock across an activation or deactivation will lock out _all_ other Pantry operations. If a activation or deactivation can not complete, this makes the Pantry un-usable. Drop the mutex guards before long-running operations. Also: in the Pantry implementation, re-implement `attach` in terms of `attach_activate_background`, which already does the activation in the background and drops the `entries` lock. However callers of `attach` expect the Volume to be activated when the call returns so wait for the background job. Co-authored-by: Alan <alan@oxide.computer> Fixes #1945.
When detaching a Volume from the Pantry, garbage-collect all the related jobs. A previous commit changed the implementation of `attach` to be implemented in terms of `attach_activate_background`, and this used the volume_id as the supplied job_id, This caused a problem because the re-use of a job_id isn't allowed, meaning that clients of the Pantry could only attach a volume once!
Has a neat new UI for reporting incompatibilities (see oxidecomputer/dropshot-api-manager#105).
Found this while debugging a nightly test failure. This is not related to the failure. Co-authored-by: Alan Hanson <alan@oxide.computer>
Ack flushes on read-only volumes without sending to downstairs A flush on a read-only volume has no data to persist, so ack it immediately in the guest Flush path instead of sending it to the downstairs. This broke a test, test_no_read_only_live_repair, which relied on a guest flush reaching the downstairs to retire skipped jobs after a fault. Retiring completed jobs still must happen, but in the production code the auto-flush will come around and clear these jobs out. The internal flush timer for the test framework is pinned to 24 hours, so to enable the internal flush we add a test-only BlockOp::FlushCheck and guest.flush_check() that runs the same work the automatic flush timer performs. Unlike a guest flush, this path still sends the flush to the downstairs on a read-only volume. Updated the test to use this new path. This appears to be a simpler solution and does not require the heavier modification that enabling the auto flush on all tests would involve. Enabling auto-flush for all tests would add non-determinism to many tests that don't expect it.
…me will be. (#1957) Print more helpful info if this condition arises. The extent number if printed as uppercase hex with 3 digits is how the extent file will be named, so print this out as well to help someone doing debug. --------- Co-authored-by: Alan Hanson <alan@oxide.computer>
Added some documentation around what the nightly class tests expect for incoming environmental variables and how the tests should use them. These are things we already did, just writing them down somewhere. Other work coming later in this area. Added a bit more info about the system we are running on for the nightly test.
Don't print the confusing message client 0 is in state Active not ready for activation Co-authored-by: Alan Hanson <alan@oxide.computer>
Added an option to crucible-verify-raw that will produce csv output instead of a formatted print. This will allow easier post processing of the output and ease comparing output from different extent files. Example output: ``` BRM42220006 # /tmp/crucible-verify-raw --verbose-csv AEF | head # file:AEF bs:512 bytes bc:131072 dirty:true gen:4 flush_number:37009787 ext_ver:2 bonus_sync:21543 defrag:939 file,block,dirty,status,slot_a_result,slot_a_flush_id,slot_a_hash,slot_b_result,slot_b_flush_id,slot_b_hash,selected_slot,data_hash,all_zeros AEF,0,true,ok,HashMatch,47428,18362918644890996774,SlotHashMismatch,45302,16039251774303925735,true,18362918644890996774,false AEF,1,true,ok,HashMatch,47428,13247521920287360720,SlotHashMismatch,45302,13200850826928211362,true,13247521920287360720,false AEF,2,true,ok,HashMatch,47428,10545746106689772998,SlotHashMismatch,45302,12954128592411415433,true,10545746106689772998,false AEF,3,true,ok,HashMatch,47428,6884446101332089095,SlotHashMismatch,45302,11361899870660325085,true,6884446101332089095,false AEF,4,true,ok,HashMatch,47428,16980956734254118825,SlotHashMismatch,45302,2419870224295018126,true,16980956734254118825,false AEF,5,true,ok,HashMatch,47428,16285263515216636595,SlotHashMismatch,45302,17408677481660435227,true,16285263515216636595,false AEF,6,true,ok,HashMatch,47428,11101626895967727594,SlotHashMismatch,45302,9823985220066489586,true,11101626895967727594,false AEF,7,true,ok,HashMatch,47428,38394401569940404,SlotHashMismatch,45302,1861286921230100119,true,38394401569940404,false ``` I'm building some other tools to compare output from crucible-verify-raw, but moving to csv first will make that whole process easier. --------- Co-authored-by: Alan Hanson <alan@oxide.computer>
Move the shared display formatting into cmon-common. cmon and ctop each carried a near identical copy of format_header() and format_row() which had already drifted apart, and both had the same LRC0/LRC1/LRC0 typo in the LiveRepair header. A test now walks every DtraceDisplay variant and checks the header and the row agree on that field's width. Fix cmon's job delta. It kept one last job id for every session, but the raw dtrace script matches every upstairs on the system, so rows from different sessions interleave and the subtraction underflowed: an abort in a debug build, a wrapped value in a release one. Job ids are now tracked per session. cmon also spun printing parse errors once stdin reached EOF, and now stops. Report why ctop's dtrace command stopped. Its stderr went to /dev/null and the exit status was ignored, so the common failure of not having enough privilege looked exactly like an idle system: an empty table and no explanation. Rebuild the ctop table on a ratatui Table. It clips rows to the terminal instead of letting them wrap and push the layout apart, scrolls itself to keep the selection visible, and follows resizes, replacing the hand rolled cursor, clear and scroll offset handling. The terminal is also restored now when the display loop returns an error, not only when it exits normally. Pace the ctop redraws. Input is answered at once, arriving records get at most one repaint every 250ms, and an idle screen still refreshes once a second. With 40 sessions that cut CPU use about four fold. Input is drained each pass, so a held arrow key stops when it is released. The selection now names a session rather than a row number, so sessions coming and going above the cursor no longer move it onto a different session. Rename the ERR and ERN columns to RECD and RECN. They count extents reconciled and extents needing reconciliation, and there is a separate error column, so the old names read as something they were not. Describe the pid wrapper that upstairs_raw.d emits, in the script and in the dtrace README, and add ctop to make-nightly.sh and make-utils.sh so it reaches a sled.
The two bottom titles competed for the same border, so the key help was cut off part way through. Move the keys onto their own line below the graph, where the table view already puts them, which leaves the whole bottom border to the statistics. That line now names Esc as well. Normalizing scales the axis to every session while the statistics still described only this one, so the two read as though they disagreed. Report the range the axis is actually scaled to alongside them. The y axis labels were printed on top of the oldest samples. Extend the x range to the left of the first sample by the width of the labels so they have a strip to themselves, and drop duplicate labels, which a session sitting at one value produced five of.
The two test modules carried about 230 lines of prose describing coverage that does not exist. Move what is worth doing into ctop/TODO.md and let the tests that are there speak for themselves. Drop one cmon-common test that only checked the compiler implements Copy, and correct another whose comment claimed a bound it did not assert. render_sparkline's doc offered to scale "if global_max is provided", which is not optional, and described the sparkline as showing values over time. It plots one column per recorded sample, so a session that stops reporting holds its shape rather than leaving a gap. Say so, and say the same of the job delta, which is a rate per second only for as long as records keep arriving.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reviewer warning.
I started this PR 100 years ago, but over time I've been using claude to
iterate on it, and now it's almost all claude generated. I've gone over
what it has done, and it seems fine for this level of tool. However, I'm
more interested in the result it produces here than the actual code
itself as this is a tool that lives outside the product and none of the code
here is ever run in production. I've been using it and find it useful.
Summary
This PR adds
ctop, an interactive curses monitor for Crucible upstairsprocesses, and factors the display code it shares with
cmoninto a newcmon-commonlibrary.cmonprints DTrace output a line at a time;ctopgives you atop-like view of every upstairs on the system, withhistory and a detail graph. It runs a DTrace command internally and then
processes the output, it does not require piping output like cmon.
ctop
One row per session, sorted by pid then session id.
DELTAis how farNEXTJOBmoved since that session's previous record, so it is a rate ofjobs per second while records keep arriving; the sparkline on the right
is the last hundred of those, scaled against the busiest session on the
screen so rows can be compared against each other.
>marks the selected row and*a session that has not reported forfive seconds. A session that goes quiet for thirty seconds is dropped.
Detail view
dopens the selected session's delta history as a graph:ntoggles normalization: the graph is scaled to every session on thesystem rather than just this one, which is how you compare a quiet
session against a busy one.
MinandMaxalways describe theselected session, and
Scaleis what the axis is actually scaled to, sothe two do not have to agree.
Keys
↑/↓PgUp/PgDndEscnq,Ctrl-CRunning it
ctop needs DTrace privileges, so you may need to elevate
Without them DTrace exits immediately, and ctop complains.
You can use
--dtrace-cmdto replace the built in DTrace command.ctop is added to
tools/make-nightly.shand totools/make-utils.sh, soit lands in the nightly archive and in the sled global zone alongside
crucible-verify-raw.cmon-common
format_header(),format_row(), theDtraceDisplayfield list, thedefault field set and
short_state()now live in one place, which bothtools call. New tests make sure cmon and ctop don't drift apart by
walking every
DtraceDisplayvariant and checking the header and therow agree on the width of that field.
cmon
cmon dtracenow expects the wrapped JSON thatupstairs_raw.demits(see below), and gained
pid,sessionandupstairs_idfields alongwith single column forms of several existing ones.
upstairs_raw.d
The script now wraps each record with the pid of the upstairs that
produced it:
This allows us to tell records from different processes apart once they
interleave.
tools/dtrace/README.mddocuments the format with a workedexample.
Testing
cargo nextest run -p ctop -p cmon-common, plus manual runs againstcaptured and synthetic DTrace output at a range of terminal sizes,
including the failure paths where the DTrace command cannot start.
I've also been using this for months to watch crucible on dogfood and
on the racklettes to see what is going on.