Skip to content

Commit

Permalink
installer/pkg/config/libvirt: Caching for libvirt pulls
Browse files Browse the repository at this point in the history
Checking our RHCOS source against ETag [1] / If-None-Match [2] and
Last-Modified [3] / If-Modified-Since [4], it seems to support
In-None-Match well, but only supports If-Modified-Since for exact
matches:

  $ URL=http://aos-ostree.rhev-ci-vms.eng.rdu2.redhat.com/rhcos/images/cloud/latest/rhcos-qemu.qcow2.gz
  $ curl -I "${URL}"
  HTTP/1.1 200 OK
  Server: nginx/1.8.0
  Date: Wed, 19 Sep 2018 04:32:19 GMT
  Content-Type: application/octet-stream
  Content-Length: 684934062
  Last-Modified: Tue, 18 Sep 2018 20:05:24 GMT
  Connection: keep-alive
  ETag: "5ba15a84-28d343ae"
  Accept-Ranges: bytes
  $ curl -sIH 'If-None-Match: "5ba15a84-28d343ae"' "${URL}" | head -n1
  HTTP/1.1 304 Not Modified
  $ curl -sIH 'If-Modified-Since: Tue, 18 Sep 2018 20:05:24 GMT' "${URL}" | head -n1
  HTTP/1.1 304 Not Modified
  $ curl -sIH 'If-Modified-Since: Tue, 18 Sep 2015 20:05:24 GMT' "${URL}" | head -n1
  HTTP/1.1 200 OK
  $ curl -sIH 'If-Modified-Since: Tue, 18 Sep 2018 20:05:25 GMT' "${URL}" | grep 'HTTP\|Last-Modified'
  HTTP/1.1 200 OK
  Last-Modified: Tue, 18 Sep 2018 20:05:24 GMT

That last entry should have 304ed, although the spec has [4]:

  When used for cache updates, a cache will typically use the value of
  the cached message's Last-Modified field to generate the field value
  of If-Modified-Since.  This behavior is most interoperable for cases
  where clocks are poorly synchronized or when the server has chosen
  to only honor exact timestamp matches (due to a problem with
  Last-Modified dates that appear to go "back in time" when the origin
  server's clock is corrected or a representation is restored from an
  archived backup).  However, caches occasionally generate the field
  value based on other data, such as the Date header field of the
  cached message or the local clock time that the message was
  received, particularly when the cached message does not contain a
  Last-Modified field.
  ...
  When used for cache updates, a cache will typically use the value of
  the cached message's Last-Modified field to generate the field value
  of If-Modified-Since.  This behavior is most interoperable for cases
  where clocks are poorly synchronized or when the server has chosen
  to only honor exact timestamp matches (due to a problem with
  Last-Modified dates that appear to go "back in time" when the origin
  server's clock is corrected or a representation is restored from an
  archived backup).  However, caches occasionally generate the field
  value based on other data, such as the Date header field of the
  cached message or the local clock time that the message was
  received, particularly when the cached message does not contain a
  Last-Modified field.

So the server is violating the SHOULD by not 304ing dates greater than
Last-Modified, but it's not violating a MUST-level requirement.  The
server requirements around If-None-Match are MUST-level [2], so using
it should be more portable.  The RFC also seems to prefer clients use
If(-None)-Match [4,5].

I'm using gregjones/httpcache for the caching, since that
implemenation seems reasonably popular and the repo's been around for
a few years.  That library uses the belt-and-suspenders approach of
setting both If-None-Match (to the cached ETag) and If-Modified-Since
(to the cached Last-Modified) [6], so we should be fine.

UserCacheDir requires Go 1.11 [7,8,9].

The BUILD.bazel changes were generated with:

  $ bazel run //:gazelle

using:

  $ bazel version
  Build label: 0.16.1- (@non-git)
  Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
  Build time: Mon Aug 13 16:42:29 2018 (1534178549)
  Build timestamp: 1534178549
  Build timestamp as int: 1534178549

[1]: https://tools.ietf.org/html/rfc7232#section-2.3
[2]: https://tools.ietf.org/html/rfc7232#section-3.2
[3]: https://tools.ietf.org/html/rfc7232#section-2.2
[4]: https://tools.ietf.org/html/rfc7232#section-3.3
[5]: https://tools.ietf.org/html/rfc7232#section-2.4
[6]: https://github.com/gregjones/httpcache/blob/9cad4c3443a7200dd6400aef47183728de563a38/httpcache.go#L169-L181
[7]: golang/go#22536
[8]: golang/go@816154b
[9]: golang/go@50bd1c4
  • Loading branch information
