Use internal "cache" module to cache libvirt and baremetal images #2595
Conversation
openshift-ci-robot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Fedosin 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-libvirt |
|
/label platform/baremetal |
|
(To run Metal3 CI) |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
pkg/tfvars/internal/cache/cache.go
Outdated
| // then the downloaded data checksum will be compared with the provided value. | ||
| func DownloadFile(baseURL string, dataType string) (string, error) { | ||
| // Send a request | ||
| resp, err := http.Get(baseURL) |
There was a problem hiding this comment.
Is there a way to avoid the HTTP query at all? If we know the SHA, could we look to see if it's already in the cache before making any network connection out? Thinking of some specific scenarios with a disconnected install where we might want to prepopulate the image. This looks like it downloads the whole image each time before populating the cache (i.e. the cache doesn't get used across multiple installer runs)?
There was a problem hiding this comment.
I added a check if a URL starts with "file://" we don't download anything and just reuse the existing file, like it happens now: https://github.com/openshift/installer/blob/master/pkg/tfvars/libvirt/cache.go#L32-L34
Also, we actually don't load any image data and immediately return the path if we found a file with ETag name in the cache. We just make a request and validate the response headers to find ETag there, that's all we do.
There was a problem hiding this comment.
The problem is any outbound query. For many bare metal environments we have a requirement that there can’t be any outbound internet connectivity at all. It’s possible to override the registry for containers, and for the image used by masters (which is different than bootstrap) we have a solution. The only one remaining is we don’t have a good solution that handles the qemu image for bootstrap. That is always an internet url in rhcos.json. Since we know the SHA and that’s definitive anyway, it makes sense to just see if we have a file named that already instead of using ETags at all.
The idea being we populate the cache manually in a disconnected environment.
There was a problem hiding this comment.
Maybe my idea isn’t all that well thought out, I just saw this PR and thought it would be an easy change to just use the SHA all the time. Anyway, I don’t want to hijack your PR and I can always make a proposal separately to handle this case maybe by explicitly overriding the rhcos baseURI
There was a problem hiding this comment.
okay, I got you now... you're gonna laugh, but what you're saying was in my original patch! I calculated a filename corresponding to the url's md5 hash. If it helps you, I can bring that solution back.
There was a problem hiding this comment.
@stbenjam done! now you can calculate image filename in the cache by using echo -n <image_url> | md5sum
|
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1282/ |
Fedosin
force-pushed
the
image_cache
branch
2 times, most recently
from
899a57a to
ea5296c
Compare
|
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1283/ |
|
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1284/ |
|
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1285/ |
|
/unlabel platform/baremetal |
|
Our CI is broken at the moment. Sorry for all the comments from @metal3ci |
|
/test e2e-openstack |
|
/label platform/libvirt |
|
@Fedosin: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
|
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1296/ |
|
will be fixed with #2657 |
|
@abhinavdahiya: Closed this PR. In response to this:
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. |
No description provided.