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 1 commit
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
22 changes: 5 additions & 17 deletions agent/src/datafile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,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 @@ -697,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 @@ -773,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 @@ -783,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
14 changes: 3 additions & 11 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 @@ -1589,15 +1589,7 @@ fn worker(
&r.id.0,
e,
);
if let Err(e) = df.destroyed(&r.id) {
error!(
log,
"Dataset {} destruction failed: {}",
&r.id.0,
e,
);
df.fail(&r.id);
}
df.fail(&r.id);
break 'requested;
}
};
Expand Down Expand Up @@ -1699,7 +1691,7 @@ fn worker(
r.id.0,
e,
);
df.fail(&r.id);
let _ = df.destroyed(&r.id);
break 'tombstoned;
}
};
Expand Down
Loading