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

Add management command to clean up unreferenced bucket objects #837

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

martinwe-adfinis
Copy link
Contributor

No description provided.

@martinwe-adfinis martinwe-adfinis added the enhancement New feature or request label Sep 3, 2024
@martinwe-adfinis martinwe-adfinis self-assigned this Sep 3, 2024
@martinwe-adfinis martinwe-adfinis force-pushed the feat/cleanup-objects branch 2 times, most recently from a8fd0a2 to db6b602 Compare November 13, 2024 13:28
@martinwe-adfinis martinwe-adfinis changed the title WIP: Add management command to clean up unreferenced bucket objects Add management command to clean up unreferenced bucket objects Dec 27, 2024
@martinwe-adfinis martinwe-adfinis force-pushed the feat/cleanup-objects branch 2 times, most recently from 86aaa48 to 5e1251c Compare December 27, 2024 12:55
@martinwe-adfinis
Copy link
Contributor Author

martinwe-adfinis commented Jan 21, 2025

CI failing due to the tests running with ISOLATE_UNOCONV=true (which causes document-merge-service to invoke Unoconv with unshare, which is not supported for unprivileged processes anymore since Ubuntu 24.04).

Three ways out that I see:

  • Don't run the tests with ISOLATE_UNOCONV=true. However, this means we won't get to test that case. --edit Just tried, that doesn't seem sufficient, for some reason it still invokes unshare, potentially there's another thing causing this.
  • Check if it's possible to configure the runner to somehow allow unprivileged user namespaces.
  • Revert to Ubuntu 22.04 as the runner image temporarily until we find a better solution (currently using ubuntu-latest, which happens to have changed to Ubuntu 24.04 recently).

@martinwe-adfinis martinwe-adfinis force-pushed the feat/cleanup-objects branch 2 times, most recently from d789fa7 to 5eaa109 Compare January 22, 2025 09:25
@martinwe-adfinis
Copy link
Contributor Author

  • Don't run the tests with ISOLATE_UNOCONV=true. However, this means we won't get to test that case. --edit Just tried, that doesn't seem sufficient, for some reason it still invokes unshare, potentially there's another thing causing this

Turns out there's also a docker-compose.override.yml which set the same environment vars. When removing ISOLATE_UNOCONV=true there, it works.

However, we run into other issues, likely due to compatibility issues with Python (see this job), so that would need to be addressed as well.

  • Check if it's possible to configure the runner to somehow allow unprivileged user namespaces.

I was unable to find any useful information on this until now. But given that this is about a kernel setting (whereas we are running our workloads in containers, where we have no way of touching kernel configs), I doubt this is possible.

To be fully thorough we could try to see how we can interact with the sysctl variables inside the runner, but I'd be highly surprised if we are able to somehow modify them.

  • Revert to Ubuntu 22.04 as the runner image temporarily until we find a better solution (currently using ubuntu-latest, which happens to have changed to Ubuntu 24.04 recently).

I've done this for now and it seems to work (potentially we may want to split this to a separate PR, though). I'm now running into missing test coverage errors, but that is now truly related to my changes. 😄

@martinwe-adfinis martinwe-adfinis force-pushed the feat/cleanup-objects branch 5 times, most recently from 3b6d934 to 73e7b44 Compare January 22, 2025 12:21
@martinwe-adfinis
Copy link
Contributor Author

Added the CI hack workaround via #933

This branch here is based on that other one, though, so this one is currently blocked.

@martinwe-adfinis
Copy link
Contributor Author

#933 merged, this one here is ready for merge as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants