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

Manage storage size on orthanc-raw #176

Closed
wants to merge 4 commits into from

Conversation

milanmlft
Copy link
Member

@milanmlft milanmlft commented Dec 6, 2023

Set the MaximumStorageSize variable in the configuation for orthanc-raw to recycle studies when running out of storage space.

I tested this locally by uploading some random Dicom images I found online and then inspecting the orthanc-raw logs, so this seems to work?

Fixes UCLH-Foundry/the-rolling-skeleton#69, also closes #130 (I think?)

To do

  • Document this config parameter
  • Add proper tests

@milanmlft
Copy link
Member Author

Putting as draft for now as this needs more proper testing, but would be keen to get some feedback already!

Copy link
Contributor

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

Seems like it should be glorious 🤞, will wait until we hear back about testing it fully for the approval

@@ -37,6 +37,7 @@ ORTHANC_RAW_USERNAME=orthanc_raw_username
ORTHANC_RAW_PASSWORD=orthanc_raw_password
ORTHANC_RAW_AE_TITLE=ORTHANCRAW
ORTHANC_AUTOROUTE_RAW_TO_ANON=true
ORTHANC_RAW_MAXIMUM_STORAGE_SIZE=1
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this to be limited during testing?

Copy link
Member Author

@milanmlft milanmlft Dec 7, 2023

Choose a reason for hiding this comment

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

I think having a low value makes testing easier as we won't need a huge amount of images to see if it's actually working. Note that this is just the sample file though, the actual .env.test is ignored so can easily be adapted locally. The 1 (MB) is just a default value for now.

Copy link
Contributor

@stefpiatek stefpiatek Dec 7, 2023

Choose a reason for hiding this comment

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

Ah yeah I guess if this is used in end to end tests then that might be an issue was my thought, but maybe it's not used in e2e tests in CI?

scripts/delete_oldest_n_studies.py Outdated Show resolved Hide resolved
Comment on lines +13 to +16
// Limit the maximum storage size
"MaximumPatientCount" : 0, // no limit
"MaximumStorageSize" : ${ORTHANC_RAW_MAXIMUM_STORAGE_SIZE}, // MB
"MaximumStorageMode" : "Recycle",
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉 Yeah this seems like a good shout. Worth testing on a dev version on the GAE. Which also reminds me that we need to document how we create a shared instance on the GAE when we make them

I'll make a PR based on step 1 of https://github.com/UCLH-DHCT/emap/edit/main/docs/core.md

"MaximumPatientCount": 20000,
// Limit the maximum storage size
"MaximumPatientCount" : 0, // no limit
"MaximumStorageSize" : ${ORTHANC_RAW_MAXIMUM_STORAGE_SIZE}, // MB
Copy link
Member Author

@milanmlft milanmlft Dec 7, 2023

Choose a reason for hiding this comment

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

Turns out this doesn't work and results in an error ERROR: Unable to parse Json file '/run/secrets/orthanc.json'; check syntax , with the orthanc-raw service not even starting.

Using

  "MaximumStorageSize" : "${ORTHANC_RAW_MAXIMUM_STORAGE_SIZE}", // MB

in turn leads to the MaximumStorageSize parameter being ignored, because of a Bad parameter type error, as it needs to be an integer...

I've tried a couple of different things but it seems it's not possible get an environment variable read in as an integer in orthanc.json 🙁

@stefpiatek, @t-young31 any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

😞
no good ones. could: (a) hardcode, (b) copy the config file into the image and sed from a build argument. I don't suppose it works if you remove the quotes?

Copy link
Contributor

@stefpiatek stefpiatek Dec 7, 2023

Choose a reason for hiding this comment

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

Yeah I would have thought having the env variable without the quotes would work, sad if it doesn't. Agreed on using sed to replace inline if not, perhaps wrapping the variable usage with something that will make that replacement unique

@milanmlft
Copy link
Member Author

Closing this, in favour of #179

@milanmlft milanmlft closed this Dec 12, 2023
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.

Storage control on Orthanc instances
3 participants