Bug 2025868: Check HyperVGenerations for instance type #5509

rna-afk · 2022-01-04T15:09:43Z

If an instance type that does not support HyperVGeneration version 1
then terraform returns an error mentioning there's support only for
V1. Adding a check during install config to check for the versions
supported by the instance type provided.

openshift-ci · 2022-01-04T15:09:50Z

@rna-afk: This pull request references Bugzilla bug 2025868, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @MayXuQQ

Details

In response to this:

Bug 2025868: Check HyperVGenerations for instance type

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.

pkg/asset/installconfig/azure/validation.go

patrickdillon · 2022-01-04T18:27:35Z

I believe (am trying to get images in order to test) that Gen2 VMs may work with compute nodes when we use images from the Azure Marketplace.

Therefore, I think we should only run this validation against control plane nodes. Or perhaps also against compute nodes when a marketplace image is not being used.

pkg/asset/installconfig/azure/validation.go

If an instance type that does not support HyperVGeneration version 1 then terraform returns an error mentioning there's support only for V1. Adding a check during install config to check for the versions supported by the instance type provided.

staebler · 2022-01-04T18:37:00Z

I believe (am trying to get images in order to test) that Gen2 VMs may work with compute nodes when we use images from the Azure Marketplace.

Therefore, I think we should only run this validation against control plane nodes. Or perhaps also against compute nodes when a marketplace image is not being used.

The install config does not support using marketplace images for the compute nodes yet, though, right? It still requires the user manually adjusting the machineset manifests.

patrickdillon · 2022-01-05T17:41:53Z

I believe (am trying to get images in order to test) that Gen2 VMs may work with compute nodes when we use images from the Azure Marketplace.
Therefore, I think we should only run this validation against control plane nodes. Or perhaps also against compute nodes when a marketplace image is not being used.

The install config does not support using marketplace images for the compute nodes yet, though, right? It still requires the user manually adjusting the machineset manifests.

Ah, yes, that is correct. So you can ignore my comment and once we add support in the install config we can skip this check (assuming the images do support Gen2).

patrickdillon · 2022-01-06T00:31:07Z

