Skip to content

[sled-agent-config-reconciler] Add separate OmicronDatasets type #8218

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

Open
wants to merge 1 commit into
base: john/sled-agent-config-reconciler-2
Choose a base branch
from

Conversation

jgallagher
Copy link
Contributor

This is more consistent with how the reconciler remembers zones and disks. This is almost all just moving code around. The only nontrivial changes are:

  1. In the spot where we ought to delete datasets, we at least "forget" them in-memory and log an error about leaking the ZFS dataset
  2. The dataset serialization task remembers fewer details about datasets it ensured (just the names - that's all we need for the error checking it does when dealing with nested datasets)

Staged on top of #8064. PR 1 of 3 working towards #8173.

This is more consistent with how the reconciler remembers zones and
disks. This is almost all just moving code around. The only nontrivial
changes are:

1. In the spot where we ought to delete datasets, we at least "forget"
   them in-memory and log an error about leaking the ZFS dataset
2. The dataset serialization task remembers fewer details about datasets
   it ensured (just the names - that's all we need for the error
   checking it does when dealing with nested datasets)
};
self.datasets
.iter()
// Fitler down to successfully-ensured datasets
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Fitler down to successfully-ensured datasets
// Filter down to successfully-ensured datasets

Comment on lines +7 to +8
//! There is no separate tokio task here; our parent reconciler task owns this
//! set of datasets and is able to mutate it in place during reconciliation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to make this comment "There is no separate tokio task here" make sense, given that there is a DatasetTaskHandle field in the OmicronDatasets field below.

Seems like there is both a task-independent dataset structure, as well as a task?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm yeah this is confusing. My intent was "this datastructure isn't one that spawns a task and uses a channel to communicate to it", which is true, but it does need to use the dataset serialization task handle that's spawned elsewhere (and is also used to service support bundle nested dataset operations and inventory collections).

Maybe DatasetSerializationTask should be ZfsSerializationTask - would that be clearer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants