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(baremetal): Check available disk space #11469

Merged
merged 8 commits into from
Sep 25, 2024

Conversation

Tobbe
Copy link
Member

@Tobbe Tobbe commented Sep 9, 2024

Adds an early step to the yarn rw deploy baremetal command that checks to make sure there is enough free disk space on the server before continuing.

I ran into an issue over the weekend where I was getting the weirdest errors when trying to deploy. It would just fail at random steps during the deploy process. Clearing up some space on the server fixed it. I had a little bit over 1.5GB free when it happened. A typical deploy seems to take up about 720MB for me right now. So it seems I needed more than twice that size for a successful deploy.

For now I decided to check for at least 2 GB free space. Less than that and the deploy will show a warning or error out with a more helpful error message than the errors I was getting 😅

If the user wants to bypass the check they can just pass --no-df or configure freeSpaceRequired to be 0.

Example output:
image

image

image

image

The last one there is what it looks like when you have freeSpaceRequired set to 0
(Please ignore the red > on the ones that are supposed to be passing, it's just because I'm throwing an error later during testing to not actually deploy anything)

Existing projects deploying to servers with very little free disk space available will see a warning like this if they leave the freeSpaceRequired setting not configured

image

@Tobbe Tobbe added the release:feature This PR introduces a new feature label Sep 9, 2024
@Tobbe Tobbe added this to the next-release milestone Sep 9, 2024
@Tobbe Tobbe requested a review from cannikin September 9, 2024 14:01
Copy link
Member

@cannikin cannikin left a comment

Choose a reason for hiding this comment

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

Interesting...I probably would have just added this as a lifecycle thing (adding a [before] hook in deploy.toml) but I can see this being useful for others.

Instead of hardcoding the size to 2GB let's put that value in deploy.toml and let you customize it for your app—set it to something like freeSpaceRequired=2048 in newly generated deploy.toml files, and maybe if the value doesn't exist at all use 2048 by default so that it's backwards compatible for those that don't add the option.

How sure are you about that awk command working for different flavors of *nix df output? Maybe on lines 389 and 395 of baremetal.js you just output a warning if you can't read the output, instead of throwing an error? Because if it can't be read, that's our problem for not parsing the output of df correctly, not the user's. They'll be confused thinking they did something wrong. If we only throw errors then they'll need to add --no-df flag every time they deploy, forever (we shouldn't ever make people do that).

To avoid setting that flag we could do something like allow them to set freeSpaceRequired=false and it'll always skip, or freeSpaceRequired=0 if you have moral objections to two data types in a single variable 😉 (but obviously it should skip all the df checks altogether, not still do them and just check if the result is > 0 or we'd back in the same boat still not being able to parse the output).

Also, need docs updates!

@Tobbe
Copy link
Member Author

Tobbe commented Sep 10, 2024

@cannikin Thanks for your review. Great suggestions as usual 🙂 I think I implemented them all (except the docs updates). Please have another look and let me know what you think. I also added some screenshots to the PR description you can look at.

How sure are you about that awk command working for different flavors of *nix df output?

df is part of coreutils so should be available on all mainstream linux distros and have the same output format across them all. awk is listed as a "core utility" on the Arch wiki and this Unix Stackexchange answer confirms that it also is available on all mainstream distros. So I'm pretty confident in this code.

@@ -173,6 +174,7 @@ This lists a single server, in the `production` environment, providing the hostn
- `repo` - The path to the git repo to clone
- `branch` - [optional] The branch to deploy (defaults to `main`)
- `keepReleases` - [optional] The number of previous releases to keep on the server, including the one currently being served (defaults to 5)
- `freeSpaceRequired` - [optional] The amount of free space required on the server in MB (defaults to 2048 MB). You can set this to `0` to skip checking.
Copy link
Member

@cannikin cannikin Sep 13, 2024

Choose a reason for hiding this comment

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

This says [optional] but doesn't the task itself require it to exist? It only skips if the value is set to 0 but what if you don't have the config option at all? Like if a person upgrades and doesn't this config var, what happens? Can it just also skip if undefined?

Copy link
Member Author

@Tobbe Tobbe Sep 13, 2024

Choose a reason for hiding this comment

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

doesn't the task itself require it to exist?

No, I made it part of DEFAULT_SERVER_CONFIG. So if it's not set at all it will default to 2048 MB.

Copy link
Member

@cannikin cannikin left a comment

Choose a reason for hiding this comment

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

The deploy.toml template should include the new freeSpaceRequired var set at bottom probably?

@Tobbe
Copy link
Member Author

Tobbe commented Sep 13, 2024

The deploy.toml template should include the new freeSpaceRequired var set at bottom probably?

Done ✅

@Tobbe Tobbe added release:breaking This PR is a breaking change and removed release:feature This PR introduces a new feature labels Sep 13, 2024
@Tobbe Tobbe added release:feature This PR introduces a new feature and removed release:breaking This PR is a breaking change labels Sep 25, 2024
@Tobbe Tobbe enabled auto-merge (squash) September 25, 2024 12:11
@Tobbe Tobbe merged commit b05a9a5 into redwoodjs:main Sep 25, 2024
50 checks passed
@Tobbe Tobbe deleted the tobbe-baremetal-df branch September 25, 2024 14:13
@Josh-Walker-GM Josh-Walker-GM modified the milestones: next-release, v8.4.0 Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants