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

Jump to bottom

Add origin field to StratFilesystem #3516

Merged
merged 8 commits into from
Feb 26, 2024
Merged

Add origin field to StratFilesystem #3516

merged 8 commits into from
Feb 26, 2024

Conversation

mulkieran
Copy link
Member

@mulkieran mulkieran commented Dec 12, 2023 •
edited
Loading

Closes #3515

@mulkieran mulkieran self-assigned this Dec 12, 2023
@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 47c514e. @martinpitt, @jelly, @mvollmer please check.

@mulkieran mulkieran force-pushed the issue_stratisd_3515 branch 2 times, most recently from a5649a9 to 2ade7f5 Compare December 12, 2023 19:02
@mulkieran mulkieran requested a review from bmr-cymru December 12, 2023 19:25
@mulkieran mulkieran marked this pull request as ready for review December 12, 2023 22:10
@martinpitt martinpitt mentioned this pull request Dec 13, 2023
Closed
@mulkieran
Copy link
Member Author

We can not add an origin field without including the abllity to update the origin field in the case that a filesystem is removed and another filesystem has an origin field which points to that filesystem. So we must determine, before this PR is merged, what the rule is supposed to be. There are two possibilities:

  • set the origin field to None
  • redirect it to point to the same filesystem as the removed filesystem's origin pointed to, if that exists

@mulkieran mulkieran force-pushed the issue_stratisd_3515 branch 2 times, most recently from fd9f226 to 7cf9692 Compare December 21, 2023 04:21
@mulkieran
Copy link
Member Author

We're best off setting the origin field to None. TODO: handle D-Bus signals.

@mulkieran mulkieran force-pushed the issue_stratisd_3515 branch 2 times, most recently from 0524e0e to 3973bfe Compare December 21, 2023 04:41
@mulkieran
Copy link
Member Author

We may want to make the D-Bus property settable, for debugging purposes.

@mulkieran mulkieran force-pushed the issue_stratisd_3515 branch 5 times, most recently from b5e6e35 to c0c2ee8 Compare December 22, 2023 02:18
@mulkieran mulkieran force-pushed the issue_stratisd_3515 branch from c0c2ee8 to 22b73b3 Compare January 2, 2024 02:38
@mulkieran mulkieran force-pushed the issue_stratisd_3515 branch from 3adcd7f to 00c8aba Compare February 5, 2024 13:47
@mulkieran
Copy link
Member Author

rebased

@mulkieran mulkieran force-pushed the issue_stratisd_3515 branch from 00c8aba to 719a4dc Compare February 5, 2024 13:52
@mulkieran
Copy link
Member Author

/packit test

@mulkieran mulkieran requested a review from bmr-cymru February 5, 2024 14:36
bmr-cymru
bmr-cymru approved these changes Feb 6, 2024
View reviewed changes
Copy link
Member

@bmr-cymru bmr-cymru left a comment

Choose a reason for hiding this comment

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

I've tested this with the changes from stratis-cli#1045 and I'm seeing all the behaviour I expect: for regular file systems the origin field is unset and for snapshots I get the UUID of the origin volume.

The only thing I was unsure of reading the patches was whether the value printed for the report should be "None" rather than "Not set" to be consistent with the CLI output for file systems with no origin, but looking at the rest of the report code this is consistent with what we report when size_limit is not set so I think this is fine.

@mulkieran mulkieran force-pushed the issue_stratisd_3515 branch from 719a4dc to b202b11 Compare February 7, 2024 01:10
@mulkieran mulkieran requested a review from jbaublitz February 7, 2024 01:10
@mulkieran mulkieran force-pushed the issue_stratisd_3515 branch from b202b11 to 0202870 Compare February 7, 2024 01:11
jbaublitz
jbaublitz requested changes Feb 20, 2024
View reviewed changes
Copy link
Member

@jbaublitz jbaublitz left a comment

Choose a reason for hiding this comment

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

Just a few code cleanliness comments, but overall I think this looks good.

src/engine/strat_engine/thinpool/thinpool.rs Outdated
Comment on lines 1588 to 1590
let (name, fs) = self
.get_filesystem_by_uuid(fs_uuid)
.expect("Looked up above.");
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any reason for this not to be inside the if changed ... block. That would avoid looking it up if nothing has changed.

src/engine/sim_engine/pool.rs Outdated
Comment on lines 487 to 503
let snapshots: Vec<FilesystemUuid> = self
.filesystems()
.iter()
.filter_map(|(_, u, fs)| {
fs.origin()
.and_then(|x| if removed.contains(&x) { Some(*u) } else { None })
})
.collect();

let mut updated_origins = vec![];
for sn_uuid in snapshots {
if let Some((_, fs)) = self.filesystems.get_mut_by_uuid(sn_uuid) {
if fs.unset_origin() {
updated_origins.push(sn_uuid);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this could all be collapsed into a single iterator using filesystems_mut() for the iterator and fold() at the end to collect the updated origins.

src/engine/types/actions.rs Outdated
}
}
}

impl Display for SetDeleteAction<FilesystemUuid> {
impl<U> Display for SetDeleteAction<FilesystemUuid, U> {
Copy link
Member

Choose a reason for hiding this comment

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

I think erring on the side of specificity is good for these impls so using FilesystemUuid might be better. Otherwise, we have a frequent problem of overlapping impls. That would also allow us to add to the log message indicating what UUIDs had their origin updated.

@mulkieran mulkieran requested a review from jbaublitz February 21, 2024 18:29
@mulkieran
Copy link
Member Author

@jbaublitz I think it is ready now.

@mulkieran mulkieran mentioned this pull request Feb 21, 2024
Merged
@mulkieran
Copy link
Member Author

rebased

@mulkieran
Copy link
Member Author

squashed...

@mulkieran mulkieran merged commit fb888f7 into stratis-storage:master Feb 26, 2024
51 checks passed
@mulkieran mulkieran deleted the issue_stratisd_3515 branch February 26, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbaublitz jbaublitz jbaublitz approved these changes

@bmr-cymru bmr-cymru bmr-cymru approved these changes

Assignees

@mulkieran mulkieran

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support origin field for filesystem snapshots
4 participants
@mulkieran @martinpitt @bmr-cymru @jbaublitz