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 some basic bind filter functions #274

Merged
merged 10 commits into from
Feb 28, 2023
Merged

Add some basic bind filter functions #274

merged 10 commits into from
Feb 28, 2023

Conversation

gabriel-samfira
Copy link
Contributor

This change adds the ability to mount a a single folder or a volume inside another folder, using the bind filter API.

While the API allows mounting multiple sources inside a single mount point, acting as an overlay, we disable this functionality in the ApplyFileBinding() function. We could add another flags parameter to this function to allow using any of the existing flags, making it more generic. Let me know if this is preferred.

Tests will be added shortly.

Signed-off-by: Gabriel Adrian Samfira gsamfira@cloudbasesolutions.com

@gabriel-samfira
Add some basic bind filter functions
726b1c3 
This change adds the ability to mount a a single folder or a volume
inside another folder, using the bind filter API.

While the API allows mounting multiple sources inside a single mount
point, acting as an overlay, we disable this functionality in the ApplyFileBinding
function.

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
@gabriel-samfira gabriel-samfira requested a review from a team as a code owner February 4, 2023 16:38
@gabriel-samfira gabriel-samfira mentioned this pull request Feb 4, 2023
Merged
@TBBle TBBle mentioned this pull request Feb 6, 2023
Open
@TBBle
Copy link
Contributor

TBBle commented Feb 6, 2023

Cross-tagging from #187 (comment): I suggest that once this lands, hcsshim be updated to use this for wclayer mount and wclayer unmount replacing the Volume Mount Point APIs that #187 was deduplicating, and hence allowing #187 to be sunset as "No current users": The lowest-maintenance-cost code is the code never committed. ^_^

@TBBle
Copy link
Contributor

TBBle commented Feb 6, 2023

I realise the current layout of this repo is... adventurous... but should this API be somewhere under pkg/ rather than in the root?

@gabriel-samfira
Add some tests
9a22fe1 
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
@gabriel-samfira
Copy link
Contributor Author

I realise the current layout of this repo is... adventurous... but should this API be somewhere under pkg/ rather than in the root?

I am extremely malleable in regard to this. Any suggestions or preferences are fine with me.

@gabriel-samfira
Copy link
Contributor Author

gabriel-samfira commented Feb 6, 2023 •
edited
Loading

I added some tests. It would be nice to add some tests with volume mount points, but I am not sure if pulling in computestorage dependencies (I think it requires additional windows features?) in order to format a VHD and make it mountable, is desirable.

@helsaawy helsaawy self-assigned this Feb 6, 2023
@TBBle
Copy link
Contributor

TBBle commented Feb 6, 2023 •
edited
Loading

Thinking more about my earlier thought regarding hcsshim, I just remembered that there's already code in hcsshim that uses Bind Filter (and a syscall wrapper for the one call they use), so converting that to use this API would be a good validation of the API, including questions about generalisation.

@gabriel-samfira
Copy link
Contributor Author

Thinking more about my earlier thought regarding hcsshim, I just remembered that there's already code in hcsshim that uses Bind Filter (and a syscall wrapper for the one call they use), so converting that to use this API would be a good validation of the API, including questions about generalisation.

Yup. It's in use in hcsshim. I think we can replace some things in hcsshim with this, but the BfGetMappings seems to only be available in ltsc2022. The tests seem to fail on 2019. Initially, this will be used in containerd to allow us to add support for buildkitd. Come to think of it, we may need to put the containerd windows snapshotter behind a feature flag or a check that the OS version is supported. We'll circle back to that later, though.

@gabriel-samfira gabriel-samfira force-pushed the add-bind-filter branch 8 times, most recently from 8f965a7 to fc72bee Compare February 6, 2023 19:49
@gabriel-samfira
Move bind filter to different package
76c22e3 
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
helsaawy
helsaawy reviewed Feb 6, 2023
View reviewed changes
pkg/bindfilter/bind_filter.go Outdated Show resolved Hide resolved
pkg/bindfilter/bind_filter.go Outdated Show resolved Hide resolved
pkg/bindfilter/bind_filter.go Show resolved Hide resolved
pkg/bindfilter/bind_filter.go Outdated Show resolved Hide resolved
@gabriel-samfira
Use string in signature and fix getFinalPath
2c3a145 
  * Properly close handle in getFinalPath()
  * Use string in function signature. mksyscall generates proper code to
    convert to utf16
  * Enable TestRemoveFileBinding on Windows Server 2019

Windows Server 2019 only exposes 2 function in bindfltapi.dll:

  * BfRemoveMapping
  * BfSetupFilter

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
@gabriel-samfira
Use windows.UTF16ToString to decode string
e1b82b1 
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
@gabriel-samfira
Copy link
Contributor Author

Come to think of it, we may need to put the containerd windows snapshotter behind a feature flag or a check that the OS version is supported. We'll circle back to that later, though.

There is no way to list mappings on ltsc2019. We can however blindly try to remove a mapping and ignore the error if it's The parameter is incorrect. That way we can support this starting with RS5.

@gabriel-samfira
Optimize bfGetMappings signature
258cf20 
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
@gabriel-samfira
Skip unsupported tests on ltsc2019
841fe89 
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
helsaawy
helsaawy approved these changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@helsaawy helsaawy left a comment

Choose a reason for hiding this comment

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

LGTM, though I would prefer a review from @msscotb before merging this

msscotb
msscotb requested changes Feb 12, 2023
View reviewed changes
pkg/bindfilter/bind_filter_test.go Outdated Show resolved Hide resolved
pkg/bindfilter/bind_filter_test.go Show resolved Hide resolved
pkg/bindfilter/bind_filter.go Outdated Show resolved Hide resolved
pkg/bindfilter/bind_filter.go Show resolved Hide resolved
@gabriel-samfira
Fix typo, add testcase
14245b8 
  * Additionally check if we can write to a read-only mount point, not
    just delete from it
  * No need to set FILE_FLAG_OPEN_REPARSE_POINT when opening a file

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
@gabriel-samfira
Remove extra flags
d05c10d 
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
@gabriel-samfira
Add test to account for symlinks as sources
33c45b1 
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
msscotb
msscotb approved these changes Feb 18, 2023
View reviewed changes
Copy link
Contributor

@msscotb msscotb left a comment

Choose a reason for hiding this comment

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

lgtm

@gabriel-samfira gabriel-samfira mentioned this pull request Feb 23, 2023
Open
@helsaawy helsaawy merged commit dd5de69 into microsoft:main Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msscotb msscotb msscotb approved these changes

@helsaawy helsaawy helsaawy approved these changes

Assignees

@helsaawy helsaawy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gabriel-samfira @TBBle @msscotb @helsaawy