Skip to content

OCPCLOUD-1769: Make machine phases public consts #1084

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

Closed
wants to merge 1 commit into from
Closed

OCPCLOUD-1769: Make machine phases public consts #1084

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 21 additions & 21 deletions pkg/controller/machine/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,27 +72,27 @@ const (
// indicates a state that will likely need to be fixed before progress can be made
// e.g Instance does NOT exist but Machine has providerID/address
// e.g Cloud service returns a 4xx response
phaseFailed = "Failed"
PhaseFailed = "Failed"

// Instance does NOT exist
// Machine has NOT been given providerID/address
phaseProvisioning = "Provisioning"
PhaseProvisioning = "Provisioning"

// Instance exists
// Machine has been given providerID/address
// Machine has NOT been given nodeRef
phaseProvisioned = "Provisioned"
PhaseProvisioned = "Provisioned"

// Instance exists
// Machine has been given providerID/address
// Machine has been given a nodeRef
phaseRunning = "Running"
PhaseRunning = "Running"

// Machine has a deletion timestamp
phaseDeleting = "Deleting"
PhaseDeleting = "Deleting"

// Hardcoded instance state set on machine failure
unknownInstanceState = "Unknown"
UnknownInstanceState = "Unknown"

skipWaitForDeleteTimeoutSeconds = 1
)
Expand Down Expand Up @@ -218,7 +218,7 @@ func (r *ReconcileMachine) Reconcile(ctx context.Context, request reconcile.Requ
}

