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

feat(client): adapt session storage to repository #3058

Merged
merged 6 commits into from
Mar 8, 2024

Conversation

leafty
Copy link
Member

@leafty leafty commented Mar 4, 2024

Fixes #3057.

Adds storage size computation based on GitLab statistics when starting a session.

  • minimumStorage is repository_size or repository_size + 2 x lfs_objects_size if LFS auto-fetch is enabled
  • recommendedStorage is minimumStorage + lfs_objects_size + 1GB or minimumStorage + 1GB if LFS auto-fetch is enabled

All sizes are rounded up to the next gigabytes and we use powers of 10 to go from bytes to GB.

The recommendedStorage is used as a default value and minimumStorage is used to interrupt the auto-start session flow and to filter session classes.

Note that statistics are available only to project members, so starting sessions on public projects could still lead to session start issues.

/deploy

@leafty leafty temporarily deployed to renku-ci-ui-3058 March 4, 2024 08:33 — with GitHub Actions Inactive
@RenkuBot
Copy link
Contributor

RenkuBot commented Mar 4, 2024

You can access the deployment of this PR at https://renku-ci-ui-3058.dev.renku.ch

Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

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

This looks very good!

I think the code that computes the storage size to request would benefit from a few test cases in the repo, but that is the only thing that needs to be added.

client/src/features/session/utils/sessionOptions.utils.ts Outdated Show resolved Hide resolved
Comment on lines 52 to 69
const { minimumStorageGb, recommendedStorageGb } =
checkStorage({ lfsAutoFetch, statistics }) ?? {};
const clampedRecommended = recommendedStorageGb
? validateStorageAmount({
value: recommendedStorageGb,
maxValue: currentSessionClass.max_storage,
})
: null;
const requestedStorage =
clampedRecommended &&
minimumStorageGb &&
clampedRecommended > currentSessionClass.default_storage &&
clampedRecommended > minimumStorageGb
? clampedRecommended
: minimumStorageGb &&
minimumStorageGb > currentSessionClass.default_storage
? minimumStorageGb
: currentSessionClass.default_storage;
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: extract this logic into a utility function in the sessionOptions.utils.ts file. That would allow writing a test for it; I think I understand the logic here, but a few test cases would greatly help clarify.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 9bbc1e2.

Co-authored-by: Chandrasekhar Ramakrishnan <ciyer@users.noreply.github.com>
@leafty leafty temporarily deployed to renku-ci-ui-3058 March 7, 2024 14:32 — with GitHub Actions Inactive
@leafty
Copy link
Member Author

leafty commented Mar 7, 2024

This looks very good!

I think the code that computes the storage size to request would benefit from a few test cases in the repo, but that is the only thing that needs to be added.

Done in 9bbc1e2.

@leafty leafty requested a review from ciyer March 7, 2024 14:32
Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

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

Thanks for adding those refactoring and adding the tests! 🚀

@leafty leafty merged commit 487a8d5 into main Mar 8, 2024
17 checks passed
@leafty leafty deleted the leafty/3057-adapt-session-storage-size branch March 8, 2024 08:25
@RenkuBot
Copy link
Contributor

RenkuBot commented Mar 8, 2024

Tearing down the temporary RenkuLab deplyoment for this PR.

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.

At the start of a session, check if the storage size is compatible with the repository
3 participants