diff --git a/pkg/driver/driver.go b/pkg/driver/driver.go index f7540e17..22b2cd05 100644 --- a/pkg/driver/driver.go +++ b/pkg/driver/driver.go @@ -7,11 +7,14 @@ package driver import ( "context" + "errors" "fmt" + "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1" "github.com/gardener/machine-controller-manager/pkg/util/provider/driver" "github.com/gardener/machine-controller-manager/pkg/util/provider/machinecodes/codes" "github.com/gardener/machine-controller-manager/pkg/util/provider/machinecodes/status" + "github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers" corev1 "k8s.io/api/core/v1" "k8s.io/klog/v2" @@ -28,39 +31,31 @@ const ( // NOTE // // The basic working of the controller will work with just implementing the CreateMachine() & DeleteMachine() methods. -// You can first implement these two methods and check the working of the controller. -// Leaving the other methods to NOT_IMPLEMENTED error status. -// Once this works you can implement the rest of the methods. +// The other methods should be implemented according to specs or return the NOT_IMPLEMENTED error status. // -// Also make sure each method return appropriate errors mentioned in `https://github.com/gardener/machine-controller-manager/blob/master/docs/development/machine_error_codes.md` - -// CreateMachine handles a machine creation request -// -// OPTIONAL IMPLEMENTATION LOGIC -// It is optionally expected by the safety controller to use an identification mechanism to map the VM Created by a providerSpec. -// These could be done using tag(s)/resource-groups etc. -// This logic is used by safety controller to delete orphan VMs which are not backed by any machine CRD -func (p *OpenstackDriver) CreateMachine(ctx context.Context, req *driver.CreateMachineRequest) (*driver.CreateMachineResponse, error) { - 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) +// Make sure each method returns the appropriate errors mentioned in +// `https://github.com/gardener/machine-controller-manager/blob/master/docs/development/machine_error_codes.md` +// setupExecutor handles the common validation and setup, returning a ready-to-use executor. +func (p *OpenstackDriver) setupExecutor(ctx context.Context, machineClass *v1alpha1.MachineClass, secret *corev1.Secret) (*executor.Executor, error) { // Check if incoming provider in the MachineClass is a provider we support - if req.MachineClass.Provider != openstackProvider { - err := fmt.Errorf("requested for Provider '%s', we only support '%s'", req.MachineClass.Provider, openstackProvider) + if machineClass.Provider != openstackProvider { + err := fmt.Errorf("requested for Provider '%s', we only support '%s'", machineClass.Provider, openstackProvider) return nil, status.Error(codes.InvalidArgument, err.Error()) } - providerConfig, err := p.decodeProviderSpec(req.MachineClass.ProviderSpec) + providerConfig, err := p.decodeProviderSpec(machineClass.ProviderSpec) if err != nil { - klog.Errorf("decoding provider spec for machine class %q failed with: %v", req.MachineClass.Name, err) + klog.Errorf("decoding provider spec for machine class %q failed with: %v", machineClass.Name, err) return nil, status.Error(codes.InvalidArgument, err.Error()) } - if err := validation.ValidateRequest(providerConfig, req.Secret); err != nil { - klog.Errorf("validating request for machine %q failed with: %v", req.Machine.Name, err) + + if err := validation.ValidateRequest(providerConfig, secret); err != nil { + klog.Errorf("validating request failed with: %v", err) return nil, status.Error(codes.InvalidArgument, err.Error()) } - factory, err := client.NewFactoryFromSecret(ctx, req.Secret) + factory, err := client.NewFactoryFromSecret(ctx, secret) if err != nil { klog.Errorf("failed to construct OpenStack client: %v", err) return nil, status.Error(mapErrorToCode(err), fmt.Sprintf("failed to construct OpenStack client: %v", err)) @@ -72,6 +67,24 @@ func (p *OpenstackDriver) CreateMachine(ctx context.Context, req *driver.CreateM return nil, status.Error(mapErrorToCode(err), fmt.Sprintf("failed to construct context for the request: %v", err)) } + return ex, nil +} + +// CreateMachine handles a machine creation request +// +// OPTIONAL IMPLEMENTATION LOGIC +// It is optionally expected by the safety controller to use an identification mechanism to map the VM Created by a providerSpec. +// These could be done using tag(s)/resource-groups etc. +// This logic is used by safety controller to delete orphan VMs which are not backed by any machine CRD +func (p *OpenstackDriver) CreateMachine(ctx context.Context, req *driver.CreateMachineRequest) (*driver.CreateMachineResponse, error) { + 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) + + ex, err := p.setupExecutor(ctx, req.MachineClass, req.Secret) + if err != nil { + return nil, err + } + server, err := ex.CreateMachine(ctx, req.Machine.Name, req.Secret.Data[cloudprovider.UserData]) if err != nil { klog.Errorf("machine creation for machine %q failed with: %v", req.Machine.Name, err) @@ -109,48 +122,58 @@ func (p *OpenstackDriver) DeleteMachine(ctx context.Context, req *driver.DeleteM 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) - // Check if incoming provider in the MachineClass is a provider we support - if req.MachineClass.Provider != openstackProvider { - err := fmt.Errorf("requested for Provider '%s', we only support '%s'", req.MachineClass.Provider, openstackProvider) - return nil, status.Error(codes.InvalidArgument, err.Error()) + ex, err := p.setupExecutor(ctx, req.MachineClass, req.Secret) + if err != nil { + return nil, err } - providerConfig, err := p.decodeProviderSpec(req.MachineClass.ProviderSpec) + err = ex.DeleteMachine(ctx, req.Machine.Name, req.Machine.Spec.ProviderID) if err != nil { - klog.V(2).Infof("decoding provider spec for machine class %q failed with: %v", req.MachineClass.Name, err) - return nil, status.Error(codes.InvalidArgument, err.Error()) - } - if err := validation.ValidateRequest(providerConfig, req.Secret); err != nil { - klog.V(2).Infof("validating request for machine class %q failed with: %v", req.MachineClass.Name, err) - return nil, status.Error(codes.InvalidArgument, err.Error()) + return nil, status.Error(mapErrorToCode(err), err.Error()) } + return &driver.DeleteMachineResponse{}, nil +} + +// GetMachineStatus handles a machine get status request +func (p *OpenstackDriver) GetMachineStatus(ctx context.Context, req *driver.GetMachineStatusRequest) (response *driver.GetMachineStatusResponse, err 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) - factory, err := client.NewFactoryFromSecret(ctx, req.Secret) + ex, err := p.setupExecutor(ctx, req.MachineClass, req.Secret) if err != nil { - klog.Errorf("failed to construct OpenStack client: %v", err) - return nil, status.Error(mapErrorToCode(err), fmt.Sprintf("failed to construct OpenStack client: %v", err)) + return nil, err } - ex, err := executor.NewExecutor(factory, providerConfig) - if err != nil { - klog.Errorf("failed to construct context for the request: %v", err) - return nil, status.Error(mapErrorToCode(err), fmt.Sprintf("failed to construct context for the request: %v", err)) + var machine *servers.Server + // Finding by ProviderID should be the common path, by name fallback for pre-creation + if req.Machine.Spec.ProviderID != "" { + klog.V(2).Infof("Finding Machine (%q) by ProviderID: %q", req.Machine.Name, req.Machine.Spec.ProviderID) + machine, err = ex.GetMachineByProviderID(ctx, req.Machine.Spec.ProviderID) + } else { + klog.V(2).Infof("Finding Machine by Tags and Name: %q", req.Machine.Name) + machine, err = ex.GetMachineByName(ctx, req.Machine.Name) } - err = ex.DeleteMachine(ctx, req.Machine.Name, req.Machine.Spec.ProviderID) if err != nil { + if errors.Is(err, executor.ErrNotFound) { + klog.V(2).Infof("Machine status: did not find Machine: %q", req.Machine.Name) + } else { + klog.Errorf("Failed to get Machine %q: %v", req.Machine.Name, err) + } return nil, status.Error(mapErrorToCode(err), err.Error()) } - return &driver.DeleteMachineResponse{}, nil -} -// GetMachineStatus handles a machine get status request -func (p *OpenstackDriver) GetMachineStatus(_ 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 is not implemented") + if machine.Name != req.Machine.Name { + klog.Errorf("Name of server with ProviderID %q (%q) does not match req.Machine.Name %q", + req.Machine.Spec.ProviderID, machine.Name, req.Machine.Name) + return nil, status.Error(codes.Internal, "Name and request machine name mismatch") + } - return nil, status.Error(codes.Unimplemented, "method not implemented") + return &driver.GetMachineStatusResponse{ + ProviderID: req.Machine.Spec.ProviderID, + NodeName: machine.Name, + }, nil } // ListMachines lists all the machines possibly created by a providerSpec @@ -161,33 +184,9 @@ func (p *OpenstackDriver) ListMachines(ctx context.Context, req *driver.ListMach 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) - // Check if incoming provider in the MachineClass is a provider we support - if req.MachineClass.Provider != openstackProvider { - err := fmt.Errorf("requested for Provider '%s', we only support '%s'", req.MachineClass.Provider, openstackProvider) - return nil, status.Error(codes.InvalidArgument, err.Error()) - } - - providerConfig, err := p.decodeProviderSpec(req.MachineClass.ProviderSpec) - if err != nil { - klog.Errorf("decoding provider spec for machine class %q failed with: %v", req.MachineClass.Name, err) - return nil, status.Error(codes.InvalidArgument, err.Error()) - } - - if err := validation.ValidateRequest(providerConfig, req.Secret); err != nil { - klog.Errorf("validating request for machine class %q failed with: %v", req.MachineClass.Name, err) - return nil, status.Error(codes.InvalidArgument, err.Error()) - } - - factory, err := client.NewFactoryFromSecret(ctx, req.Secret) + ex, err := p.setupExecutor(ctx, req.MachineClass, req.Secret) if err != nil { - klog.Errorf("failed to construct OpenStack client: %v", err) - return nil, status.Error(mapErrorToCode(err), fmt.Sprintf("failed to construct OpenStack client: %v", err)) - } - - ex, err := executor.NewExecutor(factory, providerConfig) - if err != nil { - klog.Errorf("failed to construct context for the request: %v", err) - return nil, status.Error(mapErrorToCode(err), fmt.Sprintf("failed to construct context for the request: %v", err)) + return nil, err } machines, err := ex.ListMachines(ctx) diff --git a/pkg/driver/executor/executor.go b/pkg/driver/executor/executor.go index 5387cbc3..4e921d21 100644 --- a/pkg/driver/executor/executor.go +++ b/pkg/driver/executor/executor.go @@ -113,7 +113,7 @@ func (ex *Executor) CreateMachine(ctx context.Context, machineName string, userD return err } - server, err = ex.getMachineByName(ctx, machineName) + server, err = ex.GetMachineByName(ctx, machineName) if err == nil { klog.Infof("found existing server [Name=%q, ID=%q]", machineName, server.ID) } else if !errors.Is(err, ErrNotFound) { @@ -510,10 +510,9 @@ func (ex *Executor) DeleteMachine(ctx context.Context, machineName, providerID s ) if !isEmptyString(ptr.To(providerID)) { - serverID := decodeProviderID(providerID) - server, err = ex.getMachineByID(ctx, serverID) + server, err = ex.GetMachineByProviderID(ctx, providerID) } else { - server, err = ex.getMachineByName(ctx, machineName) + server, err = ex.GetMachineByName(ctx, machineName) } if err == nil { @@ -640,9 +639,10 @@ func (ex *Executor) deleteVolume(ctx context.Context, machineName string) error return nil } -// getMachineByProviderID fetches the data for a server based on a provider-encoded ID. -func (ex *Executor) getMachineByID(ctx context.Context, serverID string) (*servers.Server, error) { - klog.V(2).Infof("finding server with [ID=%q]", serverID) +// GetMachineByProviderID fetches the data for a server based on a provider-encoded ID. +func (ex *Executor) GetMachineByProviderID(ctx context.Context, providerID string) (*servers.Server, error) { + klog.V(2).Infof("finding server with [ID=%q]", providerID) + serverID := decodeProviderID(providerID) server, err := ex.Compute.GetServer(ctx, serverID) if err != nil { klog.V(2).Infof("error finding server [ID=%q]: %v", serverID, err) @@ -669,13 +669,13 @@ func (ex *Executor) getMachineByID(ctx context.Context, serverID string) (*serve return nil, fmt.Errorf("could not find server [ID=%q]: %w", serverID, ErrNotFound) } -// getMachineByName returns a server that matches the following criteria: +// GetMachineByName returns a server that matches the following criteria: // a) has the same name as machineName // b) has the cluster and role tags as set in the machineClass // The current approach is weak because the tags are currently stored as server metadata. Later Nova versions allow // to store tags in a respective field and do a server-side filtering. To avoid incompatibility with older versions // we will continue making the filtering clientside. -func (ex *Executor) getMachineByName(ctx context.Context, machineName string) (*servers.Server, error) { +func (ex *Executor) GetMachineByName(ctx context.Context, machineName string) (*servers.Server, error) { searchClusterName, searchNodeRole, ok := findMandatoryTags(ex.Config.Spec.Tags) if !ok { klog.Warningf("getMachineByName operation can not proceed: cluster/role tags are missing for machine [Name=%q]", machineName) diff --git a/pkg/driver/executor/executor_test.go b/pkg/driver/executor/executor_test.go index cc639bad..7d4bf340 100644 --- a/pkg/driver/executor/executor_test.go +++ b/pkg/driver/executor/executor_test.go @@ -326,7 +326,7 @@ var _ = Describe("Executor", func() { }) }) - Context("#GetMachineStatus", func() { + Context("GetMachineByName", func() { var serverList []servers.Server BeforeEach(func() { @@ -337,32 +337,32 @@ var _ = Describe("Executor", func() { Name: "foo", }, { - ID: "id2", + ID: "id2", // No Match: Name yes, Tags no Name: "foo", }, { - ID: "id3", + ID: "id3", // No Match: Name no, Tags yes Name: "bar", Metadata: tags, }, { - ID: "id4", + ID: "id4", // No Match: Name no, Tags no Name: "baz", }, { - ID: "id5", + ID: "id5", // Match: Name + Tags Name: "lorem", Metadata: tags, }, { - ID: "id6", + ID: "id6", // Match: Name + Tags Name: "lorem", Metadata: tags, }, } }) - DescribeTable("#Status", + DescribeTable("finding by name behavior", func(name string, expectedID string, expectedErr error) { compute.EXPECT().ListServers(ctx, &servers.ListOpts{Name: name}).Return(serverList, nil) ex := Executor{ @@ -370,8 +370,9 @@ var _ = Describe("Executor", func() { Network: network, Config: cfg, } - server, err := ex.getMachineByName(ctx, name) + server, err := ex.GetMachineByName(ctx, name) if expectedErr != nil { + Expect(err).To(HaveOccurred()) Expect(errors.Is(err, expectedErr)).To(BeTrue()) } else { Expect(err).ToNot(HaveOccurred()) @@ -385,6 +386,88 @@ var _ = Describe("Executor", func() { ) }) + Context("GetMachineByID", func() { + var ( + serverID = "server-id-123" + providerID = encodeProviderID(region, serverID) + ) + + BeforeEach(func() { + cfg = &openstack.MachineProviderConfig{ + Spec: openstack.MachineProviderConfigSpec{ + Region: region, + SecurityGroups: nil, + Tags: tags, + NetworkID: networkID, + RootDiskSize: 0, + }, + } + }) + + DescribeTable("finding by ID behavior", + func( + setupMocks func(), + expectedErr error, + ) { + ex := Executor{ + Compute: compute, + Network: network, + Config: cfg, + } + + setupMocks() + + server, err := ex.GetMachineByProviderID(ctx, providerID) + if expectedErr != nil { + Expect(err).To(HaveOccurred()) + + if errors.Is(err, ErrNotFound) { + Expect(errors.Is(err, expectedErr)).To(BeTrue()) + } else { + Expect(err).To(MatchError(expectedErr)) + } + } else { + Expect(err).ToNot(HaveOccurred()) + Expect(server.ID).To(Equal(serverID)) + } + }, + + Entry("Should find the server if ID and tags match", + func() { + compute.EXPECT().GetServer(ctx, serverID).Return(&servers.Server{ + ID: serverID, + Metadata: tags, + }, nil) + }, + nil, + ), + + Entry("Should return ErrNotFound if server is not found via API", + func() { + compute.EXPECT().GetServer(ctx, serverID).Return(nil, gophercloud.ErrResourceNotFound{}) + }, + ErrNotFound, + ), + + Entry("Should return ErrNotFound if server is found but tags do not match", + func() { + compute.EXPECT().GetServer(ctx, serverID).Return(&servers.Server{ + ID: serverID, + Metadata: map[string]string{"wrong-tag": "wrong-value"}, + }, nil) + }, + ErrNotFound, + ), + + Entry("Should return other API errors", + func() { + compute.EXPECT().GetServer(ctx, serverID).Return(nil, errors.New("internal compute error")) + }, + errors.New("internal compute error"), + ), + ) + }) + Context("Delete", func() { var serverList []servers.Server