if !m.ObjectMeta.DeletionTimestamp.IsZero() {
if err := r.updateStatus(ctx, m, phaseDeleting, nil, originalConditions); err != nil {
if err := r.updateStatus(ctx, m, PhaseDeleting, nil, originalConditions); err != nil {
return reconcile.Result{}, err
}

Expand Down Expand Up @@ -288,7 +288,7 @@ func (r *ReconcileMachine) Reconcile(ctx context.Context, request reconcile.Requ
}

if machineIsFailed(m) {
klog.Warningf("%v: machine has gone %q phase. It won't reconcile", machineName, phaseFailed)
klog.Warningf("%v: machine has gone %q phase. It won't reconcile", machineName, PhaseFailed)
return reconcile.Result{}, nil
}

Expand Down Expand Up @@ -335,14 +335,14 @@ func (r *ReconcileMachine) Reconcile(ctx context.Context, request reconcile.Requ

if !machineHasNode(m) {
// Requeue until we reach running phase
if err := r.updateStatus(ctx, m, phaseProvisioned, nil, originalConditions); err != nil {
if err := r.updateStatus(ctx, m, PhaseProvisioned, nil, originalConditions); err != nil {
return reconcile.Result{}, err
}
klog.Infof("%v: has no node yet, requeuing", machineName)
return reconcile.Result{RequeueAfter: requeueAfter}, nil
}

return reconcile.Result{}, r.updateStatus(ctx, m, phaseRunning, nil, originalConditions)
return reconcile.Result{}, r.updateStatus(ctx, m, PhaseRunning, nil, originalConditions)
}

// Instance does not exist but the machine has been given a providerID/address.
Expand All @@ -355,7 +355,7 @@ func (r *ReconcileMachine) Reconcile(ctx context.Context, request reconcile.Requ
"Instance not found on provider",
))

if err := r.updateStatus(ctx, m, phaseFailed, errors.New("Can't find created instance."), originalConditions); err != nil {
if err := r.updateStatus(ctx, m, PhaseFailed, errors.New("Can't find created instance."), originalConditions); err != nil {
return reconcile.Result{}, err
}
return reconcile.Result{}, nil
Expand All @@ -371,7 +371,7 @@ func (r *ReconcileMachine) Reconcile(ctx context.Context, request reconcile.Requ
// Machine resource created and instance does not exist yet.
if stringPointerDeref(m.Status.Phase) == "" {
klog.V(2).Infof("%v: setting phase to Provisioning and requeuing", machineName)
if err := r.updateStatus(ctx, m, phaseProvisioning, nil, originalConditions); err != nil {
if err := r.updateStatus(ctx, m, PhaseProvisioning, nil, originalConditions); err != nil {
return reconcile.Result{}, err
}
return reconcile.Result{}, nil
Expand All @@ -381,7 +381,7 @@ func (r *ReconcileMachine) Reconcile(ctx context.Context, request reconcile.Requ
if err := r.actuator.Create(ctx, m); err != nil {
klog.Warningf("%v: failed to create machine: %v", machineName, err)
if isInvalidMachineConfigurationError(err) {
if err := r.updateStatus(ctx, m, phaseFailed, err, originalConditions); err != nil {
if err := r.updateStatus(ctx, m, PhaseFailed, err, originalConditions); err != nil {
return reconcile.Result{}, err
}
return reconcile.Result{}, nil
Expand Down Expand Up @@ -447,7 +447,7 @@ func (r *ReconcileMachine) updateStatus(ctx context.Context, machine *machinev1.
// A call to Patch will mutate our local copy of the machine to match what is stored in the API.
// Before we make any changes to the status subresource on our local copy, we need to patch the object first,
// otherwise our local changes to the status subresource will be lost.
if phase == phaseFailed {
if phase == PhaseFailed {
err := r.patchFailedMachineInstanceAnnotation(ctx, machine)
if err != nil {
klog.Errorf("Failed to update machine %q: %v", machine.GetName(), err)
Expand All @@ -465,7 +465,7 @@ func (r *ReconcileMachine) updateStatus(ctx context.Context, machine *machinev1.
// Any updates to the status must be done after this point.
baseToPatch := client.MergeFrom(baseMachine)

if phase == phaseFailed {
if phase == PhaseFailed {
if err := r.overrideFailedMachineProviderStatusState(machine); err != nil {
klog.Errorf("Failed to update machine provider status %q: %v", machine.GetName(), err)
return err
Expand All @@ -475,7 +475,7 @@ func (r *ReconcileMachine) updateStatus(ctx context.Context, machine *machinev1.
machine.Status.Phase = &phase
machine.Status.ErrorReason = nil
machine.Status.ErrorMessage = nil
if phase == phaseFailed && failureCause != nil {
if phase == PhaseFailed && failureCause != nil {
var machineError *MachineError
if errors.As(failureCause, &machineError) {
machine.Status.ErrorReason = &machineError.Reason
Expand All @@ -501,7 +501,7 @@ func (r *ReconcileMachine) updateStatus(ctx context.Context, machine *machinev1.
// entries when there are failures.
// Only update when there is a change to the phase to avoid duplicating entries for
// individual machines.
if phaseChanged && phase != phaseDeleting {
if phaseChanged && phase != PhaseDeleting {
// Apart from deleting, update the transition metric
// Deleting would always end up in the infinite bucket
timeElapsed := r.now().Sub(machine.GetCreationTimestamp().Time).Seconds()
Expand All @@ -516,7 +516,7 @@ func (r *ReconcileMachine) patchFailedMachineInstanceAnnotation(ctx context.Cont
if machine.Annotations == nil {
machine.Annotations = map[string]string{}
}
machine.Annotations[MachineInstanceStateAnnotationName] = unknownInstanceState
machine.Annotations[MachineInstanceStateAnnotationName] = UnknownInstanceState
if err := r.Client.Patch(ctx, machine, baseToPatch); err != nil {
return err
}
Expand All @@ -543,14 +543,14 @@ func (r *ReconcileMachine) overrideFailedMachineProviderStatusState(machine *mac

// if the instanceState is set already, update it to unknown
if _, found, err := unstructured.NestedString(providerStatus, instanceStateField); err == nil && found {
if err := unstructured.SetNestedField(providerStatus, unknownInstanceState, instanceStateField); err != nil {
if err := unstructured.SetNestedField(providerStatus, UnknownInstanceState, instanceStateField); err != nil {
return fmt.Errorf("could not set %s: %v", instanceStateField, err)
}
}

// if the vmState is set already, update it to unknown
if _, found, err := unstructured.NestedString(providerStatus, vmStateField); err == nil && found {
if err := unstructured.SetNestedField(providerStatus, unknownInstanceState, vmStateField); err != nil {
if err := unstructured.SetNestedField(providerStatus, UnknownInstanceState, vmStateField); err != nil {
return fmt.Errorf("could not set %s: %v", instanceStateField, err)
}
}
Expand Down Expand Up @@ -621,7 +621,7 @@ func machineHasNode(machine *machinev1.Machine) bool {
}

func machineIsFailed(machine *machinev1.Machine) bool {
return stringPointerDeref(machine.Status.Phase) == phaseFailed
return stringPointerDeref(machine.Status.Phase) == PhaseFailed
}

func nodeIsUnreachable(node *corev1.Node) bool {
Expand Down
56 changes: 28 additions & 28 deletions pkg/controller/machine/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func TestReconcileRequest(t *testing.T) {
},
},
Status: machinev1.MachineStatus{
Phase: pointer.StringPtr(phaseProvisioning),
Phase: pointer.StringPtr(PhaseProvisioning),
},
}
machineProvisioned := machinev1.Machine{
Expand Down Expand Up @@ -346,7 +346,7 @@ func TestReconcileRequest(t *testing.T) {
deleteCallCount: 0,
result: reconcile.Result{},
error: false,
phase: phaseProvisioning,
phase: PhaseProvisioning,
},
},
{
Expand All @@ -359,7 +359,7 @@ func TestReconcileRequest(t *testing.T) {
deleteCallCount: 0,
result: reconcile.Result{RequeueAfter: requeueAfter},
error: false,
phase: phaseProvisioning,
phase: PhaseProvisioning,
},
},
{
Expand All @@ -372,7 +372,7 @@ func TestReconcileRequest(t *testing.T) {
deleteCallCount: 0,
result: reconcile.Result{RequeueAfter: requeueAfter},
error: false,
phase: phaseProvisioned,
phase: PhaseProvisioned,
},
},
{
Expand All @@ -385,7 +385,7 @@ func TestReconcileRequest(t *testing.T) {
deleteCallCount: 0,
result: reconcile.Result{},
error: false,
phase: phaseDeleting,
phase: PhaseDeleting,
},
},
{
Expand All @@ -398,7 +398,7 @@ func TestReconcileRequest(t *testing.T) {
deleteCallCount: 0,
result: reconcile.Result{},
error: false,
phase: phaseDeleting,
phase: PhaseDeleting,
},
},
{
Expand All @@ -411,7 +411,7 @@ func TestReconcileRequest(t *testing.T) {
deleteCallCount: 0,
result: reconcile.Result{},
error: false,
phase: phaseDeleting,
phase: PhaseDeleting,
},
},
{
Expand All @@ -424,7 +424,7 @@ func TestReconcileRequest(t *testing.T) {
deleteCallCount: 0,
result: reconcile.Result{},
error: false,
phase: phaseDeleting,
phase: PhaseDeleting,
},
},
{
Expand All @@ -437,7 +437,7 @@ func TestReconcileRequest(t *testing.T) {
deleteCallCount: 0,
result: reconcile.Result{},
error: false,
phase: phaseDeleting,
phase: PhaseDeleting,
},
},
{
Expand All @@ -450,7 +450,7 @@ func TestReconcileRequest(t *testing.T) {
deleteCallCount: 1,
result: reconcile.Result{RequeueAfter: requeueAfter},
error: false,
phase: phaseDeleting,
phase: PhaseDeleting,
},
},
{
Expand All @@ -463,7 +463,7 @@ func TestReconcileRequest(t *testing.T) {
deleteCallCount: 1,
result: reconcile.Result{},
error: false,
phase: phaseDeleting,
phase: PhaseDeleting,
},
},
{
Expand All @@ -476,7 +476,7 @@ func TestReconcileRequest(t *testing.T) {
deleteCallCount: 0,
result: reconcile.Result{},
error: false,
phase: phaseFailed, // A machine which does not exist but has providerID or addresses
phase: PhaseFailed, // A machine which does not exist but has providerID or addresses
},
},
{
Expand All @@ -489,7 +489,7 @@ func TestReconcileRequest(t *testing.T) {
deleteCallCount: 0,
result: reconcile.Result{},
error: false,
phase: phaseRunning,
phase: PhaseRunning,
},
},
}
Expand Down Expand Up @@ -576,27 +576,27 @@ func TestUpdateStatus(t *testing.T) {
}{
{
name: "when the status is not changed",
phase: phaseRunning,
phase: PhaseRunning,
err: nil,
annotations: nil,
conditions: defaultLifecycleConditions,
},
{
name: "when updating the phase to Failed",
phase: phaseFailed,
phase: PhaseFailed,
err: errors.New("test"),
annotations: map[string]string{
MachineInstanceStateAnnotationName: unknownInstanceState,
MachineInstanceStateAnnotationName: UnknownInstanceState,
},
updated: true,
conditions: defaultLifecycleConditions,
},
{
name: "when updating the phase to Failed with instanceState Set",
phase: phaseFailed,
phase: PhaseFailed,
err: errors.New("test"),
annotations: map[string]string{
MachineInstanceStateAnnotationName: unknownInstanceState,
MachineInstanceStateAnnotationName: UnknownInstanceState,
},
existingProviderStatus: `{"instanceState":"Running"}`,
expectedProviderStatus: `{"instanceState":"Unknown"}`,
Expand All @@ -605,10 +605,10 @@ func TestUpdateStatus(t *testing.T) {
},
{
name: "when updating the phase to Failed with vmState Set",
phase: phaseFailed,
phase: PhaseFailed,
err: errors.New("test"),
annotations: map[string]string{
MachineInstanceStateAnnotationName: unknownInstanceState,
MachineInstanceStateAnnotationName: UnknownInstanceState,
},
existingProviderStatus: `{"vmState":"Running"}`,
expectedProviderStatus: `{"vmState":"Unknown"}`,
Expand All @@ -617,10 +617,10 @@ func TestUpdateStatus(t *testing.T) {
},
{
name: "when updating the phase to Failed with state Set",
phase: phaseFailed,
phase: PhaseFailed,
err: errors.New("test"),
annotations: map[string]string{
MachineInstanceStateAnnotationName: unknownInstanceState,
MachineInstanceStateAnnotationName: UnknownInstanceState,
},
existingProviderStatus: `{"state":"Running"}`,
expectedProviderStatus: `{"state":"Running"}`,
Expand All @@ -629,7 +629,7 @@ func TestUpdateStatus(t *testing.T) {
},
{
name: "when adding a condition",
phase: phaseRunning,
phase: PhaseRunning,
err: nil,
annotations: nil,
conditions: machinev1.Conditions{
Expand All @@ -641,7 +641,7 @@ func TestUpdateStatus(t *testing.T) {
},
{
name: "when updating a condition",
phase: phaseRunning,
phase: PhaseRunning,
err: nil,
annotations: nil,
conditions: machinev1.Conditions{
Expand All @@ -656,7 +656,7 @@ func TestUpdateStatus(t *testing.T) {
},
{
name: "when the conditions do not change",
phase: phaseRunning,
phase: PhaseRunning,
err: nil,
annotations: nil,
conditions: machinev1.Conditions{
Expand Down Expand Up @@ -722,18 +722,18 @@ func TestUpdateStatus(t *testing.T) {
}

// Set the phase to Running initially
g.Expect(reconciler.updateStatus(context.TODO(), machine, phaseRunning, nil, machinev1.Conditions{})).To(Succeed())
g.Expect(reconciler.updateStatus(context.TODO(), machine, PhaseRunning, nil, machinev1.Conditions{})).To(Succeed())
// validate persisted object
got := machinev1.Machine{}
g.Expect(reconciler.Client.Get(context.TODO(), namespacedName, &got)).To(Succeed())
g.Expect(got.Status.Phase).ToNot(BeNil())
g.Expect(*got.Status.Phase).To(Equal(phaseRunning))
g.Expect(*got.Status.Phase).To(Equal(PhaseRunning))
lastUpdated := got.Status.LastUpdated
gotConditions := got.Status.Conditions
g.Expect(lastUpdated).ToNot(BeNil())
// validate passed object
g.Expect(machine.Status.Phase).ToNot(BeNil())
g.Expect(*machine.Status.Phase).To(Equal(phaseRunning))
g.Expect(*machine.Status.Phase).To(Equal(PhaseRunning))
objectLastUpdated := machine.Status.LastUpdated
g.Expect(objectLastUpdated).ToNot(BeNil())

Expand Down
Loading