Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking Issue: Cleanup Blueprint and Collection Diffs #7240

Open
2 of 8 tasks
andrewjstone opened this issue Dec 12, 2024 · 0 comments
Open
2 of 8 tasks

Tracking Issue: Cleanup Blueprint and Collection Diffs #7240

andrewjstone opened this issue Dec 12, 2024 · 0 comments
Assignees
Labels
cleanup Code cleanliness Debugging For when you want better data in debugging an issue (log messages, post mortem debugging, and more) development Bugs, paper cuts, feature requests, or other thoughts on making omicron development better Update System Replacing old bits with newer, cooler bits

Comments

@andrewjstone
Copy link
Contributor

andrewjstone commented Dec 12, 2024

We currently use omdb to output blueprint diffs. This is an enormously important feature and we use it all the time. However, the code used to implement these diffs is gnarly and tedious. The primary reason for this is that we also generate diffs between blueprints and inventory collections and represent the output in the same way. Inventory collections lack a number of things present in blueprints, and yet we try to diff the 2 in a way that's useful for humans.

I seem to be the primary one continuously getting stuck here when adding reconfigurator features to blueprints, which I guess is fair, because I also refactored a bunch of the original code for generating diffs to make it easier to use. It may be slightly easier, but it still sucks. What we need to do is make the diff generation between blueprints and between blueprints and collections entirely different code paths, and have them generate different output. We want as much information as we can get in these diffs and we also want to make it easy to go back and add more.

Currently, we don't actually use the diffs between blueprints and collections anywhere, and so I'm starting on this immediately because I need diff changes to finish my implementation to resolve #6999 and #7098. I don't wan to go through the same tortuous process I went through in #6968. I assure you that you will also not want to do this.

The steps I'm proposing - loosely ordered after the first are:

  • Remove all code that diffs collections and blueprints. This will substantially simplify the path, but not make it super easy to add new code.
  • Add support for diffus to blueprints
  • Add Visitor trait for traversing blueprints
  • Implement Visitor for outputting Diffs and use it
  • Implement Visitors for test usage instead of BlueprintDiff and delete legacy BlueprintDiff code
  • Re-introduce code to convert blueprints and collections into a common type with only fields common between them.
  • Create a Visitor for for the common type
  • Implement Visitor for table output and use it

This will get us to a much better place and help maintain sanity.

@andrewjstone andrewjstone added cleanup Code cleanliness Debugging For when you want better data in debugging an issue (log messages, post mortem debugging, and more) development Bugs, paper cuts, feature requests, or other thoughts on making omicron development better Update System Replacing old bits with newer, cooler bits labels Dec 12, 2024
@andrewjstone andrewjstone self-assigned this Dec 12, 2024
andrewjstone added a commit that referenced this issue Dec 18, 2024
This is the second step in #7240. The next steps will use this support
to replace our handrolled code for generating diffs and generate output
from the diffus based diffs.
andrewjstone added a commit that referenced this issue Dec 19, 2024
This PR is intended to fix #6999 and #7098.

The executor steps are all independent already except those that will be
fixed in this PR. A disk disposition was added to the blueprint and gets
changed from `InService` to `Expunged` when the `PlanningInput` changes.
A disk state was also added that goes from `Active` -> `Decommissioned`.
This disk state changes when the parent sled is expunged or the
sled-agent has been guaranteed to have seen the disk expungement as
reported via inventory.

This code fully implements the planner bits. The exucutor bits are
going to be pulled in from the [fewer-fatal-errors-in-executor-branch](main...fewer-fatal-errors-in-executor).
The executor bits are going to be expanded to implement the expunge and
decommission as dictated by the blueprint. Because the planner changes
are tightly coupled with the executor,  they will all come in this PR.

Further code will also be added to represent the disk disposition and
state in the blueprint and diff tables for omdb output. This PR is primarily
blocked on [revamping the diff system](#7240).
This is particularly necessary because we need to change those tables
and want to do it rigorously after we have a visitor based mechanism
for doing so.

There is some more cleanup to do here as well, in particular around
the `physical_disk_filter` and things that may not be needed when the
executor bits get fully implemented.
andrewjstone added a commit that referenced this issue Dec 21, 2024
This is the second step in
#7240.
The next steps will use this support to replace our handrolled code for
generating diffs and generate output from the diffus based diffs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code cleanliness Debugging For when you want better data in debugging an issue (log messages, post mortem debugging, and more) development Bugs, paper cuts, feature requests, or other thoughts on making omicron development better Update System Replacing old bits with newer, cooler bits
Projects
None yet
Development

No branches or pull requests

1 participant