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

Vsphere: Add 15 missing err checks #73358

Merged
merged 1 commit into from
Jan 29, 2019
Merged

Vsphere: Add 15 missing err checks #73358

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion pkg/cloudprovider/providers/vsphere/credentialmanager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,10 @@ func TestSecretCredentialManager_GetCredential(t *testing.T) {
t.Fatal("Failed to get all secrets from sharedInformer. error: ", err)
}
for _, secret := range secrets {
secretInformer.Informer().GetIndexer().Delete(secret)
err := secretInformer.Informer().GetIndexer().Delete(secret)
if err != nil {
t.Fatalf("Failed to delete secret from informer: %v", err)
}
}
}

Expand Down
3 changes: 1 addition & 2 deletions pkg/cloudprovider/providers/vsphere/nodemanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,7 @@ func (nm *NodeManager) DiscoverNode(node *v1.Node) error {

func (nm *NodeManager) RegisterNode(node *v1.Node) error {
nm.addNode(node)
nm.DiscoverNode(node)
return nil
return nm.DiscoverNode(node)
}

func (nm *NodeManager) UnRegisterNode(node *v1.Node) error {
Expand Down
4 changes: 2 additions & 2 deletions pkg/cloudprovider/providers/vsphere/vclib/connection_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func TestWithValidCaCert(t *testing.T) {
}

// Ignoring error here, because we only care about the TLS connection
connection.NewClient(context.Background())
_, _ = connection.NewClient(context.Background())
Copy link
Member

Choose a reason for hiding this comment

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

does a linter complain about these?
it shouldn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it does, because the error return gets implicitly ignored. With this change, it gets explicitly ignored.

Why do you think it shouldn't complain about these?

Copy link
Member

@neolit123 neolit123 Jan 27, 2019 •
edited

Choose a reason for hiding this comment

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

what is the exact linter message?

just tested with golint locally.
something in the lines of this does not cause an issue:

package main

import "errors"

func test(returnError bool) (string, error) {
    if returnError {
        return "", errors.New("some error")
    }
    return "hello world", nil
}

func main() {
    test(true)
    test(false)
}

Why do you think it shouldn't complain about these?

it depends. the programmer has the freedom to execute the logic without caring about the outcome of function - i.e. in a case where the function also performs extra work unrelated to the returned result. linters in C do complain about these, then again they also complain about unused return values from printf.

Copy link
Member Author

Choose a reason for hiding this comment

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

The linter I used to find these is errcheck: https://github.com/kisielk/errcheck

It basically complains every time a returned error is not explicitly handled, so for your example this is the result:

$ errcheck t.go 
t.go:13:6:	test(true)
t.go:14:6:	test(false)

it depends. the programmer has the freedom to execute the logic without caring about the outcome of function - i.e. in a case where the function also performs extra work unrelated to the returned result

Yes but the returned error may also indicate that during that extra work unrelated to the other returns an error was encountered.
I personally find it a sensible requirement to explicitly ignore all errors, its the only safe way to avoid doing so by accident.

Copy link
Member

@neolit123 neolit123 Jan 27, 2019 •
edited

Choose a reason for hiding this comment

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

since the PR is against k/k, to me it feels like unless ./hack/verify-golint.sh (uses golint) complains about the _, _ = case i don't think we should fix these based on a third party linter.

but the decision is obviously up to the maintainers. 👍

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 doesn't care at all about unhandled errors, this is why the things I fixed here (which are mostly not _ = assignments) got merged in the first place.

I asked about this in #sig-testing and it was agreed that adding linting for this would be a good thing: https://kubernetes.slack.com/archives/C09QZ4DQB/p1548535905927500


verifyConnectionWasMade()
}
Expand Down Expand Up @@ -151,7 +151,7 @@ func TestWithValidThumbprint(t *testing.T) {
}

// Ignoring error here, because we only care about the TLS connection
connection.NewClient(context.Background())
_, _ = connection.NewClient(context.Background())

verifyConnectionWasMade()
}
Expand Down
3 changes: 3 additions & 0 deletions pkg/cloudprovider/providers/vsphere/vclib/datacenter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ func (dc *Datacenter) GetVMByUUID(ctx context.Context, vmUUID string) (*VirtualM
// GetHostByVMUUID gets the host object from the given vmUUID
func (dc *Datacenter) GetHostByVMUUID(ctx context.Context, vmUUID string) (*types.ManagedObjectReference, error) {
virtualMachine, err := dc.GetVMByUUID(ctx, vmUUID)
if err != nil {
return nil, err
}
var vmMo mo.VirtualMachine
pc := property.DefaultCollector(virtualMachine.Client())
err = pc.RetrieveOne(ctx, virtualMachine.Reference(), []string{"summary.runtime.host"}, &vmMo)
Expand Down
29 changes: 23 additions & 6 deletions pkg/cloudprovider/providers/vsphere/vclib/virtualmachine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,10 @@ func (vm *VirtualMachine) AttachDisk(ctx context.Context, vmDiskPath string, vol
RecordvSphereMetric(APIAttachVolume, requestTime, err)
klog.Errorf("Failed to attach the disk with storagePolicy: %q on VM: %q. err - %+v", volumeOptions.StoragePolicyID, vm.InventoryPath, err)
if newSCSIController != nil {
vm.deleteController(ctx, newSCSIController, vmDevices)
nestedErr := vm.deleteController(ctx, newSCSIController, vmDevices)
if nestedErr != nil {
return "", fmt.Errorf("failed to delete SCSI Controller after reconfiguration failed with err=%v: %v", err, nestedErr)
}
}
return "", err
}
Expand All @@ -136,7 +139,10 @@ func (vm *VirtualMachine) AttachDisk(ctx context.Context, vmDiskPath string, vol
if err != nil {
klog.Errorf("Failed to attach the disk with storagePolicy: %+q on VM: %q. err - %+v", volumeOptions.StoragePolicyID, vm.InventoryPath, err)
if newSCSIController != nil {
vm.deleteController(ctx, newSCSIController, vmDevices)
nestedErr := vm.deleteController(ctx, newSCSIController, vmDevices)
if nestedErr != nil {
return "", fmt.Errorf("failed to delete SCSI Controller after waiting for reconfiguration failed with err='%v': %v", err, nestedErr)
}
}
return "", err
}
Expand All @@ -145,9 +151,15 @@ func (vm *VirtualMachine) AttachDisk(ctx context.Context, vmDiskPath string, vol
diskUUID, err := vm.Datacenter.GetVirtualDiskPage83Data(ctx, vmDiskPath)
if err != nil {
klog.Errorf("Error occurred while getting Disk Info from VM: %q. err: %v", vm.InventoryPath, err)
vm.DetachDisk(ctx, vmDiskPath)
nestedErr := vm.DetachDisk(ctx, vmDiskPath)
if nestedErr != nil {
return "", fmt.Errorf("failed to detach disk after getting VM UUID failed with err='%v': %v", err, nestedErr)
}
if newSCSIController != nil {
vm.deleteController(ctx, newSCSIController, vmDevices)
nestedErr = vm.deleteController(ctx, newSCSIController, vmDevices)
if nestedErr != nil {
return "", fmt.Errorf("failed to delete SCSI Controller after getting VM UUID failed with err='%v': %v", err, nestedErr)
}
}
return "", err
}
Expand Down Expand Up @@ -271,7 +283,9 @@ func (vm *VirtualMachine) CreateDiskSpec(ctx context.Context, diskPath string, d
if scsiController == nil {
klog.Errorf("Cannot find SCSI controller of type: %q in VM", volumeOptions.SCSIControllerType)
// attempt clean up of scsi controller
vm.deleteController(ctx, newSCSIController, vmDevices)
if err := vm.deleteController(ctx, newSCSIController, vmDevices); err != nil {
return nil, nil, fmt.Errorf("failed to delete SCSI controller after failing to find it on VM: %v", err)
}
return nil, nil, fmt.Errorf("Cannot find SCSI controller of type: %q in VM", volumeOptions.SCSIControllerType)
}
}
Expand Down Expand Up @@ -351,7 +365,10 @@ func (vm *VirtualMachine) createAndAttachSCSIController(ctx context.Context, dis
if err != nil {
klog.V(LogLevel).Infof("Cannot add SCSI controller to VM: %q. err: %+v", vm.InventoryPath, err)
// attempt clean up of scsi controller
vm.deleteController(ctx, newSCSIController, vmDevices)
nestedErr := vm.deleteController(ctx, newSCSIController, vmDevices)
if nestedErr != nil {
return nil, fmt.Errorf("failed to delete SCSI controller after failing to add it to vm with err='%v': %v", err, nestedErr)
}
return nil, err
}
return newSCSIController, nil
Expand Down
14 changes: 11 additions & 3 deletions pkg/cloudprovider/providers/vsphere/vsphere.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1288,7 +1288,9 @@ func (vs *VSphere) NodeAdded(obj interface{}) {
}

klog.V(4).Infof("Node added: %+v", node)
vs.nodeManager.RegisterNode(node)
if err := vs.nodeManager.RegisterNode(node); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Note that RegisterNode always returns nil, as does UnRegisterNode. Looks like those methods could be changed to have no return value, instead of unused error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially that was true, but RegisterNode calls DiscoverNode and that does return an error which has been ignored so far. I've changed that above.

klog.Errorf("failed to add node %+v: %v", node, err)
}
}

// Notification handler when node is removed from k8s cluster.
Expand All @@ -1300,7 +1302,9 @@ func (vs *VSphere) NodeDeleted(obj interface{}) {
}

klog.V(4).Infof("Node deleted: %+v", node)
vs.nodeManager.UnRegisterNode(node)
if err := vs.nodeManager.UnRegisterNode(node); err != nil {
klog.Errorf("failed to delete node %s: %v", node.Name, err)
}
}

func (vs *VSphere) NodeManager() (nodeManager *NodeManager) {
Expand All @@ -1316,7 +1320,11 @@ func withTagsClient(ctx context.Context, connection *vclib.VSphereConnection, f
if err := c.Login(ctx, user); err != nil {
return err
}
defer c.Logout(ctx)
defer func() {
if err := c.Logout(ctx); err != nil {
klog.Errorf("failed to logout: %v", err)
}
}()
return f(c)
}

Expand Down
7 changes: 6 additions & 1 deletion pkg/cloudprovider/providers/vsphere/vsphere_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,12 @@ func getVSphereConfig() (*VSphereConfig, error) {
if err != nil {
return nil, err
}
defer confFile.Close()
defer func() {
if err := confFile.Close(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Can just ignore this error _ := ... since the file is only opened for reading.

klog.Errorf("failed to close config file: %v", err)
}
}()

cfg, err := readConfig(confFile)
if err != nil {
return nil, err
Expand Down