Skip to content
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

replace individual endpoint_cnt read from store with 1 bulk read #1632

Merged
merged 2 commits into from
Feb 2, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 36 additions & 6 deletions datastore/datastore.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ type DataStore interface {
// key. The caller must pass a KVObject of the same type as
// the objects that need to be listed
List(string, KVObject) ([]KVObject, error)
// Map returns a Map of KVObjects
Map(key string, kvObject KVObject) (map[string]KVObject, error)
// Scope returns the scope of the store
Scope() string
// KVStore returns access to the KV Store
Expand Down Expand Up @@ -512,40 +514,68 @@ func (ds *datastore) List(key string, kvObject KVObject) ([]KVObject, error) {
return ds.cache.list(kvObject)
}

var kvol []KVObject
cb := func(key string, val KVObject) {
kvol = append(kvol, val)
}
err := ds.iterateKVPairsFromStore(key, kvObject, cb)
if err != nil {
return nil, err
}
return kvol, nil
}

func (ds *datastore) iterateKVPairsFromStore(key string, kvObject KVObject, callback func(string, KVObject)) error {
// Bail out right away if the kvObject does not implement KVConstructor
ctor, ok := kvObject.(KVConstructor)
if !ok {
return nil, fmt.Errorf("error listing objects, object does not implement KVConstructor interface")
return fmt.Errorf("error listing objects, object does not implement KVConstructor interface")
}

// Make sure the parent key exists
if err := ds.ensureParent(key); err != nil {
return nil, err
return err
}

kvList, err := ds.store.List(key)
if err != nil {
return nil, err
return err
}

var kvol []KVObject
for _, kvPair := range kvList {
if len(kvPair.Value) == 0 {
continue
}

dstO := ctor.New()
if err := dstO.SetValue(kvPair.Value); err != nil {
return nil, err
return err
}

// Make sure the object has a correct view of the DB index in
// case we need to modify it and update the DB.
dstO.SetIndex(kvPair.LastIndex)
callback(kvPair.Key, dstO)
}

return nil
}

kvol = append(kvol, dstO)
func (ds *datastore) Map(key string, kvObject KVObject) (map[string]KVObject, error) {
if ds.sequential {
ds.Lock()
defer ds.Unlock()
}

kvol := make(map[string]KVObject)
cb := func(key string, val KVObject) {
// Trim the leading & trailing "/" to make it consistent across all stores
kvol[strings.Trim(key, "/")] = val
}
err := ds.iterateKVPairsFromStore(key, kvObject, cb)
if err != nil {
return nil, err
}
return kvol, nil
}

Expand Down
3 changes: 3 additions & 0 deletions endpoint_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,9 @@ type tableEntry struct {
}

func (ep *endpoint) Info() EndpointInfo {
if ep.sandboxID != "" {
return ep
}
n, err := ep.getNetworkFromStore()
if err != nil {
return nil
Expand Down
22 changes: 13 additions & 9 deletions store.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package libnetwork

import (
"fmt"
"strings"

"github.com/Sirupsen/logrus"
"github.com/docker/libkv/store/boltdb"
Expand Down Expand Up @@ -152,21 +153,24 @@ func (c *controller) getNetworksFromStore() ([]*network, error) {
continue
}

kvep, err := store.Map(datastore.Key(epCntKeyPrefix), &endpointCnt{})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the diffs, I am realizing there was no need for getNetworks() to reconstruct the epCount for each pulled network from store. Whatever code that makes a decision about the epCount, it does pull the latest epCount from store itself. There is no need to retrieve and construct the internal epCounts in the networks retrieved as list.

My advice is to simply remove the existing code which pulls the epCount in the loop below.
It is not needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Goal now is to improve the performance of current code in respect to the getNetworksFromStore().
Given getNetworksFromStore() retrieves the list of networks in a batch, it makes a lot of sense to retrieve the epCnt as a batch along it.

We can address the code removal I was suggesting in a subsequent PR outside of the release cycle.

if err != nil {
if err != datastore.ErrKeyNotFound {
logrus.Warnf("failed to get endpoint_count map for scope %s: %v", store.Scope(), err)
}
}

for _, kvo := range kvol {
n := kvo.(*network)
n.Lock()
n.ctrlr = c
n.Unlock()

ec := &endpointCnt{n: n}
err = store.GetObject(datastore.Key(ec.Key()...), ec)
if err != nil && !n.inDelete {
logrus.Warnf("could not find endpoint count key %s for network %s while listing: %v", datastore.Key(ec.Key()...), n.Name(), err)
continue
// Trim the leading & trailing "/" to make it consistent across all stores
if val, ok := kvep[strings.Trim(datastore.Key(ec.Key()...), "/")]; ok {
ec = val.(*endpointCnt)
ec.n = n
n.epCnt = ec
}

n.Lock()
n.epCnt = ec
n.scope = store.Scope()
n.Unlock()
nl = append(nl, n)
Expand Down