WIP: capi azure async image upload #8782
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-azure-ovn-techpreview |
1 similar comment
|
/test e2e-azure-ovn-techpreview |
| stream, err := rhcos.FetchCoreOSBuild(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get rhcos stream: %w", err) | ||
| } | ||
| archName := arch.RpmArch(string(installConfig.ControlPlane.Architecture)) | ||
| streamArch, err := stream.GetArchitecture(archName) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get rhcos architecture: %w", err) | ||
| } | ||
|
|
||
| azureDisk := streamArch.RHELCoreOSExtensions.AzureDisk | ||
| imageURL := azureDisk.URL |
There was a problem hiding this comment.
Isn't this already done the rhcos/image asset?
| stream, err := rhcos.FetchCoreOSBuild(ctx) | |
| if err != nil { | |
| return fmt.Errorf("failed to get rhcos stream: %w", err) | |
| } | |
| archName := arch.RpmArch(string(installConfig.ControlPlane.Architecture)) | |
| streamArch, err := stream.GetArchitecture(archName) | |
| if err != nil { | |
| return fmt.Errorf("failed to get rhcos architecture: %w", err) | |
| } | |
| azureDisk := streamArch.RHELCoreOSExtensions.AzureDisk | |
| imageURL := azureDisk.URL | |
| imageURL := in.RhcosImage.ControlPlane |
There was a problem hiding this comment.
@r4f4 yes ;-) I complicated things. This can all go away.
|
|
||
| imageLength := headResponse.ContentLength | ||
| if imageLength%512 != 0 { | ||
| return fmt.Errorf("image length is not alisnged on a 512 byte boundary") |
There was a problem hiding this comment.
| return fmt.Errorf("image length is not alisnged on a 512 byte boundary") | |
| return fmt.Errorf("image length is not aligned on a 512 byte boundary") |
| galleryGen2ImageName := fmt.Sprintf("%s-gen2", in.InfraID) | ||
| galleryGen2ImageVersionName := imageVersion | ||
|
|
||
| headResponse, err := http.Head(imageURL) // nolint:gosec |
There was a problem hiding this comment.
It'd be nice to add a reason why the linter can be silenced in this case.
There was a problem hiding this comment.
IIRC this was to get the size. Go was complaining. We can ditch all of this.
There was a problem hiding this comment.
This linter report:
pkg/infrastructure/azure/azure.go:573:9: copylocks: Ignition passes lock by value: github.com/openshift/installer/pkg/infrastructure/azure.Provider contains sync.WaitGroup contains sync.noCopy (govet)
func (p Provider) Ignition(ctx context.Context, in clusterapi.IgnitionInput) ([]byte, error) {
should be looked into. Using copy of locks can result in deadlocks.
So total up to pre-machine provisioning:
But overall not much of a difference? We'd need more runs to build a statistically relevant sample. |
|
@r4f4 thanks for the analysis Yes, let's try to build up at least a small little sample. /test e2e-azure-ovn-techpreview Regarding the comments, I generally agree with them. Almost all of that code is simply cut-and-pasted and slightly refactored to be wrapped in goroutines; so I didn't look at it closely. If I do end up pushing this through to completion I will refactor further. 👍 |
|
Also, I was trying to dig up some logs of a CAPI install with marketplace images and it does look like a marked improvement. |
|
|
/test e2e-azure-ovn-techpreview |
|
openshift-merge-robot
added
the
needs-rebase
|
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-sigs/prow repository. |
|
@patrickdillon I like this. Any particular reason it's still WIP? |
|
/uncc |
I would want to look a bit more at the error handling before moving forward with this. In general, though, we decided it would be better to just pursue azure marketplace images, with their dramatic improvement in time than this. Also I think we would want to refactor quite a bit in terms of encapsulating each resource into a function to increase legibility. |
|
@patrickdillon: The following tests failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
We're still working on getting free marketplace images available, so let's see if we can improve managed image creation by making the creation async and run in the background while creating the non-machine resources.