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

[3006.x][s3fs] delete files from local cache when deleted from s3 #65614

Merged
merged 1 commit into from Feb 29, 2024

Conversation

dead10ck
Copy link
Contributor

@dead10ck dead10ck commented Nov 26, 2023

What does this PR do?

Currently, s3 files are not deleted from the local cache when they are deleted from s3. See #65611

This is currently a draft, as it's based on #65610. The two are not directly related, but the latter adds some testing boilerplate for the file server, which this builds on.

What issues does this PR fix or reference?

Fixes: #65611

Merge requirements satisfied?

@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title [s3fs] delete files from local cache when deleted from s3 [master][s3fs] delete files from local cache when deleted from s3 Nov 26, 2023
@dead10ck dead10ck changed the base branch from master to 3006.x December 4, 2023 23:34
@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title [master][s3fs] delete files from local cache when deleted from s3 [3006.x][s3fs] delete files from local cache when deleted from s3 Dec 4, 2023
@dead10ck dead10ck force-pushed the fix-s3-delete branch 2 times, most recently from ee03580 to 96794a5 Compare December 7, 2023 03:33
@dwoz dwoz added this to the Sulfur v3006.6 milestone Dec 11, 2023
twangboy
twangboy previously approved these changes Dec 20, 2023
@twangboy
Copy link
Contributor

This needs to be rebased (not merged) and conflicts resolved.

@dwoz dwoz modified the milestones: Sulfur v3006.6, Sulfur v3006.7 Feb 14, 2024
@dead10ck dead10ck marked this pull request as ready for review February 17, 2024 15:53
@dead10ck dead10ck requested a review from a team as a code owner February 17, 2024 15:53
@dead10ck dead10ck requested review from dwoz and removed request for a team February 17, 2024 15:53
@dead10ck
Copy link
Contributor Author

Ok this was rebased and has a change log entry now.

@dwoz dwoz requested a review from whytewolf February 21, 2024 02:36
@dwoz
Copy link
Contributor

dwoz commented Feb 21, 2024

Asking for @whytewolf's input on this one.

@whytewolf
Copy link
Contributor

my main concern would be people using large filesystems in s3 with this. lots of for loops isn't exactly speedy when crawling large filesystems. but hard to see a way around them without redesigning the whole thing.

no one should be relying on the cache item still being there. if they are that is undefined behavior and was never promised. so not sure we need that warning.

@dead10ck
Copy link
Contributor Author

my main concern would be people using large filesystems in s3 with this. lots of for loops isn't exactly speedy when crawling large filesystems. but hard to see a way around them without redesigning the whole thing.

Yeah, though if you have a large fs, then simply doing the s3 object listing would become a bottleneck well before the local file system would. S3 isn't really meant to serve as a large scale NFS, so s3 wouldn't be a good choice of file server for such a use case anyway.

no one should be relying on the cache item still being there. if they are that is undefined behavior and was never promised. so not sure we need that warning.

Yeah, but my suspicion is that people who are relying on it are doing so unknowingly. This came about because I was still seeing states getting applied that I had deleted because I forgot to remove its invocation from top.sls. It's easy to imagine this change causing breakage for systems where s3fs has been in use for a long period of time.

@whytewolf
Copy link
Contributor

my main concern would be people using large filesystems in s3 with this. lots of for loops isn't exactly speedy when crawling large filesystems. but hard to see a way around them without redesigning the whole thing.

Yeah, though if you have a large fs, then simply doing the s3 object listing would become a bottleneck well before the local file system would. S3 isn't really meant to serve as a large scale NFS, so s3 wouldn't be a good choice of file server for such a use case anyway.

no one should be relying on the cache item still being there. if they are that is undefined behavior and was never promised. so not sure we need that warning.

Yeah, but my suspicion is that people who are relying on it are doing so unknowingly. This came about because I was still seeing states getting applied that I had deleted because I forgot to remove its invocation from top.sls. It's easy to imagine this change causing breakage for systems where s3fs has been in use for a long period of time.

both of those are good reasons. and why i accepted in the review. @dwoz this is good to go.

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.

[BUG] [s3fs] files are not deleted
4 participants