Skip to content

Fix leftover volumes after testing#590

Merged
saratpoluri merged 7 commits intorelease-2025.2from
fix-leftover-volumes
Nov 21, 2025
Merged

Fix leftover volumes after testing#590
saratpoluri merged 7 commits intorelease-2025.2from
fix-leftover-volumes

Conversation

@FilipAdamus97
Copy link
Copy Markdown
Contributor

@FilipAdamus97 FilipAdamus97 commented Nov 17, 2025

Make sure tests clean up volumes after themselves

📝 Description

Runtest file, which is responsible for setting up and cleaning after tests, was lacking the '-v' flag responsible for cleaning up volumes. This resulted in ALL volumes remaining after the execution. Added it at the end of the line.

After adding it, some volumes still remained after execution. I verified that all of them were referencing installed models. Volumes for models are created outside of the Docker Compose and because of this they are not taken down by it. I believe this is expected - this way we only need to install models once and not every time scenescape is restarted, but it creates issue when running tests, and the volumes pile up.

To remedy this, I added a single line at the end of the runtest file which manually removes the model volume. The naming is consistent across all tests, so there is no risk of deleting a volume unrelated to the validation.

After implementing these changes tests I run stopped leaving leftover volumes. I have noticed another test that leaves a single volume after its execution, but this seems to be an isolated case. I will look into it later.

✨ Type of Change

Select the type of change your PR introduces:

  • 🐞 Bug fix – Non-breaking change which fixes an issue
  • 🚀 New feature – Non-breaking change which adds functionality
  • 🔨 Refactor – Non-breaking change which refactors the code base
  • 💥 Breaking change – Changes that break existing functionality
  • 📚 Documentation update
  • 🔒 Security update
  • 🧪 Tests
  • 🚂 CI

🧪 Testing Scenarios

Describe how the changes were tested and how reviewers can test them too:

  • ✅ Tested manually
  • 🤖 Ran automated end-to-end tests

✅ Checklist

Before submitting the PR, ensure the following:

  • 🔍 PR title is clear and descriptive
  • 📝 For internal contributors: If applicable, include the JIRA ticket number (e.g., ITEP-123456) in the PR title. Do not include full URLs
  • 💬 I have commented my code, especially in hard-to-understand areas
  • 📄 I have made corresponding changes to the documentation
  • ✅ I have added tests that prove my fix is effective or my feature works

Make sure tests clean up volumes after themselves
@ltalarcz ltalarcz requested a review from Copilot November 17, 2025 15:14
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes volume cleanup issues in the test infrastructure by adding proper volume removal flags and commands to ensure test environments are fully cleaned up after execution.

Key Changes:

  • Added -v flag to docker compose down command to remove volumes created by Docker Compose
  • Added explicit removal of model volumes that are created outside Docker Compose context

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@saratpoluri saratpoluri enabled auto-merge (squash) November 17, 2025 23:48
Comment thread tests/runtest
docker compose ${COMPOSE_FLAGS} --project-directory ${PWD} --project-name ${PROJECT} down
docker compose ${COMPOSE_FLAGS} --project-directory ${PWD} --project-name ${PROJECT} down -v
docker volume rm -f ${PROJECT}_vol-models
rm -f ${COMPOSE_DELETE}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What if we follow this up with docker system prune --volumes? It will do a thorough job of cleaning up but it does extend the time taken for each test to complete.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was testing 'docker volume prune' and it didn't clean anything that '-v' didn't already. If I remember correctly it should get rid of the same volumes 'docker system prune --volumes' would, but I will check it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

After testing I found out that 'docker system prune --volumes' does not help with cleaning up our volumes, but it helps cleaning up cached objects. It only increased the runtime during the first execution of the command by around 10s (due to the buildup of cache). After that it was nearly instantaneous.

If we want to make sure no cached objects remain after the test run, then I think it's a good idea to add it to the script, but since it has virtually no effect on volumes I think we can drop the '--volumes' flag.

@saratpoluri saratpoluri disabled auto-merge November 18, 2025 00:22
@saratpoluri
Copy link
Copy Markdown
Contributor

@dpitulax I see that the BAT is taking longer than 45 mins to finish for this PR. Could you please check if the down -v addition causes the test completion to take longer. An 2x increase in BAT execution time will slow us down as we get closer to code freeze.

@FilipAdamus97
Copy link
Copy Markdown
Contributor Author

When I was debugging and running tests locally I saw no increase in the test runtime, but I will double check this as well.

@FilipAdamus97
Copy link
Copy Markdown
Contributor Author

I just ran BAT package on my machin with an up to date branch and the execution took less than 20 minutes. I think it would be wise to double check on someone else's machine just to be sure.

@FilipAdamus97
Copy link
Copy Markdown
Contributor Author

After investigation it turns out that BAT package taking such a long time is due to an issue with one of the metric tests, which sometimes hangs up and gets stuck. IT seems to be an issue in a number of other PRs and 'All Tests' runs, so it isn't tied to this particular change.

@saratpoluri saratpoluri enabled auto-merge (squash) November 20, 2025 00:03
@saratpoluri saratpoluri merged commit 8738ae4 into release-2025.2 Nov 21, 2025
25 checks passed
@saratpoluri saratpoluri deleted the fix-leftover-volumes branch November 21, 2025 07:22
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.

5 participants