From b6580d6c6500ff8a97f66a503ffa6431f7b5bf87 Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Mon, 5 Mar 2018 15:46:21 -0800 Subject: [PATCH 1/2] Remove metadata store interface Layer metadata storage has not been implemented outside of the layer store and will be deprecated by containerd metadata storage. To prepare for this and freeze the current metadata storage, remove the exported interface and make it internal to the layer store. Signed-off-by: Derek McGowan Upstream-commit: 0aebcbc32dc6a7b35982a2bfc44335562cbb68ec Component: engine --- components/engine/layer/filestore.go | 6 +-- components/engine/layer/filestore_test.go | 4 +- components/engine/layer/layer.go | 48 ----------------------- components/engine/layer/layer_store.go | 24 ++++++------ components/engine/layer/layer_test.go | 9 ++--- components/engine/layer/migration_test.go | 14 ++----- components/engine/layer/ro_layer.go | 2 +- 7 files changed, 26 insertions(+), 81 deletions(-) diff --git a/components/engine/layer/filestore.go b/components/engine/layer/filestore.go index a7454b5e9f..b1cbb80166 100644 --- a/components/engine/layer/filestore.go +++ b/components/engine/layer/filestore.go @@ -37,10 +37,10 @@ type fileMetadataTransaction struct { ws *ioutils.AtomicWriteSet } -// NewFSMetadataStore returns an instance of a metadata store +// newFSMetadataStore returns an instance of a metadata store // which is backed by files on disk using the provided root // as the root of metadata files. -func NewFSMetadataStore(root string) (MetadataStore, error) { +func newFSMetadataStore(root string) (*fileMetadataStore, error) { if err := os.MkdirAll(root, 0700); err != nil { return nil, err } @@ -66,7 +66,7 @@ func (fms *fileMetadataStore) getMountFilename(mount, filename string) string { return filepath.Join(fms.getMountDirectory(mount), filename) } -func (fms *fileMetadataStore) StartTransaction() (MetadataTransaction, error) { +func (fms *fileMetadataStore) StartTransaction() (*fileMetadataTransaction, error) { tmpDir := filepath.Join(fms.root, "tmp") if err := os.MkdirAll(tmpDir, 0755); err != nil { return nil, err diff --git a/components/engine/layer/filestore_test.go b/components/engine/layer/filestore_test.go index 8e6dd27f94..498379e37f 100644 --- a/components/engine/layer/filestore_test.go +++ b/components/engine/layer/filestore_test.go @@ -24,12 +24,12 @@ func newFileMetadataStore(t *testing.T) (*fileMetadataStore, string, func()) { if err != nil { t.Fatal(err) } - fms, err := NewFSMetadataStore(td) + fms, err := newFSMetadataStore(td) if err != nil { t.Fatal(err) } - return fms.(*fileMetadataStore), td, func() { + return fms, td, func() { if err := os.RemoveAll(td); err != nil { t.Logf("Failed to cleanup %q: %s", td, err) } diff --git a/components/engine/layer/layer.go b/components/engine/layer/layer.go index 48fa7cf3c4..d0c7fa8608 100644 --- a/components/engine/layer/layer.go +++ b/components/engine/layer/layer.go @@ -201,54 +201,6 @@ type DescribableStore interface { RegisterWithDescriptor(io.Reader, ChainID, distribution.Descriptor) (Layer, error) } -// MetadataTransaction represents functions for setting layer metadata -// with a single transaction. -type MetadataTransaction interface { - SetSize(int64) error - SetParent(parent ChainID) error - SetDiffID(DiffID) error - SetCacheID(string) error - SetDescriptor(distribution.Descriptor) error - setOS(string) error - TarSplitWriter(compressInput bool) (io.WriteCloser, error) - - Commit(ChainID) error - Cancel() error - String() string -} - -// MetadataStore represents a backend for persisting -// metadata about layers and providing the metadata -// for restoring a Store. -type MetadataStore interface { - // StartTransaction starts an update for new metadata - // which will be used to represent an ID on commit. - StartTransaction() (MetadataTransaction, error) - - GetSize(ChainID) (int64, error) - GetParent(ChainID) (ChainID, error) - GetDiffID(ChainID) (DiffID, error) - GetCacheID(ChainID) (string, error) - GetDescriptor(ChainID) (distribution.Descriptor, error) - getOS(ChainID) (string, error) - TarSplitReader(ChainID) (io.ReadCloser, error) - - SetMountID(string, string) error - SetInitID(string, string) error - SetMountParent(string, ChainID) error - - GetMountID(string) (string, error) - GetInitID(string) (string, error) - GetMountParent(string) (ChainID, error) - - // List returns the full list of referenced - // read-only and read-write layers - List() ([]ChainID, []string, error) - - Remove(ChainID) error - RemoveMount(string) error -} - // CreateChainID returns ID for a layerDigest slice func CreateChainID(dgsts []DiffID) ChainID { return createChainIDFromParent("", dgsts...) diff --git a/components/engine/layer/layer_store.go b/components/engine/layer/layer_store.go index ec8ecd6c17..bf0705afc5 100644 --- a/components/engine/layer/layer_store.go +++ b/components/engine/layer/layer_store.go @@ -27,7 +27,7 @@ import ( const maxLayerDepth = 125 type layerStore struct { - store MetadataStore + store *fileMetadataStore driver graphdriver.Driver useTarSplit bool @@ -65,18 +65,15 @@ func NewStoreFromOptions(options StoreOptions) (Store, error) { } logrus.Debugf("Initialized graph driver %s", driver) - fms, err := NewFSMetadataStore(fmt.Sprintf(options.MetadataStorePathTemplate, driver)) - if err != nil { - return nil, err - } + root := fmt.Sprintf(options.MetadataStorePathTemplate, driver) - return NewStoreFromGraphDriver(fms, driver, options.OS) + return newStoreFromGraphDriver(root, driver, options.OS) } -// NewStoreFromGraphDriver creates a new Store instance using the provided +// newStoreFromGraphDriver creates a new Store instance using the provided // metadata store and graph driver. The metadata store will be used to restore // the Store. -func NewStoreFromGraphDriver(store MetadataStore, driver graphdriver.Driver, os string) (Store, error) { +func newStoreFromGraphDriver(root string, driver graphdriver.Driver, os string) (Store, error) { if !system.IsOSSupported(os) { return nil, fmt.Errorf("failed to initialize layer store as operating system '%s' is not supported", os) } @@ -85,8 +82,13 @@ func NewStoreFromGraphDriver(store MetadataStore, driver graphdriver.Driver, os caps = capDriver.Capabilities() } + ms, err := newFSMetadataStore(root) + if err != nil { + return nil, err + } + ls := &layerStore{ - store: store, + store: ms, driver: driver, layerMap: map[ChainID]*roLayer{}, mounts: map[string]*mountedLayer{}, @@ -94,7 +96,7 @@ func NewStoreFromGraphDriver(store MetadataStore, driver graphdriver.Driver, os os: os, } - ids, mounts, err := store.List() + ids, mounts, err := ms.List() if err != nil { return nil, err } @@ -225,7 +227,7 @@ func (ls *layerStore) loadMount(mount string) error { return nil } -func (ls *layerStore) applyTar(tx MetadataTransaction, ts io.Reader, parent string, layer *roLayer) error { +func (ls *layerStore) applyTar(tx *fileMetadataTransaction, ts io.Reader, parent string, layer *roLayer) error { digester := digest.Canonical.Digester() tr := io.TeeReader(ts, digester.Hash()) diff --git a/components/engine/layer/layer_test.go b/components/engine/layer/layer_test.go index 0bf25bc72b..5c4e8fab19 100644 --- a/components/engine/layer/layer_test.go +++ b/components/engine/layer/layer_test.go @@ -69,11 +69,8 @@ func newTestStore(t *testing.T) (Store, string, func()) { } graph, graphcleanup := newTestGraphDriver(t) - fms, err := NewFSMetadataStore(td) - if err != nil { - t.Fatal(err) - } - ls, err := NewStoreFromGraphDriver(fms, graph, runtime.GOOS) + + ls, err := newStoreFromGraphDriver(td, graph, runtime.GOOS) if err != nil { t.Fatal(err) } @@ -403,7 +400,7 @@ func TestStoreRestore(t *testing.T) { t.Fatal(err) } - ls2, err := NewStoreFromGraphDriver(ls.(*layerStore).store, ls.(*layerStore).driver, runtime.GOOS) + ls2, err := newStoreFromGraphDriver(ls.(*layerStore).store.root, ls.(*layerStore).driver, runtime.GOOS) if err != nil { t.Fatal(err) } diff --git a/components/engine/layer/migration_test.go b/components/engine/layer/migration_test.go index 2c6800f151..923166371c 100644 --- a/components/engine/layer/migration_test.go +++ b/components/engine/layer/migration_test.go @@ -90,11 +90,8 @@ func TestLayerMigration(t *testing.T) { t.Fatal(err) } - fms, err := NewFSMetadataStore(filepath.Join(td, "layers")) - if err != nil { - t.Fatal(err) - } - ls, err := NewStoreFromGraphDriver(fms, graph, runtime.GOOS) + root := filepath.Join(td, "layers") + ls, err := newStoreFromGraphDriver(root, graph, runtime.GOOS) if err != nil { t.Fatal(err) } @@ -218,11 +215,8 @@ func TestLayerMigrationNoTarsplit(t *testing.T) { t.Fatal(err) } - fms, err := NewFSMetadataStore(filepath.Join(td, "layers")) - if err != nil { - t.Fatal(err) - } - ls, err := NewStoreFromGraphDriver(fms, graph, runtime.GOOS) + root := filepath.Join(td, "layers") + ls, err := newStoreFromGraphDriver(root, graph, runtime.GOOS) if err != nil { t.Fatal(err) } diff --git a/components/engine/layer/ro_layer.go b/components/engine/layer/ro_layer.go index e1f1b407c2..bc0fe1dddf 100644 --- a/components/engine/layer/ro_layer.go +++ b/components/engine/layer/ro_layer.go @@ -121,7 +121,7 @@ func (rl *roLayer) depth() int { return rl.parent.depth() + 1 } -func storeLayer(tx MetadataTransaction, layer *roLayer) error { +func storeLayer(tx *fileMetadataTransaction, layer *roLayer) error { if err := tx.SetDiffID(layer.diffID); err != nil { return err } From aa0ca25049778f92f08bc60726f2d7680f140fd1 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Wed, 7 Mar 2018 13:20:21 -0800 Subject: [PATCH 2/2] daemon/stats: more resilient cpu sampling To avoid noise in sampling CPU usage metrics, we now sample the system usage closer to the actual response from the underlying runtime. Because the response from the runtime may be delayed, this makes the sampling more resilient in loaded conditions. In addition to this, we also replace the tick with a sleep to avoid situations where ticks can backup under loaded conditions. The trade off here is slightly more load reading the system CPU usage for each container. There may be an optimization required for large amounts of containers but the cost is on the order of 15 ms per 1000 containers. If this becomes a problem, we can time slot the sampling, but the complexity may not be worth it unless we can test further. Unfortunately, there aren't really any good tests for this condition. Triggering this behavior is highly system dependent. As a matter of course, we should qualify the fix with the users that are affected. Signed-off-by: Stephen J Day Upstream-commit: fd0e24b7189374e0fe7c55b6d26ee916d3ee1655 Component: engine --- components/engine/daemon/stats/collector.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/components/engine/daemon/stats/collector.go b/components/engine/daemon/stats/collector.go index 39c76128b0..24d41a3d2e 100644 --- a/components/engine/daemon/stats/collector.go +++ b/components/engine/daemon/stats/collector.go @@ -57,7 +57,7 @@ func (s *Collector) Run() { // it will grow enough in first iteration var pairs []publishersPair - for range time.Tick(s.interval) { + for { // it does not make sense in the first iteration, // but saves allocations in further iterations pairs = pairs[:0] @@ -72,12 +72,6 @@ func (s *Collector) Run() { continue } - systemUsage, err := s.getSystemCPUUsage() - if err != nil { - logrus.Errorf("collecting system cpu usage: %v", err) - continue - } - onlineCPUs, err := s.getNumberOnlineCPUs() if err != nil { logrus.Errorf("collecting system online cpu count: %v", err) @@ -89,6 +83,14 @@ func (s *Collector) Run() { switch err.(type) { case nil: + // Sample system CPU usage close to container usage to avoid + // noise in metric calculations. + systemUsage, err := s.getSystemCPUUsage() + if err != nil { + logrus.WithError(err).WithField("container_id", pair.container.ID).Errorf("collecting system cpu usage") + continue + } + // FIXME: move to containerd on Linux (not Windows) stats.CPUStats.SystemUsage = systemUsage stats.CPUStats.OnlineCPUs = onlineCPUs @@ -106,6 +108,8 @@ func (s *Collector) Run() { logrus.Errorf("collecting stats for %s: %v", pair.container.ID, err) } } + + time.Sleep(s.interval) } }