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

Bug OCPBUGS-164: OpenStack: Set minimum disk of a flavor to 100 GB #6268

Merged
merged 1 commit into from
Aug 30, 2022

Conversation

dulek
Copy link
Contributor

@dulek dulek commented Aug 25, 2022

Other platforms require at least 100 GB of disk size and we've updated
openshift-docs to reflect that in OpenStack too. Seems like we forgot to
update flavor validation code and docs in the installer. This commit
fixes this.

@openshift-ci-robot openshift-ci-robot added jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Aug 25, 2022
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Aug 25, 2022

@dulek: This pull request references [Jira Issue OCPBUGS-164](https://issues.redhat.com//browse/OCPBUGS-164), which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.12.0) matches configured target version for branch (4.12.0)
  • bug is in the state New, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST)

In response to this:

Other platforms require at least 100 GB of disk size and we've updated
openshift-docs to reflect that in OpenStack too. Seems like we forgot to
update flavor validation code and docs in the installer. This commit
fixes this.

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.

@dulek
Copy link
Contributor Author

dulek commented Aug 25, 2022

/override ci/prow/e2e-openstack-kuryr

At least this one doesn't need to run here.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 25, 2022

@dulek: dulek unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight.

In response to this:

/override ci/prow/e2e-openstack-kuryr

At least this one doesn't need to run here.

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.

@dulek
Copy link
Contributor Author

dulek commented Aug 26, 2022

cc @mandre

Copy link
Member

@mandre mandre left a comment

Choose a reason for hiding this comment

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

Well, ok the validation works, but now CI is failing because our flavors only have a 80GB disk (which should be enough for CI purpose).
How about we export the OPENSHIFT_INSTALL_SKIP_PREFLIGHT_VALIDATIONS environment variable in CI?

docs/user/openstack/README.md Show resolved Hide resolved
@dulek
Copy link
Contributor Author

dulek commented Aug 26, 2022

Yup, that will be required, I'm on it.

@pierreprinetti
Copy link
Member

How about we export the OPENSHIFT_INSTALL_SKIP_PREFLIGHT_VALIDATIONS environment variable in CI?

I think that we should test on flavors that reflect the minimum requirements exactly. I'm going to create such flavors on all our CI clouds and update our CI configuration. WDYT @mandre @dulek?

If instead we think that 80GB are enough, then why setting the requirement to 100GB in the docs?

@mandre
Copy link
Member

mandre commented Aug 26, 2022

How about we export the OPENSHIFT_INSTALL_SKIP_PREFLIGHT_VALIDATIONS environment variable in CI?

I think that we should test on flavors that reflect the minimum requirements exactly. I'm going to create such flavors on all our CI clouds and update our CI configuration. WDYT @mandre @dulek?

Please confirm first that we have enough quota / resource in our clouds.

If instead we think that 80GB are enough, then why setting the requirement to 100GB in the docs?

Because the product docs are intended for customers (you'll need to account for the growing size of data as your cluster ages) and not CI. Also, it aligns with other platforms.

@pierreprinetti
Copy link
Member

you'll need to account for the growing size of data as your cluster ages

I thought that this was not in scope of minimum requirements.

What does "minimum" in "minimum requirements for masters" mean here?

Either it means "the bare minimum for installing", and then we can set it to 80GB.
Or we want to give hints as to the cluster growth requirements, and then maybe we can go the extra mile and see if there's a metric (e.g. the number of pods) that can be a good proxy for calculating the required disk size.

As for the workers, I think we should really state what's the minimum requirements for standing up the cluster, because end users will need to figure out the extra resources based on their workloads.

@dulek
Copy link
Contributor Author

dulek commented Aug 26, 2022

you'll need to account for the growing size of data as your cluster ages

I thought that this was not in scope of minimum requirements.

What does "minimum" in "minimum requirements for masters" mean here?

Either it means "the bare minimum for installing", and then we can set it to 80GB. Or we want to give hints as to the cluster growth requirements, and then maybe we can go the extra mile and see if there's a metric (e.g. the number of pods) that can be a good proxy for calculating the required disk size.

As for the workers, I think we should really state what's the minimum requirements for standing up the cluster, because end users will need to figure out the extra resources based on their workloads.

I would not overstep here and try to be smarter than the core OpenShift team that recommends 100 GB for other platforms. 100 GB is in the official docs for a while now, we got to stick with it. The only question is if we can go away with 80 GB on the CI, which might be true as CI was working okay for a while.

@mandre
Copy link
Member

mandre commented Aug 26, 2022

Then perhaps we want to have a warning when the flavors disk size is between 25 (the absolute minimum required to install a cluster) and 100 GB (the recommended minimum value for production clusters) and fail the validation when it's under 25GB like it is the case today. WDYT?

@pierreprinetti
Copy link
Member

pierreprinetti commented Aug 26, 2022

Then perhaps we want to have a warning when the flavors disk size is between 25 (the absolute minimum required to install a cluster) and 100 GB (the recommended minimum value for production clusters) and fail the validation when it's under 25GB like it is the case today. WDYT?

LGTM 👍

and keep the docs at 100GB as per the other platforms, right?

@dulek dulek force-pushed the openstack-update-disk branch from da3ebc8 to 86e5b6b Compare August 26, 2022 10:33
@dulek
Copy link
Contributor Author

dulek commented Aug 26, 2022

Then perhaps we want to have a warning when the flavors disk size is between 25 (the absolute minimum required to install a cluster) and 100 GB (the recommended minimum value for production clusters) and fail the validation when it's under 25GB like it is the case today. WDYT?

LGTM +1

and keep the docs at 100GB as per the other platforms, right?

Alright, I did so then. Seems like in other platforms warnings are just logged, so I haven't modified the whole validation system to be able to return both fatal errors and warnings.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 26, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierreprinetti

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2022
Other platforms require at least 100 GB of disk size and we've updated
openshift-docs to reflect that in OpenStack too. Seems like we forgot to
update flavor validation code and docs in the installer. This commit
fixes this.
@dulek dulek force-pushed the openstack-update-disk branch from 86e5b6b to 4e4b17a Compare August 30, 2022 08:33
@pierreprinetti
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2022
@openshift-merge-robot openshift-merge-robot merged commit f08f7d2 into openshift:master Aug 30, 2022
@openshift-ci-robot
Copy link
Contributor

@dulek: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-164 has been moved to the MODIFIED state.

In response to this:

Other platforms require at least 100 GB of disk size and we've updated
openshift-docs to reflect that in OpenStack too. Seems like we forgot to
update flavor validation code and docs in the installer. This commit
fixes this.

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-ci
Copy link
Contributor

openshift-ci bot commented Aug 30, 2022

@dulek: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-openstack-proxy 4e4b17a link false /test e2e-openstack-proxy

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants