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

Fix go lint failures in volume scheduling packages #77442

Merged
merged 2 commits into from
May 7, 2019
Merged

Fix go lint failures in volume scheduling packages #77442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3315474
Fix go lint failures in a few packages
cofyc May 4, 2019
4abd730
Do one more level of casting to get the 'assumeCache'
cofyc May 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions hack/.golint_failures
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,6 @@ pkg/controller/volume/expand
pkg/controller/volume/persistentvolume
pkg/controller/volume/persistentvolume/config/v1alpha1
pkg/controller/volume/persistentvolume/options
pkg/controller/volume/persistentvolume/testing
pkg/controller/volume/scheduling
pkg/credentialprovider
pkg/credentialprovider/gcp
pkg/features
Expand Down
6 changes: 3 additions & 3 deletions pkg/controller/volume/persistentvolume/framework_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,7 @@ func runMultisyncTests(t *testing.T, tests []controllerTest, storageClasses []*s
obj := reactor.PopChange()
if obj == nil {
// Nothing was changed, should we exit?
if firstSync || reactor.ChangedSinceLastSync() > 0 {
if firstSync || reactor.GetChangeCount() > 0 {
// There were some changes after the last "periodic sync".
// Simulate "periodic sync" of everything (until it produces
// no changes).
Expand All @@ -711,7 +711,7 @@ func runMultisyncTests(t *testing.T, tests []controllerTest, storageClasses []*s
ctrl.claims.Update(claim)
err = ctrl.syncClaim(claim)
if err != nil {
if err == pvtesting.VersionConflictError {
if err == pvtesting.ErrVersionConflict {
// Ignore version errors
klog.V(4).Infof("test intentionaly ignores version error.")
} else {
Expand All @@ -728,7 +728,7 @@ func runMultisyncTests(t *testing.T, tests []controllerTest, storageClasses []*s
ctrl.volumes.store.Update(volume)
err = ctrl.syncVolume(volume)
if err != nil {
if err == pvtesting.VersionConflictError {
if err == pvtesting.ErrVersionConflict {
// Ignore version errors
klog.V(4).Infof("test intentionaly ignores version error.")
} else {
Expand Down
39 changes: 20 additions & 19 deletions pkg/controller/volume/persistentvolume/testing/testing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ import (
"k8s.io/kubernetes/pkg/features"
)

var VersionConflictError = errors.New("VersionError")
// ErrVersionConflict is the error returned when resource version of requested
// object conflicts with the object in storage.
var ErrVersionConflict = errors.New("VersionError")
Copy link
Member Author

Choose a reason for hiding this comment

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

golint: error var VersionConflictError should have name of the form ErrFoo


// VolumeReactor is a core.Reactor that simulates etcd and API server. It
// stores:
Expand Down Expand Up @@ -157,7 +159,7 @@ func (r *VolumeReactor) React(action core.Action) (handled bool, ret runtime.Obj
storedVer, _ := strconv.Atoi(storedVolume.ResourceVersion)
requestedVer, _ := strconv.Atoi(volume.ResourceVersion)
if storedVer != requestedVer {
return true, obj, VersionConflictError
return true, obj, ErrVersionConflict
}
if reflect.DeepEqual(storedVolume, volume) {
klog.V(4).Infof("nothing updated volume %s", volume.Name)
Expand Down Expand Up @@ -190,7 +192,7 @@ func (r *VolumeReactor) React(action core.Action) (handled bool, ret runtime.Obj
storedVer, _ := strconv.Atoi(storedClaim.ResourceVersion)
requestedVer, _ := strconv.Atoi(claim.ResourceVersion)
if storedVer != requestedVer {
return true, obj, VersionConflictError
return true, obj, ErrVersionConflict
}
if reflect.DeepEqual(storedClaim, claim) {
klog.V(4).Infof("nothing updated claim %s", claim.Name)
Expand Down Expand Up @@ -219,21 +221,19 @@ func (r *VolumeReactor) React(action core.Action) (handled bool, ret runtime.Obj
if found {
klog.V(4).Infof("GetVolume: found %s", volume.Name)
return true, volume.DeepCopy(), nil
} else {
klog.V(4).Infof("GetVolume: volume %s not found", name)
return true, nil, fmt.Errorf("Cannot find volume %s", name)
}
klog.V(4).Infof("GetVolume: volume %s not found", name)
Copy link
Member Author

Choose a reason for hiding this comment

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

if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
it applies to all if else cases

return true, nil, fmt.Errorf("Cannot find volume %s", name)

case action.Matches("get", "persistentvolumeclaims"):
name := action.(core.GetAction).GetName()
claim, found := r.claims[name]
if found {
klog.V(4).Infof("GetClaim: found %s", claim.Name)
return true, claim.DeepCopy(), nil
} else {
klog.V(4).Infof("GetClaim: claim %s not found", name)
return true, nil, apierrs.NewNotFound(action.GetResource().GroupResource(), name)
}
klog.V(4).Infof("GetClaim: claim %s not found", name)
return true, nil, apierrs.NewNotFound(action.GetResource().GroupResource(), name)

case action.Matches("delete", "persistentvolumes"):
name := action.(core.DeleteAction).GetName()
Expand All @@ -246,9 +246,8 @@ func (r *VolumeReactor) React(action core.Action) (handled bool, ret runtime.Obj
}
r.changedSinceLastSync++
return true, nil, nil
} else {
return true, nil, fmt.Errorf("Cannot delete volume %s: not found", name)
}
return true, nil, fmt.Errorf("Cannot delete volume %s: not found", name)

case action.Matches("delete", "persistentvolumeclaims"):
name := action.(core.DeleteAction).GetName()
Expand All @@ -261,9 +260,8 @@ func (r *VolumeReactor) React(action core.Action) (handled bool, ret runtime.Obj
}
r.changedSinceLastSync++
return true, nil, nil
} else {
return true, nil, fmt.Errorf("Cannot delete claim %s: not found", name)
}
return true, nil, fmt.Errorf("Cannot delete claim %s: not found", name)
}

return false, nil, nil
Expand Down Expand Up @@ -299,12 +297,6 @@ func (r *VolumeReactor) getWatches(gvr schema.GroupVersionResource, ns string) [
return watches
}

func (r *VolumeReactor) ChangedSinceLastSync() int {
Copy link
Member Author

Choose a reason for hiding this comment

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

Duplicate function

r.lock.RLock()
defer r.lock.RUnlock()
return r.changedSinceLastSync
}

// injectReactError returns an error when the test requested given action to
// fail. nil is returned otherwise.
func (r *VolumeReactor) injectReactError(action core.Action) error {
Expand Down Expand Up @@ -435,6 +427,7 @@ func (r *VolumeReactor) SyncAll() {
r.changedSinceLastSync = 0
}

// GetChangeCount returns changes since last sync.
func (r *VolumeReactor) GetChangeCount() int {
r.lock.Lock()
defer r.lock.Unlock()
Expand Down Expand Up @@ -515,6 +508,7 @@ func (r *VolumeReactor) AddClaimEvent(claim *v1.PersistentVolumeClaim) {
}
}

// AddClaims adds PVCs into VolumeReactor.
func (r *VolumeReactor) AddClaims(claims []*v1.PersistentVolumeClaim) {
r.lock.Lock()
defer r.lock.Unlock()
Expand All @@ -523,6 +517,7 @@ func (r *VolumeReactor) AddClaims(claims []*v1.PersistentVolumeClaim) {
}
}

// AddVolumes adds PVs into VolumeReactor.
func (r *VolumeReactor) AddVolumes(volumes []*v1.PersistentVolume) {
r.lock.Lock()
defer r.lock.Unlock()
Expand All @@ -531,24 +526,28 @@ func (r *VolumeReactor) AddVolumes(volumes []*v1.PersistentVolume) {
}
}

// AddClaim adds a PVC into VolumeReactor.
func (r *VolumeReactor) AddClaim(claim *v1.PersistentVolumeClaim) {
r.lock.Lock()
defer r.lock.Unlock()
r.claims[claim.Name] = claim
}

// AddVolume adds a PV into VolumeReactor.
func (r *VolumeReactor) AddVolume(volume *v1.PersistentVolume) {
r.lock.Lock()
defer r.lock.Unlock()
r.volumes[volume.Name] = volume
}

// DeleteVolume deletes a PV by name.
func (r *VolumeReactor) DeleteVolume(name string) {
r.lock.Lock()
defer r.lock.Unlock()
delete(r.volumes, name)
}

// AddClaimBoundToVolume adds a PVC and binds it to corresponding PV.
func (r *VolumeReactor) AddClaimBoundToVolume(claim *v1.PersistentVolumeClaim) {
r.lock.Lock()
defer r.lock.Unlock()
Expand All @@ -558,6 +557,7 @@ func (r *VolumeReactor) AddClaimBoundToVolume(claim *v1.PersistentVolumeClaim) {
}
}

// MarkVolumeAvaiable marks a PV available by name.
func (r *VolumeReactor) MarkVolumeAvaiable(name string) {
r.lock.Lock()
defer r.lock.Unlock()
Expand All @@ -568,6 +568,7 @@ func (r *VolumeReactor) MarkVolumeAvaiable(name string) {
}
}

// NewVolumeReactor creates a volume reactor.
func NewVolumeReactor(client *fake.Clientset, fakeVolumeWatch, fakeClaimWatch *watch.FakeWatcher, errors []ReactorError) *VolumeReactor {
reactor := &VolumeReactor{
volumes: make(map[string]*v1.PersistentVolume),
Expand Down
13 changes: 8 additions & 5 deletions pkg/controller/volume/scheduling/scheduler_assume_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ func (c *assumeCache) objInfoIndexFunc(obj interface{}) ([]string, error) {
return c.indexFunc(objInfo.latestObj)
}

func NewAssumeCache(informer cache.SharedIndexInformer, description, indexName string, indexFunc cache.IndexFunc) *assumeCache {
// NewAssumeCache creates an assume cache for genernal objects.
func NewAssumeCache(informer cache.SharedIndexInformer, description, indexName string, indexFunc cache.IndexFunc) AssumeCache {
Copy link
Member Author

@cofyc cofyc May 4, 2019 •
edited

Choose a reason for hiding this comment

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

exported func NewAssumeCache returns unexported type *scheduling.assumeCache, which can be annoying to use
I guess it's better to expose interface instead struct, so I changed the function to return interface

c := &assumeCache{
description: description,
indexFunc: indexFunc,
Expand Down Expand Up @@ -344,7 +345,7 @@ type PVAssumeCache interface {
}

type pvAssumeCache struct {
*assumeCache
AssumeCache
}

func pvStorageClassIndexFunc(obj interface{}) ([]string, error) {
Expand All @@ -354,8 +355,9 @@ func pvStorageClassIndexFunc(obj interface{}) ([]string, error) {
return []string{""}, fmt.Errorf("object is not a v1.PersistentVolume: %v", obj)
}

// NewPVAssumeCache creates a PV assume cache.
func NewPVAssumeCache(informer cache.SharedIndexInformer) PVAssumeCache {
return &pvAssumeCache{assumeCache: NewAssumeCache(informer, "v1.PersistentVolume", "storageclass", pvStorageClassIndexFunc)}
return &pvAssumeCache{NewAssumeCache(informer, "v1.PersistentVolume", "storageclass", pvStorageClassIndexFunc)}
}

func (c *pvAssumeCache) GetPV(pvName string) (*v1.PersistentVolume, error) {
Expand Down Expand Up @@ -411,11 +413,12 @@ type PVCAssumeCache interface {
}

type pvcAssumeCache struct {
*assumeCache
AssumeCache
}

// NewPVCAssumeCache creates a PVC assume cache.
func NewPVCAssumeCache(informer cache.SharedIndexInformer) PVCAssumeCache {
return &pvcAssumeCache{assumeCache: NewAssumeCache(informer, "v1.PersistentVolumeClaim", "namespace", cache.MetaNamespaceIndexFunc)}
return &pvcAssumeCache{NewAssumeCache(informer, "v1.PersistentVolumeClaim", "namespace", cache.MetaNamespaceIndexFunc)}
}

func (c *pvcAssumeCache) GetPVC(pvcKey string) (*v1.PersistentVolumeClaim, error) {
Expand Down