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

asset/installconfig/Azure: Validate install-config instance types #4419

Merged
merged 1 commit into from Dec 3, 2020

Conversation

jstuever
Copy link
Contributor

Ensure the user provided instance types meets minimum resource
requirements.

@jstuever
Copy link
Contributor Author

/test e2e-azure

@jstuever
Copy link
Contributor Author

@jstuever
Copy link
Contributor Author

/retest

pkg/asset/installconfig/azure/client.go Outdated Show resolved Hide resolved
pkg/asset/installconfig/azure/client.go Outdated Show resolved Hide resolved
pkg/asset/installconfig/azure/client.go Outdated Show resolved Hide resolved
pkg/asset/installconfig/azure/client.go Outdated Show resolved Hide resolved

for _, capability := range *typeMeta.Capabilities {

if strings.EqualFold(*capability.Name, "vCPUs") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we willing to accept a SKU that does not have the "vCPUs" or "MemoryGB" capability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, if the capability is not present, then we won't show any error. We filter out any SKU that is not of type "virtualMachines". I am unable to find anything in the docs where this type of SKU is guaranteed to have these capabilities specified (or not).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should move forward as-is. We can always make changes if we find cu issues later on.

pkg/asset/installconfig/azure/client.go Show resolved Hide resolved
Ensure the user provided instance types meets minimum resource
requirements.
@jstuever
Copy link
Contributor Author

jstuever commented Dec 3, 2020

/test e2e-azure

@mjudeikis
Copy link
Contributor

overall looks good. Is this in any is aligned with machine-api to do the same when creating new sets? we seen that in most cases user workaround this by creating a cluster and changing compute types post creation :/

Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Dec 3, 2020

@jstuever: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-crc 91402a2 link /test e2e-crc

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.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit b9701c5 into openshift:master Dec 3, 2020
@jstuever jstuever deleted the cors1549 branch February 24, 2021 20:33
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants