Skip to content

Fail fast if machines enter Failed phase #182

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
Danil-Grigorev wants to merge 1 commit into from
Closed

Fail fast if machines enter Failed phase #182

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
1 change: 1 addition & 0 deletions pkg/framework/framework.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const (
// if MachineSet.Spec.Replicas field is set to nil
DefaultMachineSetReplicas = 0
MachinePhaseRunning = "Running"
MachinePhaseFailed = "Failed"
MachineRoleLabel = "machine.openshift.io/cluster-api-machine-role"
MachineTypeLabel = "machine.openshift.io/cluster-api-machine-type"
MachineAnnotationKey = "machine.openshift.io/machine"
Expand Down
16 changes: 11 additions & 5 deletions pkg/framework/machines.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,23 @@ import (
)

// FilterRunningMachines returns a slice of only those Machines in the input
// that are in the "Running" phase.
func FilterRunningMachines(machines []*mapiv1beta1.Machine) []*mapiv1beta1.Machine {
// that are in the "Running" phase. If some of the machine enters Failed phase,
// it will indicate this wil an error.
func FilterRunningMachines(machines []*mapiv1beta1.Machine) ([]*mapiv1beta1.Machine, error) {
var result []*mapiv1beta1.Machine

for i, m := range machines {
if m.Status.Phase != nil && *m.Status.Phase == MachinePhaseRunning {
result = append(result, machines[i])
if m.Status.Phase != nil {
switch *m.Status.Phase {
case MachinePhaseRunning:
result = append(result, machines[i])
case MachinePhaseFailed:
return nil, fmt.Errorf("Machine entered the Failed phase: %q, reason: %v", m.GetName(), m.Status.ErrorMessage)
}
Comment on lines +29 to +35
Copy link
Contributor

@JoelSpeed JoelSpeed Aug 5, 2020

Choose a reason for hiding this comment

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

One question that comes to mind is whether any of the tests rely on a Machine going into a failed phase, we need to make sure we aren't breaking those by doing this

Copy link
Author

@Danil-Grigorev Danil-Grigorev Aug 5, 2020

Choose a reason for hiding this comment

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

We are not testing that at the moment, so it is not a breaking change. Why wait for machines running if you are expecting them not to?

Copy link
Contributor

@JoelSpeed JoelSpeed Aug 5, 2020

Choose a reason for hiding this comment

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

Good question 😅 I just wondered if we were using this to check that a machine didn't become running. I think it's ok

}
}

return result
return result, nil
}

// GetMachine get a machine by its name from the default machine API namespace.
Expand Down
3 changes: 2 additions & 1 deletion pkg/framework/machinesets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,8 @@ func WaitForMachineSet(c client.Client, name string) {
name, len(machines), int(replicas))
}

running := FilterRunningMachines(machines)
running, err := FilterRunningMachines(machines)
Expect(err).ToNot(HaveOccurred())

// This could probably be smarter, but seems fine for now.
if len(running) != len(machines) {
Expand Down
3 changes: 2 additions & 1 deletion pkg/infra/webhooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,8 @@ var _ = Describe("[Feature:Machines] Webhooks", func() {
if err != nil {
return err
}
running := framework.FilterRunningMachines([]*mapiv1.Machine{m})
running, err := framework.FilterRunningMachines([]*mapiv1.Machine{m})
Expect(err).ToNot(HaveOccurred())
if len(running) == 0 {
return fmt.Errorf("machine not yet running")
}
Expand Down