On closer inspection, I believe what is happening is that we are creating the cluster image with the default Gen1 value (see https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/image#hyper_v_generation). This is going to create all VMs created from the image as Gen1.

I believe this image is used throughout the lifespace of the cluster. I think it would be worth investigating whether there are any gen1-only instance types we want to support. Otherwise, I think we could just set the image to have generation = 2. I will try a test install with that configuration.

patrickdillon · 2022-01-06T03:37:31Z

By changing the hyper_v_generation on the image I was able to create a cluster of gen2 VMs. Seems like there are a few different ways we could take this.

rna-afk · 2022-01-06T15:44:11Z

Interesting so instead of this, can we just pass the generation value to terraform?

patrickdillon · 2022-01-06T15:57:15Z

Interesting so instead of this, can we just pass the generation value to terraform?

I have been looking into this more and I think the best course of action is to add support for both Gen1 & Gen2 VMs in our next version.

I was hoping we could either just choose all Gen1 or Gen2, but I think the Instance Type landscape is too diverse to do that.

So I think what we should do as a new feature, is have the installer create two images, a gen1 and gen2 image. We would use code similar to what you have in this PR, but when an instance type has only gen1, we would set the gen1 image in the machineset and pass the values through to terraform for the masters. Otherwise we default to the gen2 image.

I'm not sure if code changes would be needed for the machine-api operator. I don't think they would be needed, but it would be necessary if a new machineset were created that a compatible image and instance type were specified. This would probably just be a matter of documentation.

So until we add this feature I am happy with this approach.

patrickdillon · 2022-01-27T03:11:29Z

/test e2e-azure
/lgtm
/approve

openshift-ci · 2022-01-27T03:11:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/asset/installconfig/azure/OWNERS~~ [patrickdillon]

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

openshift-bot · 2022-01-27T03:43:55Z

/retest-required

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

openshift-bot · 2022-01-27T03:55:55Z

/retest-required

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

openshift-bot · 2022-01-27T06:31:54Z

/retest-required

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

openshift-bot · 2022-01-27T07:07:55Z

/retest-required

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

openshift-bot · 2022-01-27T10:20:57Z

/retest-required

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

openshift-bot · 2022-01-27T10:32:57Z

/retest-required

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

openshift-bot · 2022-01-27T10:44:56Z

/retest-required

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

openshift-bot · 2022-01-27T10:56:58Z

/retest-required

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

openshift-bot · 2022-01-27T12:33:55Z

/retest-required

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

openshift-bot · 2022-01-27T14:33:59Z

/retest-required

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

openshift-bot · 2022-01-27T14:57:59Z

/retest-required

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

openshift-ci · 2022-01-27T15:06:07Z

@rna-afk: The following tests 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-aws-workers-rhel8	`ae6805b`	link	false	`/test e2e-aws-workers-rhel8`
ci/prow/e2e-azure-upi	`ae6805b`	link	false	`/test e2e-azure-upi`
ci/prow/e2e-alibaba	`ae6805b`	link	true	`/test e2e-alibaba`

Full PR test history. Your PR dashboard.

Details

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 · 2022-01-27T16:39:57Z

/retest-required

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

rna-afk · 2022-01-27T17:17:59Z

/skip e2e-alibaba

rna-afk · 2022-01-27T18:14:15Z

e2e-alibaba is failing and was set to required by mistake. Overriding it.

/skip

openshift-ci · 2022-01-28T16:04:15Z

@rna-afk: All pull requests linked via external trackers have merged:

openshift/installer#5509

Bugzilla bug 2025868 has been moved to the MODIFIED state.

Details

In response to this:

Bug 2025868: Check HyperVGenerations for instance type

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 bot added the bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. label Jan 4, 2022

openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 4, 2022

openshift-ci bot requested review from MayXuQQ, fabianofranz and m1kola January 4, 2022 15:09

staebler reviewed Jan 4, 2022

View reviewed changes

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

rna-afk force-pushed the azure_check_hypergeneration branch 2 times, most recently from 9d063a9 to 2284454 Compare January 4, 2022 16:12

rna-afk requested a review from staebler January 4, 2022 18:04

patrickdillon reviewed Jan 4, 2022

View reviewed changes

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

rna-afk force-pushed the azure_check_hypergeneration branch from 2284454 to ed4cdad Compare January 4, 2022 18:20

staebler reviewed Jan 4, 2022

View reviewed changes

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

rna-afk force-pushed the azure_check_hypergeneration branch from ed4cdad to ae6805b Compare January 4, 2022 18:36

rna-afk requested a review from staebler January 10, 2022 13:23

openshift-ci bot assigned patrickdillon Jan 27, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2022

openshift-merge-robot merged commit 81b8ace into openshift:master Jan 28, 2022

Bug 2025868: Check HyperVGenerations for instance type #5509

Bug 2025868: Check HyperVGenerations for instance type #5509

Uh oh!

Conversation

rna-afk commented Jan 4, 2022

Uh oh!

openshift-ci bot commented Jan 4, 2022

Uh oh!

Uh oh!

Uh oh!

patrickdillon commented Jan 4, 2022

Uh oh!

Uh oh!

staebler commented Jan 4, 2022

Uh oh!

patrickdillon commented Jan 5, 2022

Uh oh!

patrickdillon commented Jan 6, 2022

Uh oh!

patrickdillon commented Jan 6, 2022

Uh oh!

rna-afk commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickdillon commented Jan 6, 2022

Uh oh!

patrickdillon commented Jan 27, 2022

Uh oh!

openshift-ci bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-ci bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

rna-afk commented Jan 27, 2022

Uh oh!

rna-afk commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jan 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rna-afk commented Jan 6, 2022 •

edited

Loading

rna-afk commented Jan 27, 2022 •

edited

Loading