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

Make the bash script's style more consistent and appropriate for today's resource usage #181

Merged
merged 4 commits into from
Sep 30, 2020

Conversation

renich
Copy link
Contributor

@renich renich commented Sep 15, 2020

Added proper form long options and followed general recommendations here: https://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html

Also suggesting a biger amount of RAM and the use of 3 virtual CPUs at least.

@renich
Copy link
Contributor Author

renich commented Sep 15, 2020

Also, it might be a good idea to pump up the proposed RAM usage. Silverblue suggests 3072 right now but I know nothing.

And, maybe suggest 2 or 4 vCPUs?

@lucab
Copy link
Contributor

lucab commented Sep 16, 2020

@renich yes thanks! A 3GiB / 2 vCPU instance looks indeed more reasonable, and keeping the actual values in variables makes it straightforward for people to tweak. Feel free to amend your commit with that.

@lucab lucab self-requested a review September 16, 2020 08:19
i've updated the amount of RAM to 3 GiB and added a vcpu count of 3. This is, probably, a safer minimal requirement.
It seems that one of the virt-install lines was a bit too long. I've moved it to it's own line.
Copy link
Contributor Author

@renich renich left a comment

Choose a reason for hiding this comment

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

OK, how's this?

@renich renich changed the title Make the bash script's style more consistent Make the bash script's style more consistent and appropriate for today's resource usage Sep 16, 2020
Copy link
Member

@travier travier left a comment

Choose a reason for hiding this comment

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

LGTM

@jlebon
Copy link
Member

jlebon commented Sep 22, 2020

Also suggesting a biger amount of RAM and the use of 3 virtual CPUs at least.

Hmm, isn't it understood that users can add more vCPUs/RAM as needed for their use case? Or were you seeing degradation of some kind with the current settings and a minimal workload? If not, I think I'd rather stick with the base resources we have now. (Note for example that a lot of our CI tests run FCOS with 1 vCPU and 1G of RAM just fine.)

@renich
Copy link
Contributor Author

renich commented Sep 24, 2020

Hmm, isn't it understood that users can add more vCPUs/RAM as needed for their use case? Or were you seeing degradation of some kind with the current settings and a minimal workload? If not, I think I'd rather stick with the base resources we have now. (Note for example that a lot of our CI tests run FCOS with 1 vCPU and 1G of RAM just fine.)

I'm just saying that we should provide a sane minimum. It's a quickstart so stuff should be, more less, ready to run on non-minimal workloads; so that the user can test it without worrying about that. Maybe 2 vCPUs would be enough for this, though. But being 1.5 GiB the minimum "recommended" by libvirt when using the Fedora 32/CentOS 8/RHEL 8.3 profile, I think 1 extra gig isn't insane.

@renich
Copy link
Contributor Author

renich commented Sep 24, 2020

s/libvirt/virt-manager/g

@jlebon
Copy link
Member

jlebon commented Sep 24, 2020

Maybe 2 vCPUs would be enough for this, though. But being 1.5 GiB the minimum "recommended" by libvirt when using the Fedora 32/CentOS 8/RHEL 8.3 profile, I think 1 extra gig isn't insane.

Looks like f32 is 1G min/2G recommended, while el8 is indeed 1.5G min/recommended. I think it makes sense to match the recommended amount for the rest of Fedora here (and actually in coreos/fedora-coreos-tracker#221 we're talking about eventually adding FCOS to osinfo-db). So how about we meet halfway and bump to 2 vCPUs but leave the RAM at 2G? :)

I don't want to give the impression that FCOS needs a minimum of 3G of RAM to be useful. Part of our messaging is that we're a lightweight OS.

Adhering to the recommended resources for Fedora 32/RHEL 8.
@renich
Copy link
Contributor Author

renich commented Sep 28, 2020

OK, I've updated the branch.

@lucab lucab requested a review from jlebon September 30, 2020 12:15
@lucab
Copy link
Contributor

lucab commented Sep 30, 2020

LGTM too, leaving to @jlebon for the final pass/merge.

Copy link
Member

@jlebon jlebon 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 the patch!

@jlebon jlebon merged commit 51018e4 into coreos:master Sep 30, 2020
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.

4 participants