-
Notifications
You must be signed in to change notification settings - Fork 2.1k
azure: upi: sync install workflow with installer changes #34678
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: upi: sync install workflow with installer changes #34678
Conversation
|
@r4f4: the following rehearsable tests have been affected by this change:
Prior to this PR being merged, you will need to either run and acknowledge or opt to skip these rehearsals. Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/hold |
0d61641 to
bde43a3
Compare
|
/pj-rehearse |
|
/pj-rehearse pull-ci-openshift-installer-master-e2e-azure-ovn-upi |
|
@r4f4: job(s): pull-ci-openshift-installer-master-e2e-azure-ovn-upi either don't exist or were not found to be affected, and cannot be rehearsed |
|
/pj-rehearse refresh |
|
@r4f4: the following rehearsable tests have been affected by this change:
|
|
/pj-rehearse pull-ci-openshift-installer-master-e2e-azure-ovn-upi |
1 similar comment
|
/pj-rehearse pull-ci-openshift-installer-master-e2e-azure-ovn-upi |
47152e5 to
e0485e9
Compare
|
/pj-rehearse pull-ci-openshift-installer-master-e2e-azure-ovn-upi |
e0485e9 to
93fa131
Compare
|
/hold cancel |
|
@r4f4: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/pj-rehearse ack |
df42d2d to
8e2b88c
Compare
ci-operator/step-registry/upi/install/azure/upi-install-azure-commands.sh
Outdated
Show resolved
Hide resolved
|
/lgtm |
This change adds support to both Managed Images and Image Galleries for the UPI workflow. Image Galleries support was added to the openshift-installer to enable aarch64 IPI deployments and openshift/installer#6684 will soon eanble it for UPI as well.
8e2b88c to
bbdbd8a
Compare
|
create UPI cluster succeed, and after scaleup, worker created succeed.
/label qe-approved |
patrickdillon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
| --parameters baseName="$INFRA_ID" | ||
|
|
||
| # Check if it's the new template using Image Galleries instead of Managed Images | ||
| if grep -qs "Microsoft.Compute/galleries" 02_storage.json; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am reluctant to bring this up without a better suggestion, but I worry that putting conditionals in a template like this can be risky in terms of legibility/debugability. Our old system of templates became so convoluted that it was hard to follow the execution path, so i am perhaps overly hesitant whenever I see any conditionals.
The change introduced here is simple, so I think it is better than any of the alternatives I can come up with, which would be overkill or more confusing than this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the best idea I could come up with to make this work with both Managed Images and Galleries. Relying on the installer or release payload versions didn't feel reliable enough. Once 4.12 migrates to Galleries and this workflow is run on 4.12+, then we can drop this conditional. But I agree with you this is not ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patrickdillon @barbacbd pointed out an interesting idea. We could use the contentVersion from the ARM template [1] [2]. It still a conditional but at least it checks for a version instead of a string inside the template. Then it's up to the Installer team to make sure that version is updated accordingly when there are changes to the templates.
[1] https://github.com/openshift/installer/blob/master/upi/azure/02_storage.json#L3
[2] https://learn.microsoft.com/en-us/azure/azure-resource-manager/templates/syntax#template-format
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon, Prashanth684, r4f4 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This change is inter-dependent on openshift/installer#6684This change adds support to both Managed Images and Image Galleries for the UPI workflow. Image Galleries support was added in the installer to enable
aarch64IPI deployments and openshift/installer#6684 will soon enable it for UPI as well.