Use ProviderID as a fallback for fetching the VM, InitializeMachine returns Uninitialized error code if VM is not found.
#173
Conversation
…d if not found. Use GetInstanceByID as fallback if getInstancesByTagsOrInstanceID does not succeed. Change error value returned if VM not found to codes.Uninitialized.
…ying to fetch instanceID
|
@thiyyakat Thank you for your contribution. |
gardener-robot
added
the
size/s
|
Thank you @thiyyakat for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below. |
rishabh-11
added
the
reviewed/ok-to-test
gardener-robot-ci-3
added
needs/ok-to-test
gardener-robot
added
size/m
rishabh-11
added
the
reviewed/ok-to-test
gardener-robot-ci-2
removed
the
reviewed/ok-to-test
pkg/aws/core.go
Outdated
| @@ -259,18 +259,20 @@ func (d *Driver) InitializeMachine(_ context.Context, request *driver.Initialize | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| instances, err := d.getInstancesFromMachineName(request.Machine.Name, providerSpec, request.Secret) | |||
| instances, err := d.getMatchingInstancesForMachine(request.Machine, providerSpec, request.Secret) | |||
There was a problem hiding this comment.
can we just pass providerSpec.Tags to getMachineInstancesByTagsAndStatus function? Inside the function you can extract cluster name and node role.
There was a problem hiding this comment.
There was actually no reason to pass the entire providerSpec. Instead only pass the service. At present in InitializeMachine you are constructing it twice which is not really required.
pkg/aws/core_util.go
Outdated
| } | ||
| if len(instances) == 0 { | ||
| errMessage := "AWS plugin is returning no VM instances backing this machine object" | ||
| return nil, status.Error(codes.NotFound, errMessage) | ||
| //if getMachineInstancesByTagsAndStatus returns an error, try fetching matching instances using ProviderID |
There was a problem hiding this comment.
This comment is misleading. If there was an error returned by getMachineInstancesByTagsAndStatus then you will not even come here but return much earlier. So change this comment.
pkg/aws/core_util.go
Outdated
| // getMatchingInstancesForMachine extracts AWS Instance object for a given machine | ||
| func (d *Driver) getMatchingInstancesForMachine(machine *v1alpha1.Machine, providerSpec *api.AWSProviderSpec, secret *corev1.Secret) ([]*ec2.Instance, error) { | ||
| var ( | ||
| clusterName string |
There was a problem hiding this comment.
considering that you will move the extraction of clusterName and nodeRole elsewhere there is no reason to define these variables and you can just use short-assign.
pkg/aws/core_util.go
Outdated
| for _, reservation := range runResult.Reservations { | ||
| instances = append(instances, reservation.Instances...) | ||
| // getMatchingInstancesForMachine extracts AWS Instance object for a given machine | ||
| func (d *Driver) getMatchingInstancesForMachine(machine *v1alpha1.Machine, providerSpec *api.AWSProviderSpec, secret *corev1.Secret) ([]*ec2.Instance, error) { |
There was a problem hiding this comment.
to prevent shadowing you can just define names for the return types so the function definition will become:
func (d *Driver) getMatchingInstancesForMachine(machine *v1alpha1.Machine, providerSpec *api.AWSProviderSpec, secret *corev1.Secret) (instances []*ec2.Instance, err error) {
}
pkg/aws/core.go
Outdated
| @@ -368,7 +369,7 @@ func (d *Driver) DeleteMachine(_ context.Context, req *driver.DeleteMachineReque | |||
|
|
|||
| } else { | |||
| // ProviderID doesn't exist, hence check for any existing machine and then delete if exists | |||
| instances, err = d.getInstancesFromMachineName(req.Machine.Name, providerSpec, req.Secret) | |||
| instances, err = d.getMatchingInstancesForMachine(req.Machine, providerSpec, req.Secret) | |||
There was a problem hiding this comment.
In the If condition you have already checked that the providerID is empty. Therefore just calling getMachineInstancesByTagsAndStatus should be sufficient, right? This also prevents recreating the service which you have already created once.
rishabh-11
added
the
reviewed/ok-to-test
gardener-robot-ci-3
removed
the
reviewed/ok-to-test
gardener-robot
added
reviewed/lgtm
gardener-robot-ci-3
added
the
reviewed/ok-to-test
gardener-robot-ci-3
removed
the
reviewed/ok-to-test
rishabh-11
added
reviewed/ok-to-test
gardener-robot
added
the
status/closed
What this PR does / why we need it:
This PR introduces a fallback for fetching the VM instances associated with a machine.
It also changes
InitializeMachineto return error codeUninitialized, instead ofNotFoundwhen instances cannot be found for the machine.Which issue(s) this PR fixes:
Fixes part of gardener/machine-controller-manager#933
Special notes for your reviewer:
The changes were manually tested by doing the following:
getInstancesByTagsOrInstanceID(), such thatinstanceIDis fetched by the fallbackgetInstanceByID(). Found that Machine was created successfully and went into Running state.codes.NotFoundfromgetMatchingInstancesForMachine(). On triggering machine creation, initialization was tried in a loop aftershortRetry.Release note: