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

azure: support selecting marketplace image in install config #5799

Closed
wants to merge 5 commits into from

Conversation

staebler
Copy link
Contributor

Additional fields added to Azure machine pools that give the user the option to select the marketplace image to use for the OS image. The OS image can only be selected for compute machine pools. Validation will be performed that (1) the marketplace image selected exists and (2) that the license terms for the marketplcae image selected have been accepted.

osImage:
  publisher: <string>
  offer: <string>
  sku: <string>
  version: <string>

https://issues.redhat.com/browse/CORS-1824

@MayXuQQ
Copy link
Contributor

MayXuQQ commented Apr 13, 2022

@staebler when the PR is rebased to branch master, please notify me, I can do pre-merge test. thanks a lot.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2022
@staebler
Copy link
Contributor Author

@patrickdillon
Copy link
Contributor

nice work!
/approve
/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 14, 2022
@openshift-bot
Copy link
Contributor

/retest-required

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

3 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@MayXuQQ
Copy link
Contributor

MayXuQQ commented Apr 14, 2022

@staebler master nodes will support customize the image with market image ? based on my test, just worker nodes use the market image.

@openshift-bot
Copy link
Contributor

/retest-required

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

@MayXuQQ
Copy link
Contributor

MayXuQQ commented Apr 14, 2022

@staebler test with pr-merge, Failed.
$ az vm list -g maxu-5799-q44n6-rg -o tsv --query "[].[name,provisioningState]"
maxu-5799-q44n6-master-0 Succeeded
maxu-5799-q44n6-master-1 Succeeded
maxu-5799-q44n6-master-2 Succeeded
maxu-5799-q44n6-worker-southcentralus1-h8sqv Failed
maxu-5799-q44n6-worker-southcentralus2-7jlnn Failed
maxu-5799-q44n6-worker-southcentralus3-brfqd Failed

kubeconfig

$ oc describe machine maxu-5799-q44n6-worker-southcentralus1-h8sqv -n openshift-machine-api
...
Events:
Type Reason Age From Message


Warning FailedCreate 125m azure-controller CreateError: failed to reconcile machine "maxu-5799-q44n6-worker-southcentralus1-h8sqv"s: failed to create vm maxu-5799-q44n6-worker-southcentralus1-h8sqv: failed to create or get machine: compute.VirtualMachinesCreateOrUpdateFuture: asynchronous operation has not completed

@staebler
Copy link
Contributor Author

@staebler master nodes will support customize the image with market image ? based on my test, just worker nodes use the market image.

Only the worker nodes can use market images.

Copy link
Contributor

@patrickdillon patrickdillon left a comment

Choose a reason for hiding this comment

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

/lgtm cancel

pkg/asset/machines/azure/machines.go Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 14, 2022
@staebler staebler force-pushed the azure_marketplace branch 3 times, most recently from 645815a to 44e3123 Compare April 20, 2022 14:53
@staebler
Copy link
Contributor Author

645815a...44e3123

  • Added missing descriptions for new Azure client methods.

@MayXuQQ
Copy link
Contributor

MayXuQQ commented Apr 21, 2022

test pass with installer_payload_image: registry.build01.ci.openshift.org/ci-ln-dk40i2t/release:latest
/label qe-approved

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 21, 2022

@MayXuQQ: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

test pass with installer_payload_image: registry.build01.ci.openshift.org/ci-ln-dk40i2t/release:latest
/label qe-approved
/bugzilla cc-qa

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 openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Apr 21, 2022
@MayXuQQ
Copy link
Contributor

MayXuQQ commented Apr 21, 2022

we will support customize the osImage in installconfig.control.platform.azure.osImage, installconfig.compute.platform.azure.osImage and installconfig.platform.azure.defaultMachinePlatform.osImage, even the market Image just can be used in installconfig.compute.platform.azure.osImage , right ? @staebler

@staebler
Copy link
Contributor Author

we will support customize the osImage in installconfig.control.platform.azure.osImage, installconfig.compute.platform.azure.osImage and installconfig.platform.azure.defaultMachinePlatform.osImage, even the market Image just can be used in installconfig.compute.platform.azure.osImage , right ? @staebler

No. The validation should fail if the fields of installconfig.controlPlane.platform.azure.osImage or installconfig.platform.azure.defaultMachinePlatform.osImage are set.

@openshift-bot
Copy link
Contributor

/retest-required

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

4 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

Vendor in additional packages from github.com/Azure/azure-sdk-for-go
to support the calls needed to validate Azure marketplace images.
Additional fields added to Azure machine pools that give the user
the option to select the marketplace image to use for the OS image.
The OS image can only be selected for compute machine pools.
Validation will be performed that (1) the marketplace image selected
exists and (2) that the license terms for the marketplcae image
selected have been accepted.

```
osImage:
  publisher: <string>
  offer: <string>
  sku: <string>
  version: <string>
```

https://issues.redhat.com/browse/CORS-1824
Re-generate the mock for the Azure client so that the additional
functions added to support marketplace images are included in the
mock.
Add unit tests for the static validation of marketplace images
selected in the install config.
Add unit tests for the dynamic validation of the Azure marketplace images.
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2022
@staebler
Copy link
Contributor Author

@patrickdillon
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2022
@openshift-bot
Copy link
Contributor

/retest-required

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

4 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-bot
Copy link
Contributor

/retest-required

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

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 23, 2022

@staebler: 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-gcp-shared-vpc aaba77d link false /test e2e-gcp-shared-vpc
ci/prow/openstack-manifests aaba77d link true /test openstack-manifests
ci/prow/e2e-gcp-upi-xpn aaba77d link false /test e2e-gcp-upi-xpn
ci/prow/e2e-aws-proxy aaba77d link false /test e2e-aws-proxy
ci/prow/e2e-alibaba aaba77d link false /test e2e-alibaba
ci/prow/e2e-openstack-parallel aaba77d link false /test e2e-openstack-parallel
ci/prow/e2e-metal-ipi-ovn-ipv6 aaba77d link true /test e2e-metal-ipi-ovn-ipv6
ci/prow/e2e-aws-disruptive aaba77d link false /test e2e-aws-disruptive
ci/prow/e2e-openstack 768e914 link false /test e2e-openstack
ci/prow/e2e-azure 768e914 link false /test e2e-azure
ci/prow/e2e-ovirt 768e914 link false /test e2e-ovirt
ci/prow/e2e-metal-assisted 768e914 link false /test e2e-metal-assisted
ci/prow/e2e-gcp 768e914 link false /test e2e-gcp
ci/prow/e2e-ibmcloud 768e914 link false /test e2e-ibmcloud
ci/prow/okd-unit 768e914 link true /test okd-unit
ci/prow/e2e-azurestack 768e914 link false /test e2e-azurestack
ci/prow/e2e-libvirt 768e914 link false /test e2e-libvirt
ci/prow/unit 768e914 link true /test unit
ci/prow/e2e-crc 768e914 link false /test e2e-crc
ci/prow/e2e-azure-resourcegroup 768e914 link false /test e2e-azure-resourcegroup
ci/prow/e2e-metal-ipi 768e914 link false /test e2e-metal-ipi
ci/prow/govet 768e914 link true /test govet

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.

@patrickdillon
Copy link
Contributor

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2022
@patrickdillon
Copy link
Contributor

Bad rebase. Close in favor of #5839

@patrickdillon
Copy link
Contributor

/close

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 28, 2022

@staebler: PR needs rebase.

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 openshift-ci bot closed this Apr 28, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 28, 2022

@patrickdillon: Closed this PR.

In response to this:

/close

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
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants