From c35cc9437b7300672f8eca769bdb11d95667a121 Mon Sep 17 00:00:00 2001 From: Stoyan Rachev Date: Wed, 14 Oct 2020 14:42:29 +0300 Subject: [PATCH] Improve error wrapping, error messages, and comments --- pkg/kubevirt/apis/provider_spec.go | 3 +- pkg/kubevirt/core/core.go | 78 ++++++++++++------- pkg/kubevirt/core/core_test.go | 31 ++------ .../custom_errors.go => core/errors.go} | 11 +-- pkg/kubevirt/core/util.go | 16 ++-- pkg/kubevirt/machine_server.go | 54 ++++++------- pkg/kubevirt/machine_server_util.go | 13 ++-- pkg/kubevirt/plugin.go | 40 ++++------ pkg/kubevirt/validation/validation.go | 1 - 9 files changed, 113 insertions(+), 134 deletions(-) rename pkg/kubevirt/{errors/custom_errors.go => core/errors.go} (72%) diff --git a/pkg/kubevirt/apis/provider_spec.go b/pkg/kubevirt/apis/provider_spec.go index d03ba59..8340619 100644 --- a/pkg/kubevirt/apis/provider_spec.go +++ b/pkg/kubevirt/apis/provider_spec.go @@ -20,7 +20,8 @@ import ( cdicorev1alpha1 "kubevirt.io/containerized-data-importer/pkg/apis/core/v1alpha1" ) -// KubeVirtProviderSpec is the spec to be used while parsing the calls. +// KubeVirtProviderSpec is the kubevirt provider specification. +// It contains parameters to be used when creating kubevirt VMs. type KubeVirtProviderSpec struct { // Region is the VM region name. Region string `json:"region"` diff --git a/pkg/kubevirt/core/core.go b/pkg/kubevirt/core/core.go index 11d54cd..f6fa0b5 100644 --- a/pkg/kubevirt/core/core.go +++ b/pkg/kubevirt/core/core.go @@ -12,7 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package core contains the cloud kubevirt specific implementations to manage machines package core import ( @@ -22,8 +21,8 @@ import ( "time" api "github.com/gardener/machine-controller-manager-provider-kubevirt/pkg/kubevirt/apis" - "github.com/gardener/machine-controller-manager-provider-kubevirt/pkg/kubevirt/errors" + "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -36,7 +35,7 @@ import ( ) const ( - // ProviderName specifies the machine controller for kubevirt cloud provider + // ProviderName is the kubevirt provider name. ProviderName = "kubevirt" ) @@ -70,23 +69,22 @@ func (f ServerVersionFactoryFunc) GetServerVersion(secret *corev1.Secret) (strin return f(secret) } -// PluginSPIImpl is the real implementation of PluginSPI interface -// that makes the calls to the provider SDK +// PluginSPIImpl is the implementation of PluginSPI interface. type PluginSPIImpl struct { cf ClientFactory svf ServerVersionFactory } // NewPluginSPIImpl creates a new PluginSPIImpl with the given ClientFactory and ServerVersionFactory. -func NewPluginSPIImpl(cf ClientFactory, svf ServerVersionFactory) (*PluginSPIImpl, error) { +func NewPluginSPIImpl(cf ClientFactory, svf ServerVersionFactory) *PluginSPIImpl { return &PluginSPIImpl{ cf: cf, svf: svf, - }, nil + } } -// CreateMachine creates a Kubevirt virtual machine with the given name and an associated data volume based on the -// DataVolumeTemplate, using the given provider spec. It also creates a secret where the userdata(cloud-init) are saved and mounted on the VM. +// CreateMachine creates a machine with the given name, using the given provider spec and secret. +// Here it creates a kubevirt virtual machine and a secret containing the userdata (cloud-init). func (p PluginSPIImpl) CreateMachine(ctx context.Context, machineName string, providerSpec *api.KubeVirtProviderSpec, secret *corev1.Secret) (providerID string, err error) { // Generate a unique name for the userdata secret userDataSecretName := fmt.Sprintf("userdata-%s-%s", machineName, strconv.Itoa(int(time.Now().Unix()))) @@ -94,7 +92,7 @@ func (p PluginSPIImpl) CreateMachine(ctx context.Context, machineName string, pr // Get client and namespace from secret c, namespace, err := p.cf.GetClient(secret) if err != nil { - return "", fmt.Errorf("failed to create client: %v", err) + return "", errors.Wrap(err, "could not create client") } // Build interfaces and networks @@ -106,7 +104,7 @@ func (p PluginSPIImpl) CreateMachine(ctx context.Context, machineName string, pr // Get Kubernetes version k8sVersion, err := p.svf.GetServerVersion(secret) if err != nil { - return "", fmt.Errorf("failed to get server version: %v", err) + return "", errors.Wrap(err, "could not get server version") } // Build affinity @@ -115,7 +113,7 @@ func (p PluginSPIImpl) CreateMachine(ctx context.Context, machineName string, pr // Add SSH keys to user data userData, err := addUserSSHKeysToUserData(string(secret.Data["userData"]), providerSpec.SSHKeys) if err != nil { - return "", fmt.Errorf("failed to add ssh keys to cloud-init: %v", err) + return "", err } // Initialize VM labels @@ -164,7 +162,7 @@ func (p PluginSPIImpl) CreateMachine(ctx context.Context, machineName string, pr // Create the VM if err := c.Create(ctx, virtualMachine); err != nil { - return "", fmt.Errorf("failed to create VirtualMachine: %v", err) + return "", errors.Wrapf(err, "could not create VirtualMachine %q", machineName) } // Build the userdata secret @@ -183,93 +181,113 @@ func (p PluginSPIImpl) CreateMachine(ctx context.Context, machineName string, pr // Create the userdata secret if err := c.Create(ctx, userDataSecret); err != nil { - return "", fmt.Errorf("failed to create secret for userdata: %v", err) + return "", errors.Wrapf(err, "could not create userdata secret %q", userDataSecretName) } + // Return the VM provider ID return encodeProviderID(machineName), nil } -// DeleteMachine deletes the Kubevirt virtual machine with the given name. +// DeleteMachine deletes the machine with the given name and provider id, using the given provider spec and secret. +// Here it deletes the kubevirt virtual machine with the given name. func (p PluginSPIImpl) DeleteMachine(ctx context.Context, machineName, _ string, _ *api.KubeVirtProviderSpec, secret *corev1.Secret) (foundProviderID string, err error) { + // Get client and namespace from secret c, namespace, err := p.cf.GetClient(secret) if err != nil { - return "", fmt.Errorf("failed to create client: %v", err) + return "", errors.Wrap(err, "could not create client") } + // Get the VM by name virtualMachine, err := p.getVM(ctx, c, machineName, namespace) if err != nil { - if errors.IsMachineNotFoundError(err) { - klog.V(2).Infof("skip VirtualMachine evicting, VirtualMachine instance %s is not found", machineName) + if IsMachineNotFoundError(err) { + klog.V(2).Infof("VirtualMachine %s not found", machineName) return "", nil } return "", err } + // Delete the VM if err := client.IgnoreNotFound(c.Delete(ctx, virtualMachine)); err != nil { - return "", fmt.Errorf("failed to delete VirtualMachine %v: %v", machineName, err) + return "", errors.Wrapf(err, "could not delete VirtualMachine %q", machineName) } + + // Return the VM provider ID return encodeProviderID(virtualMachine.Name), nil } -// GetMachineStatus fetches the provider id of the Kubevirt virtual machine with the given name. +// GetMachineStatus returns the provider id of the machine with the given name and provider id, using the given provider spec and secret. +// Here it returns the provider id of the kubevirt virtual machine with the given name. func (p PluginSPIImpl) GetMachineStatus(ctx context.Context, machineName, _ string, _ *api.KubeVirtProviderSpec, secret *corev1.Secret) (foundProviderID string, err error) { + // Get client and namespace from secret c, namespace, err := p.cf.GetClient(secret) if err != nil { - return "", fmt.Errorf("failed to create client: %v", err) + return "", errors.Wrap(err, "could not create client") } + // Get the VM by name virtualMachine, err := p.getVM(ctx, c, machineName, namespace) if err != nil { return "", err } + // Return the VM provider ID return encodeProviderID(virtualMachine.Name), nil } -// ListMachines lists the provider ids of all Kubevirt virtual machines. +// ListMachines lists all machines matching the given provider spec and secret. +// Here it lists all kubevirt virtual machines matching the tags of the given provider spec. func (p PluginSPIImpl) ListMachines(ctx context.Context, providerSpec *api.KubeVirtProviderSpec, secret *corev1.Secret) (providerIDList map[string]string, err error) { + // Get client and namespace from secret c, namespace, err := p.cf.GetClient(secret) if err != nil { - return nil, fmt.Errorf("failed to create client: %v", err) + return nil, errors.Wrap(err, "could not create client") } + // Initialize VM labels var vmLabels = map[string]string{} if len(providerSpec.Tags) > 0 { vmLabels = providerSpec.Tags } + // List all VMs matching the labels virtualMachineList, err := p.listVMs(ctx, c, namespace, vmLabels) if err != nil { return nil, err } + // Return a map containing the provider IDs and names of all found VMs var providerIDs = make(map[string]string, len(virtualMachineList.Items)) for _, virtualMachine := range virtualMachineList.Items { providerIDs[encodeProviderID(virtualMachine.Name)] = virtualMachine.Name } - return providerIDs, nil } -// ShutDownMachine shuts down the Kubevirt virtual machine with the given name by setting its spec.running field to false. +// ShutDownMachine shuts down the machine with the given name and provider id, using the given provider spec and secret. +// Here it shuts down the kubevirt virtual machine with the given name by setting its spec.running field to false. func (p PluginSPIImpl) ShutDownMachine(ctx context.Context, machineName, _ string, _ *api.KubeVirtProviderSpec, secret *corev1.Secret) (foundProviderID string, err error) { + // Get client and namespace from secret c, namespace, err := p.cf.GetClient(secret) if err != nil { - return "", fmt.Errorf("failed to create client: %v", err) + return "", errors.Wrap(err, "could not create client") } + // Get the VM by name virtualMachine, err := p.getVM(ctx, c, machineName, namespace) if err != nil { return "", err } + // Set the VM spec.running field to false virtualMachine.Spec.Running = pointer.BoolPtr(false) if err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { return c.Update(ctx, virtualMachine) }); err != nil { - return "", fmt.Errorf("failed to update VirtualMachine running state: %v", err) + return "", errors.Wrapf(err, "could not update VirtualMachine %q", machineName) } + // Return the VM provider ID return encodeProviderID(virtualMachine.Name), nil } @@ -277,11 +295,11 @@ func (p PluginSPIImpl) getVM(ctx context.Context, c client.Client, machineName, virtualMachine := &kubevirtv1.VirtualMachine{} if err := c.Get(ctx, types.NamespacedName{Namespace: namespace, Name: machineName}, virtualMachine); err != nil { if apierrors.IsNotFound(err) { - return nil, &errors.MachineNotFoundError{ + return nil, &MachineNotFoundError{ Name: machineName, } } - return nil, fmt.Errorf("failed to get VirtualMachine: %v", err) + return nil, errors.Wrapf(err, "could not get VirtualMachine %q", machineName) } return virtualMachine, nil } @@ -293,7 +311,7 @@ func (p PluginSPIImpl) listVMs(ctx context.Context, c client.Client, namespace s opts = append(opts, client.MatchingLabels(vmLabels)) } if err := c.List(ctx, virtualMachineList, opts...); err != nil { - return nil, fmt.Errorf("failed to list VirtualMachines: %v", err) + return nil, errors.Wrapf(err, "could not list VirtualMachines in namespace %q", namespace) } return virtualMachineList, nil } diff --git a/pkg/kubevirt/core/core_test.go b/pkg/kubevirt/core/core_test.go index 7f9512b..d19bc84 100644 --- a/pkg/kubevirt/core/core_test.go +++ b/pkg/kubevirt/core/core_test.go @@ -81,12 +81,9 @@ func TestPluginSPIImpl_CreateMachine(t *testing.T) { fakeClient := fake.NewFakeClientWithScheme(scheme.Scheme) t.Run("CreateMachine", func(t *testing.T) { mf := newMockFactory(fakeClient, namespace, serverVersion) - plugin, err := NewPluginSPIImpl(mf, mf) - if err != nil { - t.Fatalf("failed to create plugin: %v", err) - } + plugin := NewPluginSPIImpl(mf, mf) - _, err = plugin.CreateMachine(context.Background(), machineName, providerSpec, &corev1.Secret{}) + _, err := plugin.CreateMachine(context.Background(), machineName, providerSpec, &corev1.Secret{}) if err != nil { t.Fatalf("failed to create machine: %v", err) } @@ -106,12 +103,9 @@ func TestPluginSPIImpl_GetMachineStatus(t *testing.T) { fakeClient := fake.NewFakeClientWithScheme(scheme.Scheme) t.Run("GetMachineStatus", func(t *testing.T) { mf := newMockFactory(fakeClient, namespace, serverVersion) - plugin, err := NewPluginSPIImpl(mf, mf) - if err != nil { - t.Fatalf("failed to create plugin: %v", err) - } + plugin := NewPluginSPIImpl(mf, mf) - _, err = plugin.CreateMachine(context.Background(), machineName, providerSpec, &corev1.Secret{}) + _, err := plugin.CreateMachine(context.Background(), machineName, providerSpec, &corev1.Secret{}) if err != nil { t.Fatalf("failed to create machine: %v", err) } @@ -131,12 +125,9 @@ func TestPluginSPIImpl_ListMachines(t *testing.T) { fakeClient := fake.NewFakeClientWithScheme(scheme.Scheme) t.Run("ListMachines", func(t *testing.T) { mf := newMockFactory(fakeClient, namespace, serverVersion) - plugin, err := NewPluginSPIImpl(mf, mf) - if err != nil { - t.Fatalf("failed to create plugin: %v", err) - } + plugin := NewPluginSPIImpl(mf, mf) - _, err = plugin.CreateMachine(context.Background(), machineName, providerSpec, &corev1.Secret{}) + _, err := plugin.CreateMachine(context.Background(), machineName, providerSpec, &corev1.Secret{}) if err != nil { t.Fatalf("failed to create machine: %v", err) } @@ -156,10 +147,7 @@ func TestPluginSPIImpl_ShutDownMachine(t *testing.T) { fakeClient := fake.NewFakeClientWithScheme(scheme.Scheme) t.Run("ShutDownMachine", func(t *testing.T) { mf := newMockFactory(fakeClient, namespace, serverVersion) - plugin, err := NewPluginSPIImpl(mf, mf) - if err != nil { - t.Fatalf("failed to create plugin: %v", err) - } + plugin := NewPluginSPIImpl(mf, mf) providerID, err := plugin.CreateMachine(context.Background(), machineName, providerSpec, &corev1.Secret{}) if err != nil { @@ -186,10 +174,7 @@ func TestPluginSPIImpl_DeleteMachine(t *testing.T) { fakeClient := fake.NewFakeClientWithScheme(scheme.Scheme) t.Run("DeleteMachine", func(t *testing.T) { mf := newMockFactory(fakeClient, namespace, serverVersion) - plugin, err := NewPluginSPIImpl(mf, mf) - if err != nil { - t.Fatalf("failed to create plugin: %v", err) - } + plugin := NewPluginSPIImpl(mf, mf) providerID, err := plugin.CreateMachine(context.Background(), machineName, providerSpec, &corev1.Secret{}) if err != nil { diff --git a/pkg/kubevirt/errors/custom_errors.go b/pkg/kubevirt/core/errors.go similarity index 72% rename from pkg/kubevirt/errors/custom_errors.go rename to pkg/kubevirt/core/errors.go index 20e77a5..4a9252b 100644 --- a/pkg/kubevirt/errors/custom_errors.go +++ b/pkg/kubevirt/core/errors.go @@ -12,26 +12,23 @@ // See the License for the specific language governing permissions and // limitations under the License. -package errors +package core import ( "fmt" ) -// MachineNotFoundError is used to indicate not found error in PluginSPI +// MachineNotFoundError represents a "machine not found" error. type MachineNotFoundError struct { // Name is the machine name Name string - // MachineID is the machine uuid - MachineID string } -// Error returns the MachineNotFoundError message with machine name and uuid. func (e *MachineNotFoundError) Error() string { - return fmt.Sprintf("machine name=%s, uuid=%s not found", e.Name, e.MachineID) + return fmt.Sprintf("machine %q not found", e.Name) } -// IsMachineNotFoundError identifies MachineNotFoundError and returns true if it is and false if not. +// IsMachineNotFoundError returns true if the given error is a MachineNotFoundError, false otherwise. func IsMachineNotFoundError(err error) bool { switch err.(type) { case *MachineNotFoundError: diff --git a/pkg/kubevirt/core/util.go b/pkg/kubevirt/core/util.go index 67c7c63..0d78911 100644 --- a/pkg/kubevirt/core/util.go +++ b/pkg/kubevirt/core/util.go @@ -15,13 +15,13 @@ package core import ( - "errors" "fmt" "strings" api "github.com/gardener/machine-controller-manager-provider-kubevirt/pkg/kubevirt/apis" "github.com/Masterminds/semver" + "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" @@ -40,15 +40,15 @@ func GetClient(secret *corev1.Secret) (client.Client, string, error) { } config, err := clientConfig.ClientConfig() if err != nil { - return nil, "", fmt.Errorf("could not get REST config from client config: %v", err) + return nil, "", errors.Wrap(err, "could not get REST config from client config") } c, err := client.New(config, client.Options{}) if err != nil { - return nil, "", fmt.Errorf("could not create client from REST config: %v", err) + return nil, "", errors.Wrap(err, "could not create client from REST config") } namespace, _, err := clientConfig.Namespace() if err != nil { - return nil, "", fmt.Errorf("could not get namespace from client config: %v", err) + return nil, "", errors.Wrap(err, "could not get namespace from client config") } return c, namespace, nil } @@ -61,15 +61,15 @@ func GetServerVersion(secret *corev1.Secret) (string, error) { } config, err := clientConfig.ClientConfig() if err != nil { - return "", fmt.Errorf("could not get REST config from client config: %v", err) + return "", errors.Wrap(err, "could not get REST config from client config") } cs, err := kubernetes.NewForConfig(config) if err != nil { - return "", fmt.Errorf("could not create clientset from REST config: %v", err) + return "", errors.Wrap(err, "could not create clientset from REST config") } versionInfo, err := cs.ServerVersion() if err != nil { - return "", fmt.Errorf("could not get server version: %v", err) + return "", errors.Wrap(err, "could not get server version") } return versionInfo.GitVersion, nil } @@ -81,7 +81,7 @@ func getClientConfig(secret *corev1.Secret) (clientcmd.ClientConfig, error) { } clientConfig, err := clientcmd.NewClientConfigFromBytes(kubeconfig) if err != nil { - return nil, fmt.Errorf("could not create client config from kubeconfig: %v", err) + return nil, errors.Wrap(err, "could not create client config from kubeconfig") } return clientConfig, nil } diff --git a/pkg/kubevirt/machine_server.go b/pkg/kubevirt/machine_server.go index 40b1330..c26fa53 100644 --- a/pkg/kubevirt/machine_server.go +++ b/pkg/kubevirt/machine_server.go @@ -12,7 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package kubevirt contains the cloud kubevirt specific implementations to manage machines package kubevirt import ( @@ -48,9 +47,8 @@ import ( // This logic is used by safety controller to delete orphan VMs which are not backed by any machine CRD // func (p *MachinePlugin) CreateMachine(ctx context.Context, req *driver.CreateMachineRequest) (*driver.CreateMachineResponse, error) { - // Log messages to track request - klog.V(2).Infof("CreateMachine request has been received for %q", req.Machine.Name) - defer klog.V(2).Infof("CreateMachine request has been processed for %q", req.Machine.Name) + klog.V(2).Infof("CreateMachine request received for %q", req.Machine.Name) + defer klog.V(2).Infof("CreateMachine request processed for %q", req.Machine.Name) providerSpec, err := decodeProviderSpecAndSecret(req.MachineClass, req.Secret) if err != nil { @@ -59,15 +57,14 @@ func (p *MachinePlugin) CreateMachine(ctx context.Context, req *driver.CreateMac providerID, err := p.SPI.CreateMachine(ctx, req.Machine.Name, providerSpec, req.Secret) if err != nil { - return nil, prepareErrorf(err, "could not create machine %q", req.Machine.Name) + return nil, wrapf(err, "could not create machine %q", req.Machine.Name) } - response := &driver.CreateMachineResponse{ + return &driver.CreateMachineResponse{ ProviderID: providerID, NodeName: req.Machine.Name, LastKnownState: fmt.Sprintf("Created %s", providerID), - } - return response, nil + }, nil } // DeleteMachine handles a machine deletion request @@ -82,9 +79,8 @@ func (p *MachinePlugin) CreateMachine(ctx context.Context, req *driver.CreateMac // Could be helpful to continue operations in future requests. // func (p *MachinePlugin) DeleteMachine(ctx context.Context, req *driver.DeleteMachineRequest) (*driver.DeleteMachineResponse, error) { - // Log messages to track delete request - klog.V(2).Infof("DeleteMachine request has been received for %q", req.Machine.Name) - defer klog.V(2).Infof("DeleteMachine request has been processed for %q", req.Machine.Name) + klog.V(2).Infof("DeleteMachine request received for %q", req.Machine.Name) + defer klog.V(2).Infof("DeleteMachine request processed for %q", req.Machine.Name) providerSpec, err := decodeProviderSpecAndSecret(req.MachineClass, req.Secret) if err != nil { @@ -93,13 +89,12 @@ func (p *MachinePlugin) DeleteMachine(ctx context.Context, req *driver.DeleteMac providerID, err := p.SPI.DeleteMachine(ctx, req.Machine.Name, req.Machine.Spec.ProviderID, providerSpec, req.Secret) if err != nil { - return nil, prepareErrorf(err, "could not delete machine %q", req.Machine.Name) + return nil, wrapf(err, "could not delete machine %q", req.Machine.Name) } - response := &driver.DeleteMachineResponse{ + return &driver.DeleteMachineResponse{ LastKnownState: fmt.Sprintf("Deleted %s", providerID), - } - return response, nil + }, nil } // GetMachineStatus handles a machine get status request @@ -119,9 +114,8 @@ func (p *MachinePlugin) DeleteMachine(ctx context.Context, req *driver.DeleteMac // // The request should return a NOT_FOUND (5) status errors code if the machine is not existing func (p *MachinePlugin) GetMachineStatus(ctx context.Context, req *driver.GetMachineStatusRequest) (*driver.GetMachineStatusResponse, error) { - // Log messages to track start and end of request - klog.V(2).Infof("GetMachineStatus request has been received for %q", req.Machine.Name) - defer klog.V(2).Infof("GetMachineStatus request has been processed for %q", req.Machine.Name) + klog.V(2).Infof("GetMachineStatus request received for %q", req.Machine.Name) + defer klog.V(2).Infof("GetMachineStatus request processed for %q", req.Machine.Name) providerSpec, err := decodeProviderSpecAndSecret(req.MachineClass, req.Secret) if err != nil { @@ -130,17 +124,15 @@ func (p *MachinePlugin) GetMachineStatus(ctx context.Context, req *driver.GetMac providerID, err := p.SPI.GetMachineStatus(ctx, req.Machine.Name, req.Machine.Spec.ProviderID, providerSpec, req.Secret) if err != nil { - return nil, prepareErrorf(err, "could not get status of machine %q", req.Machine.Name) + return nil, wrapf(err, "could not get status of machine %q", req.Machine.Name) } - response := &driver.GetMachineStatusResponse{ + klog.V(2).Infof("Found machine with provider ID %q for %q", providerID, req.Machine.Name) + + return &driver.GetMachineStatusResponse{ ProviderID: providerID, NodeName: req.Machine.Name, - } - - klog.V(2).Infof("Found machine with provider ID %q for %q", response.ProviderID, req.Machine.Name) - - return response, nil + }, nil } // ListMachines lists all the machines possibly created by a providerSpec @@ -157,9 +149,8 @@ func (p *MachinePlugin) GetMachineStatus(ctx context.Context, req *driver.GetMac // for all machine's who where possibilly created by this ProviderSpec // func (p *MachinePlugin) ListMachines(ctx context.Context, req *driver.ListMachinesRequest) (*driver.ListMachinesResponse, error) { - // Log messages to track start and end of request - klog.V(2).Infof("ListMachines request has been received for %q", req.MachineClass.Name) - defer klog.V(2).Infof("ListMachines request has been processed for %q", req.MachineClass.Name) + klog.V(2).Infof("ListMachines request received for %q", req.MachineClass.Name) + defer klog.V(2).Infof("ListMachines request processed for %q", req.MachineClass.Name) providerSpec, err := decodeProviderSpecAndSecret(req.MachineClass, req.Secret) if err != nil { @@ -168,7 +159,7 @@ func (p *MachinePlugin) ListMachines(ctx context.Context, req *driver.ListMachin machineList, err := p.SPI.ListMachines(ctx, providerSpec, req.Secret) if err != nil { - return nil, prepareErrorf(err, "could not list machines") + return nil, wrapf(err, "could not list machines") } klog.V(2).Infof("Found %d machines for %q", len(machineList), req.MachineClass.Name) @@ -187,9 +178,8 @@ func (p *MachinePlugin) ListMachines(ctx context.Context, req *driver.ListMachin // VolumeIDs []string VolumeIDs is a repeated list of VolumeIDs. // func (p *MachinePlugin) GetVolumeIDs(ctx context.Context, req *driver.GetVolumeIDsRequest) (*driver.GetVolumeIDsResponse, error) { - // Log messages to track start and end of request - klog.V(2).Infof("GetVolumeIDs request has been received for %q", req.PVSpecs) - defer klog.V(2).Infof("GetVolumeIDs request has been processed for %q", req.PVSpecs) + klog.V(2).Infof("GetVolumeIDs request received for %q", req.PVSpecs) + defer klog.V(2).Infof("GetVolumeIDs request processed for %q", req.PVSpecs) return &driver.GetVolumeIDsResponse{}, status.Error(codes.Unimplemented, "") } diff --git a/pkg/kubevirt/machine_server_util.go b/pkg/kubevirt/machine_server_util.go index a95aad6..b307091 100644 --- a/pkg/kubevirt/machine_server_util.go +++ b/pkg/kubevirt/machine_server_util.go @@ -2,10 +2,9 @@ package kubevirt import ( "encoding/json" - "fmt" api "github.com/gardener/machine-controller-manager-provider-kubevirt/pkg/kubevirt/apis" - clouderrors "github.com/gardener/machine-controller-manager-provider-kubevirt/pkg/kubevirt/errors" + "github.com/gardener/machine-controller-manager-provider-kubevirt/pkg/kubevirt/core" "github.com/gardener/machine-controller-manager-provider-kubevirt/pkg/kubevirt/validation" "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1" @@ -26,7 +25,7 @@ func decodeProviderSpecAndSecret(machineClass *v1alpha1.MachineClass, secret *co } if errs := validation.ValidateKubevirtProviderSpec(spec); len(errs) > 0 { - err := fmt.Errorf("could not validate provider spec: %v", errs) + err := errors.Errorf("could not validate provider spec: %v", errs) klog.V(2).Infof(err.Error()) return nil, status.Error(codes.Internal, err.Error()) } @@ -38,7 +37,7 @@ func decodeProviderSpecAndSecret(machineClass *v1alpha1.MachineClass, secret *co } if errs := validation.ValidateKubevirtProviderSecret(secret); len(errs) > 0 { - err := fmt.Errorf("could not validate provider secret: %v", errs) + err := errors.Errorf("could not validate provider secret: %v", errs) klog.V(2).Infof(err.Error()) return nil, status.Error(codes.Internal, err.Error()) } @@ -46,14 +45,14 @@ func decodeProviderSpecAndSecret(machineClass *v1alpha1.MachineClass, secret *co return spec, nil } -// prepareErrorf prepares, formats, and wraps the given error in a status.Error. -func prepareErrorf(err error, format string, args ...interface{}) error { +// wrapf wraps the given error in a status.Error. +func wrapf(err error, format string, args ...interface{}) error { var ( code codes.Code wrapped error ) switch err.(type) { - case *clouderrors.MachineNotFoundError: + case *core.MachineNotFoundError: code = codes.NotFound wrapped = err default: diff --git a/pkg/kubevirt/plugin.go b/pkg/kubevirt/plugin.go index ddf6146..239b138 100644 --- a/pkg/kubevirt/plugin.go +++ b/pkg/kubevirt/plugin.go @@ -22,41 +22,31 @@ import ( "github.com/gardener/machine-controller-manager/pkg/util/provider/driver" corev1 "k8s.io/api/core/v1" - "k8s.io/klog" ) -// PluginSPI provides an interface to deal with cloud provider session -// You can optionally enhance this interface to add interface methods here -// You can use it to mock cloud provider calls +// PluginSPI is an interface for provider-specific machine operations. type PluginSPI interface { - // CreateMachine handles a machine creation request - CreateMachine(ctx context.Context, machineName string, providerSpec *api.KubeVirtProviderSpec, secrets *corev1.Secret) (providerID string, err error) - // DeleteMachine handles a machine deletion request - DeleteMachine(ctx context.Context, machineName, providerID string, providerSpec *api.KubeVirtProviderSpec, secrets *corev1.Secret) (foundProviderID string, err error) - // GetMachineStatus handles a machine get status request - GetMachineStatus(ctx context.Context, machineName, providerID string, providerSpec *api.KubeVirtProviderSpec, secrets *corev1.Secret) (foundProviderID string, err error) - // ListMachines lists all the machines possibly created by a providerSpec - ListMachines(ctx context.Context, providerSpec *api.KubeVirtProviderSpec, secrets *corev1.Secret) (providerIDList map[string]string, err error) - // ShutDownMachine shuts down a machine by name - ShutDownMachine(ctx context.Context, machineName, providerID string, providerSpec *api.KubeVirtProviderSpec, secrets *corev1.Secret) (foundProviderID string, err error) + // CreateMachine creates a machine with the given name, using the given provider spec and secret. + CreateMachine(ctx context.Context, machineName string, providerSpec *api.KubeVirtProviderSpec, secret *corev1.Secret) (providerID string, err error) + // DeleteMachine deletes the machine with the given name and provider id, using the given provider spec and secret. + DeleteMachine(ctx context.Context, machineName, providerID string, providerSpec *api.KubeVirtProviderSpec, secret *corev1.Secret) (foundProviderID string, err error) + // GetMachineStatus returns the provider id of the machine with the given name and provider id, using the given provider spec and secret. + GetMachineStatus(ctx context.Context, machineName, providerID string, providerSpec *api.KubeVirtProviderSpec, secret *corev1.Secret) (foundProviderID string, err error) + // ListMachines lists all machines matching the given provider spec and secret. + ListMachines(ctx context.Context, providerSpec *api.KubeVirtProviderSpec, secret *corev1.Secret) (providerIDList map[string]string, err error) + // ShutDownMachine shuts down the machine with the given name and provider id, using the given provider spec and secret. + ShutDownMachine(ctx context.Context, machineName, providerID string, providerSpec *api.KubeVirtProviderSpec, secret *corev1.Secret) (foundProviderID string, err error) } -// MachinePlugin implements the cmi.MachineServer -// It also implements the pluginSPI interface +// MachinePlugin implements cmi.MachineServer by delegating to a PluginSPI implementation. type MachinePlugin struct { - // SPI provides an interface to deal with cloud provider session. + // SPI is an implementation of the PluginSPI interface. SPI PluginSPI } -// NewKubevirtPlugin returns a new Kubevirt cloud provider driver. +// NewKubevirtPlugin creates a new kubevirt driver. func NewKubevirtPlugin() driver.Driver { - plugin, err := core.NewPluginSPIImpl(core.ClientFactoryFunc(core.GetClient), core.ServerVersionFactoryFunc(core.GetServerVersion)) - if err != nil { - klog.Errorf("failed to create Kubevirt plugin") - return nil - } - return &MachinePlugin{ - SPI: plugin, + SPI: core.NewPluginSPIImpl(core.ClientFactoryFunc(core.GetClient), core.ServerVersionFactoryFunc(core.GetServerVersion)), } } diff --git a/pkg/kubevirt/validation/validation.go b/pkg/kubevirt/validation/validation.go index 2188760..33f3267 100644 --- a/pkg/kubevirt/validation/validation.go +++ b/pkg/kubevirt/validation/validation.go @@ -12,7 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package validation is used to validate cloud specific KubeVirtProviderSpec package validation import (