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

NAS-130088 / 25.04 / Allow taking snapshots of host paths associated with an app #15247

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

sonicaj
Copy link
Member

@sonicaj sonicaj commented Dec 20, 2024

This PR adds changes to allow taking snapshots of host paths associated with an app and also allowing them to rollback optionally.

@sonicaj sonicaj requested a review from a team December 20, 2024 10:33
@sonicaj sonicaj self-assigned this Dec 20, 2024
@bugclerk bugclerk changed the title Allow taking snapshots of host paths associated with an app NAS-130088 / 25.04 / Allow taking snapshots of host paths associated with an app Dec 20, 2024
@bugclerk
Copy link
Contributor

host_path_mapping = self.middleware.call_sync('app.get_hostpaths_datasets', app_info['name'])
# Stop the app itself before we attempt to take snapshots
self.middleware.call_sync('app.stop', app_info['name']).wait_sync()
if not snapshot_hostpath:
Copy link
Contributor

Choose a reason for hiding this comment

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

This if not snapshot_hostpath should be the first thing we do in this method, right? No reason to check all the other information if that's not provided since we return early.

mapping[host_path] = None

return mapping
return await self.middleware.call('zfs.dataset.paths_to_datasets', host_paths)
Copy link
Contributor

Choose a reason for hiding this comment

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

Your change is better, but it still uses a child process for every apps host-path. That's bad. Please see the changes I introduced in #15272. Once those are merged, you can call the paths_to_datasets_impl function directly. This does not use a child process and will be significantly faster/more efficient then what you're currently doing.

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.

3 participants