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

GH#35793 Update minimum storage requirements for all providers from 120GB -> 100GB #36226

Merged
merged 1 commit into from
Sep 17, 2021

Conversation

codyhoag
Copy link
Contributor

@codyhoag codyhoag commented Sep 9, 2021

Resolves #35793

Updates were made in the machine requirement section for all providers that specified that table. For example:

@codyhoag codyhoag changed the title Update storage requirements for all providers from 120GB -> 100GB GH#35793 Update storage requirements for all providers from 120GB -> 100GB Sep 9, 2021
@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 9, 2021
@netlify
Copy link

netlify bot commented Sep 9, 2021

✔️ Deploy Preview for osdocs ready!

🔨 Explore the source changes: 6cf94d3

🔍 Inspect the deploy log: https://app.netlify.com/sites/osdocs/deploys/6144c82a46f8e500074e4d34

😎 Browse the preview: https://deploy-preview-36226--osdocs.netlify.app

@codyhoag
Copy link
Contributor Author

codyhoag commented Sep 9, 2021

@miabbott can you verify? Thanks!

@codyhoag codyhoag changed the title GH#35793 Update storage requirements for all providers from 120GB -> 100GB GH#35793 Update minimum storage requirements for all providers from 120GB -> 100GB Sep 9, 2021
@miabbott
Copy link
Member

miabbott commented Sep 9, 2021

LGTM

@codyhoag
Copy link
Contributor Author

codyhoag commented Sep 9, 2021

Due to our docs support policy, this will require several ACKs from stakeholders before merging.

@nstielau @xltian @sferich888 @makentenza @vikram-redhat can you ACK this documentation update for minimum storage requirements for a cluster VM? Thanks!

@vikram-redhat
Copy link
Contributor

LGTM

@sferich888
Copy link
Contributor