wking committed Sep 19, 2018
1 parent c922030 commit 9ca81bb
Show file tree
Hide file tree
Showing 5 changed files with 89 additions and 5 deletions.
11 changes: 9 additions & 2 deletions installer/pkg/config/libvirt/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,15 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library")

go_library(
name = "go_default_library",
srcs = ["libvirt.go"],
srcs = [
"cache.go",
"libvirt.go",
],
importpath = "github.com/openshift/installer/installer/pkg/config/libvirt",
visibility = ["//visibility:public"],
deps = ["//vendor/github.com/apparentlymart/go-cidr/cidr:go_default_library"],
deps = [
"//vendor/github.com/apparentlymart/go-cidr/cidr:go_default_library",
"//vendor/github.com/gregjones/httpcache:go_default_library",
"//vendor/github.com/gregjones/httpcache/diskcache:go_default_library",
],
)
68 changes: 68 additions & 0 deletions installer/pkg/config/libvirt/cache.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package libvirt

import (
"fmt"
"io"
"io/ioutil"
"os"
"path/filepath"
"strings"

"github.com/gregjones/httpcache"
"github.com/gregjones/httpcache/diskcache"
)

// UseCachedImage leaves non-file:// image URIs unalterered.
// Other URIs are retrieved with a local cache at
// $XDG_CACHE_HOME/openshift-install/libvirt [1]. This allows you to
// use the same remote image URI multiple times without needing to
// worry about redundant downloads, although you will want to
// periodically blow away your cache.
//
// [1]: https://standards.freedesktop.org/basedir-spec/basedir-spec-0.7.html
func (libvirt *Libvirt) UseCachedImage() (err error) {
// FIXME: set the default URI here? Leave it elsewhere?

if strings.HasPrefix(libvirt.Image, "file://") {
return nil
}

baseCacheDir, err := os.UserCacheDir()
if err != nil {
return err
}

cacheDir := filepath.Join(baseCacheDir, "openshift-install", "libvirt")
err = os.MkdirAll(cacheDir, 0777)
if err != nil {
return err
}

cache := diskcache.New(cacheDir)
transport := httpcache.NewTransport(cache)
resp, err := transport.Client().Get(libvirt.Image)
if err != nil {
return err
}
if resp.StatusCode != 200 {
return fmt.Errorf("%s while getting %s", resp.Status, libvirt.Image)
}
defer resp.Body.Close()

// FIXME: diskcache's diskv backend doesn't expose direct file access.
// We can write our own cache implementation to get around this,
// but for now just dump this into /tmp without cleanup.
file, err := ioutil.TempFile("", "openshift-install-")
if err != nil {
return err
}
defer file.Close()

_, err = io.Copy(file, resp.Body)
if err != nil {
return err
}

libvirt.Image = fmt.Sprintf("file://%s", filepath.ToSlash(file.Name()))
return nil
}
1 change: 1 addition & 0 deletions installer/pkg/config/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ func ParseConfig(data []byte) (*Cluster, error) {
return nil, err
}
cluster.PullSecret = string(data)
cluster.PullSecretPath = ""
}

if cluster.EC2AMIOverride == "" {
Expand Down
1 change: 0 additions & 1 deletion installer/pkg/workflow/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ go_library(
deps = [
"//installer/pkg/config:go_default_library",
"//installer/pkg/config-generator:go_default_library",
"//installer/pkg/copy:go_default_library",
"//vendor/github.com/Sirupsen/logrus:go_default_library",
"//vendor/gopkg.in/yaml.v2:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
Expand Down
13 changes: 11 additions & 2 deletions installer/pkg/workflow/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (

"github.com/openshift/installer/installer/pkg/config"
configgenerator "github.com/openshift/installer/installer/pkg/config-generator"
"github.com/openshift/installer/installer/pkg/copy"
)

const (
Expand Down Expand Up @@ -94,6 +93,12 @@ func prepareWorspaceStep(m *metadata) error {
return err
}

if cluster.Platform == config.PlatformLibvirt {
if err := cluster.Libvirt.UseCachedImage(); err != nil {
return err
}
}

// generate clusterDir folder
clusterDir := filepath.Join(dir, cluster.Name)
m.clusterDir = clusterDir
Expand All @@ -107,7 +112,11 @@ func prepareWorspaceStep(m *metadata) error {

// put config file under the clusterDir folder
configFilePath := filepath.Join(clusterDir, configFileName)
if err := copy.Copy(m.configFilePath, configFilePath); err != nil {
configContent, err := yaml.Marshal(cluster)
if err != nil {
return err
}
if err := ioutil.WriteFile(configFilePath, configContent, 0666); err != nil {
return fmt.Errorf("failed to create cluster config at %q: %v", clusterDir, err)
}

Expand Down

0 comments on commit 9ca81bb

Please sign in to comment.