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.

Sign up for GitHub

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

Don't unwrap when we can't create a dataset #992

Merged
merged 6 commits into from
Oct 13, 2023
Merged

Don't unwrap when we can't create a dataset #992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f9c7826
Don't unwrap when we can't create a region
Oct 6, 2023
beef95c
Fewer unwraps
Oct 9, 2023
56704b0
Try Tombstoned instead of Failed
Oct 11, 2023
1a56db9
Requested -> Destroyed
Oct 11, 2023
d65da02
dont fail at failing
Oct 13, 2023
0a6b3fa
Merge branch 'main' into alan/unwrapless
Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 6 additions & 17 deletions agent/src/datafile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,7 @@ impl DataFile {
let r = inner.regions.get_mut(id).unwrap();
let nstate = State::Destroyed;
match &r.state {
State::Requested => (),
State::Tombstoned => (),
x => bail!("region to destroy in weird state {:?}", x),
}
Expand Down Expand Up @@ -681,7 +682,7 @@ impl DataFile {
* - Already destroyed; no more work to do.
*/
}
State::Requested | State::Created => {
State::Requested | State::Created | State::Failed => {
/*
* Schedule the destruction of this region.
*/
Expand All @@ -696,17 +697,6 @@ impl DataFile {
self.bell.notify_all();
self.store(inner);
}
State::Failed => {
/*
* For now, this terminal state will preserve evidence for
* investigation.
*/
bail!(
"region {} failed to provision and cannot be \
destroyed",
r.id.0
);
}
}

Ok(())
Expand Down Expand Up @@ -772,7 +762,10 @@ impl DataFile {
let region = region.unwrap();

match region.state {
State::Requested | State::Destroyed | State::Tombstoned => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think Nexus is doing the right thing if it's asking for snapshots from a failed region - where did the call come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As part of the region delete, it first checks for snapshots, agent/src/server.rs:

#[endpoint {                                                                      
    method = DELETE,                                                              
    path = "/crucible/0/regions/{id}",                                            
}]                                                                                
async fn region_delete(                                                           
    rc: RequestContext<Arc<DataFile>>,                                            
    path: TypedPath<RegionPath>,                                                  
) -> SResult<HttpResponseDeleted, HttpError> {                                    
    let p = path.into_inner();                                                    
                                                                                  
    // Cannot delete a region that's backed by a ZFS dataset if there are         
    // snapshots.                                                                 
                                                                                      let snapshots = match rc.context().get_snapshots_for_region(&p.id) {          
        Ok(results) => results,                                                   
        Err(e) => {                                                               
            return Err(HttpError::for_internal_error(e.to_string()));             
        }                                                                         
    };

Without that, the delete is refused because we get an error back from the get_snapshots call.

State::Requested
| State::Destroyed
| State::Tombstoned
| State::Failed => {
// Either the region hasn't been created yet, or it has been
// destroyed or marked to be destroyed (both of which require
// that no snapshots exist). Return an empty list.
Expand All @@ -782,10 +775,6 @@ impl DataFile {
State::Created => {
// proceed to next section
}

State::Failed => {
bail!("region.state is failed!");
}
}

let mut path = self.base_path.to_path_buf();
Expand Down
83 changes: 66 additions & 17 deletions agent/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ impl ZFSDataset {

if i == 4 {
bail!(
"zfs list mountpoint failed! out:{} err:{}",
"zfs destroy dataset failed! out:{} err:{}",
out,
err
);
Expand Down Expand Up @@ -1527,7 +1527,6 @@ fn worker(
downstairs_prefix: String,
snapshot_prefix: String,
) {
// XXX unwraps here ok?
loop {
/*
* This loop fires whenever there's work to do. This work may be:
Expand All @@ -1553,7 +1552,7 @@ fn worker(
* then we finish up destroying the region.
*/
match &r.state {
State::Requested => {
State::Requested => 'requested: {
/*
* Compute the actual size required for a full region,
* then add our metadata overhead to that.
Expand All @@ -1573,15 +1572,41 @@ fn worker(
quota,
);

// if regions need to be created, do that before apply_smf.
let region_dataset = regions_dataset
// If regions need to be created, do that before
// apply_smf.
let region_dataset = match regions_dataset
.ensure_child_dataset(
&r.id.0,
Some(reservation),
Some(quota),
&log,
)
.unwrap();
) {
Ok(region_dataset) => region_dataset,
Err(e) => {
error!(
log,
"Dataset {} creation failed: {}",
&r.id.0,
e,
);
df.fail(&r.id);
break 'requested;
leftwo marked this conversation as resolved.
Show resolved Hide resolved
}
};

let dataset_path = match region_dataset.path() {
Ok(dataset_path) => dataset_path,
Err(e) => {
error!(
log,
"Failed to find path for dataset {}: {}",
&r.id.0,
e,
);
df.fail(&r.id);
break 'requested;
}
};

// It's important that a region transition to "Created"
// only after it has been created as a dataset:
Expand All @@ -1596,7 +1621,7 @@ fn worker(
&log,
&downstairs_program,
&r,
&region_dataset.path().unwrap(),
&dataset_path,
)
.and_then(|_| df.created(&r.id));

Expand All @@ -1606,13 +1631,14 @@ fn worker(
"region {:?} create failed: {:?}", r.id.0, e
);
df.fail(&r.id);
break 'requested;
}

info!(log, "applying SMF actions post create...");
let result = apply_smf(
&log,
&df,
regions_dataset.path().unwrap(),
dataset_path,
&downstairs_prefix,
&snapshot_prefix,
);
Expand All @@ -1624,12 +1650,25 @@ fn worker(
}
}

State::Tombstoned => {
State::Tombstoned => 'tombstoned: {
let dataset_path = match regions_dataset.path() {
Ok(dataset_path) => dataset_path,
Err(e) => {
error!(
log,
"Cannot get path on tombstoned dataset {}: {}",
&r.id.0,
e,
);
df.fail(&r.id);
break 'tombstoned;
}
};
info!(log, "applying SMF actions before removal...");
let result = apply_smf(
&log,
&df,
regions_dataset.path().unwrap(),
dataset_path,
&downstairs_prefix,
&snapshot_prefix,
);
Expand All @@ -1640,12 +1679,22 @@ fn worker(
info!(log, "SMF ok!");
}

// After SMF successfully shuts off downstairs, remove zfs
// dataset.
let region_dataset = regions_dataset
.from_child_dataset(&r.id.0)
.unwrap();

// After SMF successfully shuts off downstairs, remove
// zfs dataset.
let region_dataset =
match regions_dataset.from_child_dataset(&r.id.0) {
Ok(region_dataset) => region_dataset,
Err(e) => {
error!(
log,
"Cannot find region {:?} to remove: {}",
r.id.0,
e,
);
let _ = df.destroyed(&r.id);
break 'tombstoned;
}
};
let res =
worker_region_destroy(&log, &r, region_dataset)
.and_then(|_| df.destroyed(&r.id));
Expand Down