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.

Sign up for GitHub

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

Jump to bottom

[specific ci=Group10-VCH-Restart] Optimize portlayer volume cache rebuild on startup #7267

Merged
merged 1 commit into from
Feb 13, 2018
Merged

[specific ci=Group10-VCH-Restart] Optimize portlayer volume cache rebuild on startup #7267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 18 additions & 20 deletions lib/portlayer/storage/volume_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2016-2017 VMware, Inc. All Rights Reserved.
// Copyright 2016-2018 VMware, Inc. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -59,7 +59,7 @@ func (v *VolumeLookupCache) GetVolumeStore(op trace.Operation, storeName string)
return u, nil
}

// AddStore adds a volumestore by name. The url returned is the service url to the volume store.
// AddStore adds a volumestore by name. The url returned is the service url to the volume store.
func (v *VolumeLookupCache) AddStore(op trace.Operation, storeName string, vs VolumeStorer) (*url.URL, error) {
v.vlcLock.Lock()
defer v.vlcLock.Unlock()
Expand All @@ -70,12 +70,13 @@ func (v *VolumeLookupCache) AddStore(op trace.Operation, storeName string, vs Vo
return nil, err
}

if _, ok := v.volumeStores[u.String()]; ok {
return nil, fmt.Errorf("volumestore (%s) already added", u.String())
storeURLStr := u.String()
if _, ok := v.volumeStores[storeURLStr]; ok {
return nil, fmt.Errorf("volumestore (%s) already added", storeURLStr)
}

v.volumeStores[u.String()] = vs
return u, v.rebuildCache(op)
v.volumeStores[storeURLStr] = vs
return u, v.addVolumesToCache(op, storeURLStr, vs)
}

func (v *VolumeLookupCache) volumeStore(store *url.URL) (VolumeStorer, error) {
Expand All @@ -94,7 +95,6 @@ func (v *VolumeLookupCache) volumeStore(store *url.URL) (VolumeStorer, error) {

// VolumeStoresList returns a list of volume store names
func (v *VolumeLookupCache) VolumeStoresList(op trace.Operation) ([]string, error) {

v.vlcLock.RLock()
defer v.vlcLock.RUnlock()

Expand Down Expand Up @@ -201,21 +201,19 @@ func (v *VolumeLookupCache) VolumesList(op trace.Operation) ([]*Volume, error) {
return l, nil
}

// goto the volume store and repopulate the cache.
func (v *VolumeLookupCache) rebuildCache(op trace.Operation) error {
op.Infof("Refreshing volume cache.")
// addVolumesToCache finds the volumes in the input volume store and adds them to the cache.
func (v *VolumeLookupCache) addVolumesToCache(op trace.Operation, storeURLStr string, vs VolumeStorer) error {
op.Infof("Adding volumes in volume store %s to volume cache", storeURLStr)

for _, vs := range v.volumeStores {
vols, err := vs.VolumesList(op)
if err != nil {
return err
}
vols, err := vs.VolumesList(op)
if err != nil {
return err
}

for _, vol := range vols {
log.Infof("Volumestore: Found vol %s on store %s.", vol.ID, vol.Store)
// Add it to the cache.
v.vlc[vol.ID] = *vol
}
for _, vol := range vols {
log.Infof("Volumestore: Found vol %s on store %s", vol.ID, vol.Store)
// Add it to the cache.
v.vlc[vol.ID] = *vol
}

return nil
Expand Down
67 changes: 51 additions & 16 deletions lib/portlayer/storage/volume_cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2016-2017 VMware, Inc. All Rights Reserved.
// Copyright 2016-2018 VMware, Inc. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -95,7 +95,7 @@ func (m *MockVolumeStore) VolumeDestroy(op trace.Operation, vol *Volume) error {
return nil
}

// Lists all volumes on the given volume store`
// VolumesList lists all volumes on the given volume store.
func (m *MockVolumeStore) VolumesList(op trace.Operation) ([]*Volume, error) {
var i int
list := make([]*Volume, len(m.db))
Expand Down Expand Up @@ -214,6 +214,52 @@ func TestVolumeCreateGetListAndDelete(t *testing.T) {
}
}

// createVolumes is a test helper that creates a set of num volumes on the input volume cache and volume store.
func createVolumes(t *testing.T, op trace.Operation, v *VolumeLookupCache, storeURL *url.URL, num int) map[string]*Volume {
vols := make(map[string]*Volume)
for i := 1; i <= num; i++ {
id := fmt.Sprintf("ID-%d", i)

// Write to the datastore
vol, err := v.VolumeCreate(op, id, storeURL, 0, nil)
if !assert.NoError(t, err) || !assert.NotNil(t, vol) {
return nil
}

vols[id] = vol
}

return vols
}

func TestAddVolumesToCache(t *testing.T) {
mvs1 := NewMockVolumeStore()
op := trace.NewOperation(context.Background(), "test")
v := NewVolumeLookupCache(op)

storeURL, err := util.VolumeStoreNameToURL("testStore")
assert.NotNil(t, storeURL)
storeURLStr := storeURL.String()
v.volumeStores[storeURLStr] = mvs1

// Create 50 volumes on the volume store.
vols := createVolumes(t, op, v, storeURL, 50)

// Clear the volume map after it has been filled during volume creation.
v.vlc = make(map[string]Volume)

err = v.addVolumesToCache(op, storeURLStr, mvs1)
assert.Nil(t, err)

// Check that the volume map is intact again in the cache.
for _, expectedVol := range vols {
vol, err := v.VolumeGet(op, expectedVol.ID)
if !assert.NoError(t, err) || !assert.Equal(t, expectedVol, vol) {
Copy link
Member

Choose a reason for hiding this comment

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

No need to change it for this PR, but be aware that there is a require package that has the same set of tests as assert but will immediately fail if the test fails instead of marking the test as failed but continuing the logic.
It can make the code much more declarative as you do not have to test the return value of the assert calls and handle it.

return
}
}
}

// Create 2 store caches but use the same backing datastore. Create images
// with the first cache, then get the image with the second. This simulates
// restart since the second cache is empty and has to go to the backing store.
Expand All @@ -227,26 +273,15 @@ func TestVolumeCacheRestart(t *testing.T) {
return
}

// Create a set of volumes
inVols := make(map[string]*Volume)
for i := 1; i < 50; i++ {
id := fmt.Sprintf("ID-%d", i)

// Write to the datastore
vol, err := firstCache.VolumeCreate(op, id, storeURL, 0, nil)
if !assert.NoError(t, err) || !assert.NotNil(t, vol) {
return
}

inVols[id] = vol
}
// Create a set of 50 volumes.
inVols := createVolumes(t, op, firstCache, storeURL, 50)

secondCache := NewVolumeLookupCache(op)
if !assert.NotNil(t, secondCache) {
return
}

_, err = secondCache.AddStore(op, "testStore", mvs)
storeURL, err = secondCache.AddStore(op, "testStore", mvs)
if !assert.NoError(t, err) || !assert.NotNil(t, storeURL) {
return
}
Expand Down