ACK (and ill proxy @nstielau via #35793 (comment) and #35793 (comment))

@nstielau
Copy link

nstielau commented Sep 9, 2021

LGTM, thanks!

@holgwolf
Copy link

one comment from Z and LinuxONE side. The documentation above considers the off-prem side of things for KVM. We should change the minimum for all Z related docs including z/VM as well. I appreciate the lower minimum.

@SNiemann15
Copy link
Contributor

I've verified 100 GB also show in the z/VM on IBM Z and LinuxONE machine requirements now.

@makentenza
Copy link
Contributor

LGTM

@holgwolf
Copy link

I think we should not have a statement about IOPS for z and LinuxONE since this is not commonly used by our customers.

@codyhoag
Copy link
Contributor Author

I think we should not have a statement about IOPS for z and LinuxONE since this is not commonly used by our customers.

@nstielau wanted to check with you on this recommendation. Are you OK with hiding the IOPS column for IBM Z and LinuxONE?

@makentenza
Copy link
Contributor

I think we should not have a statement about IOPS for z and LinuxONE since this is not commonly used by our customers.

@nstielau wanted to check with you on this recommendation. Are you OK with hiding the IOPS column for IBM Z and LinuxONE?

Is there any technical reason why a Z or LinuxONE customer should have any issues to satisfy the required IOPS? From my point of view not having a high demand on these platforms is not a reason to avoid these requirements.

@pamoedom
Copy link

My 2 cents:

100GB is more than sufficient for a bare minimum installation, some examples:

[IPI AWS]

[root@ip-10-0-141-49 /]# df -h /
Filesystem      Size  Used Avail Use% Mounted on
/dev/nvme0n1p4  100G   12G   89G  12% /

[root@ip-10-0-155-85 /]# df -h /
Filesystem      Size  Used Avail Use% Mounted on
/dev/nvme0n1p4  100G  8.1G   92G   9% /

[UPI BM]

[root@master-00 /]# df -h /
Filesystem      Size  Used Avail Use% Mounted on
/dev/sdc4       112G   14G   98G  13% /

[root@worker-00 /]# df -h /
Filesystem      Size  Used Avail Use% Mounted on
/dev/sda4       224G   15G  209G   7% /

However, please note that IOPS could be critical for ETCD performance and cluster stability, I remember cases in the past where the default type of disks being used had very poor performance (e.g. Azure).

Additional references:

Best Regards.

@holgwolf
Copy link

I think we should not have a statement about IOPS for z and LinuxONE since this is not commonly used by our customers.

@nstielau wanted to check with you on this recommendation. Are you OK with hiding the IOPS column for IBM Z and LinuxONE?

Is there any technical reason why a Z or LinuxONE customer should have any issues to satisfy the required IOPS? From my point of view not having a high demand on these platforms is not a reason to avoid these requirements.

There is not a issue to satisfy those requirements - all testing do show no problems on the setup. I do agree that IO bandwidth needs to be available and the setup needs to be done that the required demand is satisfied.

We verify before each release that all requirements are satisfied and would document anything the customer needs to perform to get the correct setup done.

The IOPS metric is just not really used on Z and LinuxONE and if we introduce this to the documentation we would need to tell the customer how they can find out how to verify what IOPS they have. This would mean service questions. Therefore we would need a whole chapter to tell the customer how he can get this verified.

Therefore I suggest keep the model as we have it right now and do not introduce this part of the table for Z and LinuxONE.

@yunjiang29
Copy link
Contributor

@codyhoag confirmed on AWS platform, LGTM.

@jinyunma
Copy link

I verified 100G on vSphere platform, it works well.

@makentenza
Copy link
Contributor

@codyhoag can we use this PR to reflect this change as well on the provided templates for UPI deployments? At least for AWS we provide 120 fixed sizes (https://docs.openshift.com/container-platform/4.8/installing/installing_aws/installing-aws-user-infra.html#installation-cloudformation-control-plane_installing-aws-user-infra
) but might be others as well

@codyhoag
Copy link
Contributor Author

Because the UPI templates are auto-generated from the openshift/installer repo, once the templates are updated there by Engineering, the changes will be reflected in the docs 👍

@codyhoag
Copy link
Contributor Author

@nstielau can you confirm the action to hide the new IOPS column for IBM Z?
@xltian are there any other verifications this needs from a QE perspective?

@xltian
Copy link

xltian commented Sep 16, 2021

@jianlinliu Can you check if we complete all the verification?

@jianlinliu
Copy link

Thanks for @pamoedom mentioned IOPS, especially for azure platform, that is really a good point. I totally agree with that. cc @mgahagan73 and @Amoghrd to notice this.

After go through the PR, sound like the minimum requirement does not link to azure install, so I think this PR is safe to merge.

@codyhoag
Copy link
Contributor Author

@jianlinliu is Azure the only provider you have concerns for these adjustments applying to? We have a follow-up planned to add this table to providers that don't have it (AWS, Azure, GCP). So we'd like to confirm this information is applicable everywhere (or gather what needs to be adjusted per provider).

@jianlinliu
Copy link

@codyhoag for gcp/aws/azure, as far as I know, installer do some minimum requirement check when running installation, especially for control-plane, I guess the minimum requirement on those cloud platforms should be aligned with the ones installer required.

@nstielau
Copy link

Let's replace the Z IOPS requirement with "N/A". Of course disk performance is still a consideration, but if "iops
isn't the usual metric on Z we can leave it blank. Thanks @holgwolf

@codyhoag
Copy link
Contributor Author

Thanks, all! I have made the recommended changes. We have all the necessary ACKs to get this merged. I'll wait until EOD tomorrow (EST) and proceed with merging if there are no other comments.

@codyhoag codyhoag added the peer-review-needed Signifies that the peer review team needs to review this PR label Sep 17, 2021
Copy link
Contributor

@kalexand-rh kalexand-rh 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 like it's all rendering right. :) I have a style pick, then it's good to go.

@kalexand-rh kalexand-rh added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Sep 17, 2021
@codyhoag codyhoag merged commit 7f74333 into openshift:main Sep 17, 2021
@codyhoag
Copy link
Contributor Author

/cherrypick enterprise-4.9

@codyhoag
Copy link
Contributor Author

/cherrypick enterprise-4.8

@codyhoag
Copy link
Contributor Author

/cherrypick enterprise-4.7

@codyhoag
Copy link
Contributor Author

/cherrypick enterprise-4.6

@openshift-cherrypick-robot

@codyhoag: new pull request created: #36523

In response to this:

/cherrypick enterprise-4.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-cherrypick-robot

@codyhoag: #36226 failed to apply on top of branch "enterprise-4.8":

Applying: Update storage requirements for all providers from 120GB -> 100GB
Using index info to reconstruct a base tree...
M	modules/installation-requirements-user-infra.adoc
Falling back to patching base and 3-way merge...
Auto-merging modules/installation-requirements-user-infra.adoc
CONFLICT (content): Merge conflict in modules/installation-requirements-user-infra.adoc
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Update storage requirements for all providers from 120GB -> 100GB
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick enterprise-4.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-cherrypick-robot

@codyhoag: #36226 failed to apply on top of branch "enterprise-4.7":

Applying: Update storage requirements for all providers from 120GB -> 100GB
Using index info to reconstruct a base tree...
M	modules/installation-requirements-user-infra-ibm-z-kvm.adoc
M	modules/installation-requirements-user-infra.adoc
Falling back to patching base and 3-way merge...
Auto-merging modules/installation-requirements-user-infra.adoc
CONFLICT (content): Merge conflict in modules/installation-requirements-user-infra.adoc
Auto-merging modules/installation-requirements-user-infra-ibm-z-kvm.adoc
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Update storage requirements for all providers from 120GB -> 100GB
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick enterprise-4.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-cherrypick-robot

@codyhoag: #36226 failed to apply on top of branch "enterprise-4.6":

Applying: Update storage requirements for all providers from 120GB -> 100GB
Using index info to reconstruct a base tree...
A	modules/installation-requirements-user-infra-ibm-z-kvm.adoc
M	modules/installation-requirements-user-infra.adoc
Falling back to patching base and 3-way merge...
Auto-merging modules/installation-requirements-user-infra.adoc
CONFLICT (content): Merge conflict in modules/installation-requirements-user-infra.adoc
CONFLICT (modify/delete): modules/installation-requirements-user-infra-ibm-z-kvm.adoc deleted in HEAD and modified in Update storage requirements for all providers from 120GB -> 100GB. Version Update storage requirements for all providers from 120GB -> 100GB of modules/installation-requirements-user-infra-ibm-z-kvm.adoc left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Update storage requirements for all providers from 120GB -> 100GB
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick enterprise-4.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.6 branch/enterprise-4.7 branch/enterprise-4.8 branch/enterprise-4.9 peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

change required minimum disk size from 120GB to 100GB