Skip to content

Bug 1704587: Reconcile nodes on MHC changes #371

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

Merged
merged 1 commit into from
Aug 7, 2019
Merged

Bug 1704587: Reconcile nodes on MHC changes #371

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
89 changes: 86 additions & 3 deletions pkg/controller/machinehealthcheck/machinehealthcheck_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package machinehealthcheck
import (
"context"
golangerrors "errors"
"fmt"
"time"

"github.com/golang/glog"
Expand Down Expand Up @@ -37,11 +38,12 @@ const (
// Add creates a new MachineHealthCheck Controller and adds it to the Manager. The Manager will set fields on the Controller
// and start it when the Manager is started.
func Add(mgr manager.Manager, opts manager.Options) error {
return add(mgr, newReconciler(mgr, opts))
r := newReconciler(mgr, opts)
return add(mgr, r, r.nodeRequestsFromMachineHealthCheck)
}

// newReconciler returns a new reconcile.Reconciler
func newReconciler(mgr manager.Manager, opts manager.Options) reconcile.Reconciler {
func newReconciler(mgr manager.Manager, opts manager.Options) *ReconcileMachineHealthCheck {
return &ReconcileMachineHealthCheck{
client: mgr.GetClient(),
scheme: mgr.GetScheme(),
Expand All @@ -50,12 +52,21 @@ func newReconciler(mgr manager.Manager, opts manager.Options) reconcile.Reconcil
}

// add adds a new Controller to mgr with r as the reconcile.Reconciler
func add(mgr manager.Manager, r reconcile.Reconciler) error {
func add(mgr manager.Manager, r reconcile.Reconciler, mapFn handler.ToRequestsFunc) error {
// Create a new controller
c, err := controller.New("machinehealthcheck-controller", mgr, controller.Options{Reconciler: r})
if err != nil {
return err
}

// Watch MachineHealthChecks and enqueue reconcile.Request for the backed nodes.
// This is useful to trigger remediation when a machineHealCheck is created against
// a node which is already unhealthy and is not able to receive status updates.
err = c.Watch(&source.Kind{Type: &healthcheckingv1alpha1.MachineHealthCheck{}}, &handler.EnqueueRequestsFromMapFunc{ToRequests: mapFn})
if err != nil {
return err
}

return c.Watch(&source.Kind{Type: &corev1.Node{}}, &handler.EnqueueRequestForObject{})
}

Expand Down Expand Up @@ -142,6 +153,78 @@ func (r *ReconcileMachineHealthCheck) Reconcile(request reconcile.Request) (reco
return reconcile.Result{}, nil
}

func (r *ReconcileMachineHealthCheck) nodeRequestsFromMachineHealthCheck(o handler.MapObject) []reconcile.Request {
glog.V(3).Infof("Watched machineHealthCheck event, finding nodes to reconcile.Request...")
mhc := &healthcheckingv1alpha1.MachineHealthCheck{}
if err := r.client.Get(
context.Background(),
client.ObjectKey{
Namespace: o.Meta.GetNamespace(),
Name: o.Meta.GetName(),
},
mhc,
); err != nil {
glog.Errorf("No-op: Unable to retrieve mhc %s/%s from store: %v", o.Meta.GetNamespace(), o.Meta.GetName(), err)
return []reconcile.Request{}
}

if mhc.DeletionTimestamp != nil {
glog.V(3).Infof("No-op: mhc %q is being deleted", o.Meta.GetName())
return []reconcile.Request{}
}

// get nodes covered by then mhc
nodeNames, err := r.getNodeNamesForMHC(*mhc)
if err != nil {
glog.Errorf("No-op: failed to get nodes for mhc %q", o.Meta.GetName())
return []reconcile.Request{}
}
if nodeNames != nil {
var requests []reconcile.Request
for _, nodeName := range nodeNames {
// convert to namespacedName to satisfy type Request struct
nodeNamespacedName := client.ObjectKey{
Name: string(nodeName),
}
requests = append(requests, reconcile.Request{NamespacedName: nodeNamespacedName})
}
return requests
}
return []reconcile.Request{}
}

func (r *ReconcileMachineHealthCheck) getNodeNamesForMHC(mhc healthcheckingv1alpha1.MachineHealthCheck) ([]types.NodeName, error) {
machineList := &mapiv1.MachineList{}
selector, err := metav1.LabelSelectorAsSelector(&mhc.Spec.Selector)
if err != nil {
return nil, fmt.Errorf("failed to build selector")
}
options := client.ListOptions{
LabelSelector: selector,
}

if err := r.client.List(context.Background(),
machineList,
client.UseListOptions(options.InNamespace(mhc.GetNamespace()))); err != nil {
return nil, fmt.Errorf("failed to list machines: %v", err)
}

if len(machineList.Items) < 1 {
return nil, nil
}

var nodeNames []types.NodeName
for _, machine := range machineList.Items {
if machine.Status.NodeRef != nil {
nodeNames = append(nodeNames, types.NodeName(machine.Status.NodeRef.Name))
}
}
if len(nodeNames) < 1 {
return nil, nil
}
return nodeNames, nil
}

// This is set so the fake client can be used for unit test. See:
// https://github.com/kubernetes-sigs/controller-runtime/issues/168
func getMachineHealthCheckListOptions() *client.ListOptions {
Expand Down
156 changes: 151 additions & 5 deletions pkg/controller/machinehealthcheck/machinehealthcheck_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ import (
"testing"
"time"

mapiv1alpha1 "github.com/openshift/cluster-api/pkg/apis/machine/v1beta1"
"sigs.k8s.io/controller-runtime/pkg/client"

"sigs.k8s.io/controller-runtime/pkg/handler"

mapiv1beta1 "github.com/openshift/cluster-api/pkg/apis/machine/v1beta1"
healthcheckingv1alpha1 "github.com/openshift/machine-api-operator/pkg/apis/healthchecking/v1alpha1"
"github.com/openshift/machine-api-operator/pkg/util/conditions"
maotesting "github.com/openshift/machine-api-operator/pkg/util/testing"
Expand All @@ -31,14 +35,14 @@ const (

func init() {
// Add types to scheme
mapiv1alpha1.AddToScheme(scheme.Scheme)
mapiv1beta1.AddToScheme(scheme.Scheme)
healthcheckingv1alpha1.AddToScheme(scheme.Scheme)
}

func TestHasMatchingLabels(t *testing.T) {
machine := maotesting.NewMachine("machine", "node")
testsCases := []struct {
machine *mapiv1alpha1.Machine
machine *mapiv1beta1.Machine
machineHealthCheck *healthcheckingv1alpha1.MachineHealthCheck
expected bool
}{
Expand Down Expand Up @@ -180,7 +184,7 @@ func testReconcile(t *testing.T, remediationWaitTime time.Duration, initObjects
machineUnhealthyForTooLong := maotesting.NewMachine("machineUnhealthyForTooLong", nodeUnhealthyForTooLong.Name)

testsCases := []struct {
machine *mapiv1alpha1.Machine
machine *mapiv1beta1.Machine
node *corev1.Node
remediationStrategy healthcheckingv1alpha1.RemediationStrategyType
expected expectedReconcile
Expand Down Expand Up @@ -312,7 +316,7 @@ func TestHasMachineSetOwner(t *testing.T) {
machineWithNoMachineSet.OwnerReferences = nil

testsCases := []struct {
machine *mapiv1alpha1.Machine
machine *mapiv1beta1.Machine
expected bool
}{
{
Expand Down Expand Up @@ -385,3 +389,145 @@ func TestApplyRemediationReboot(t *testing.T) {
t.Errorf("Expected: node to have reboot annotion %s, got: %v", machineRebootAnnotationKey, node.Annotations)
}
}

func TestGetNodeNamesForMHC(t *testing.T) {
testCases := []struct {
mhc healthcheckingv1alpha1.MachineHealthCheck
machines []*mapiv1beta1.Machine
expectedNodeNames []types.NodeName
}{
{
mhc: *maotesting.NewMachineHealthCheck("matchNodes"),
machines: []*mapiv1beta1.Machine{
maotesting.NewMachine("test", "node1"),
maotesting.NewMachine("test2", "node2"),
},
expectedNodeNames: []types.NodeName{
"node1",
"node2",
},
},
{
mhc: *&healthcheckingv1alpha1.MachineHealthCheck{
ObjectMeta: metav1.ObjectMeta{
Name: "noMatchingMachines",
Namespace: namespace,
},
TypeMeta: metav1.TypeMeta{
Kind: "MachineHealthCheck",
},
Spec: healthcheckingv1alpha1.MachineHealthCheckSpec{
Selector: metav1.LabelSelector{
MatchLabels: map[string]string{
"no": "match",
},
},
},
Status: healthcheckingv1alpha1.MachineHealthCheckStatus{},
},
machines: []*mapiv1beta1.Machine{
maotesting.NewMachine("test", "node1"),
maotesting.NewMachine("test2", "node2"),
},
expectedNodeNames: nil,
},
{
mhc: *maotesting.NewMachineHealthCheck("noNodeRefs"),
machines: []*mapiv1beta1.Machine{
maotesting.NewMachine("test", ""),
maotesting.NewMachine("test2", ""),
},
expectedNodeNames: nil,
},
}
for _, tc := range testCases {
objects := []runtime.Object{}
for i := range tc.machines {
objects = append(objects, runtime.Object(tc.machines[i]))
}
r := newFakeReconciler(objects...)
nodeNames, err := r.getNodeNamesForMHC(tc.mhc)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if !reflect.DeepEqual(nodeNames, tc.expectedNodeNames) {
t.Errorf("Expected: %v, got: %v", tc.expectedNodeNames, nodeNames)
}
}
}

func TestNodeRequestsFromMachineHealthCheck(t *testing.T) {
testCases := []struct {
mhc healthcheckingv1alpha1.MachineHealthCheck
machines []*mapiv1beta1.Machine
expectedRequests []reconcile.Request
}{
{
mhc: *maotesting.NewMachineHealthCheck("matchNodes"),
machines: []*mapiv1beta1.Machine{
maotesting.NewMachine("test", "node1"),
maotesting.NewMachine("test2", "node2"),
},
expectedRequests: []reconcile.Request{
{
NamespacedName: client.ObjectKey{
Name: string("node1"),
},
},
{
NamespacedName: client.ObjectKey{
Name: string("node2"),
},
},
},
},
{
mhc: *&healthcheckingv1alpha1.MachineHealthCheck{
ObjectMeta: metav1.ObjectMeta{
Name: "noMatchingMachines",
Namespace: namespace,
},
TypeMeta: metav1.TypeMeta{
Kind: "MachineHealthCheck",
},
Spec: healthcheckingv1alpha1.MachineHealthCheckSpec{
Selector: metav1.LabelSelector{
MatchLabels: map[string]string{
"no": "match",
},
},
},
Status: healthcheckingv1alpha1.MachineHealthCheckStatus{},
},
machines: []*mapiv1beta1.Machine{
maotesting.NewMachine("test", "node1"),
maotesting.NewMachine("test2", "node2"),
},
expectedRequests: []reconcile.Request{},
},
{
mhc: *maotesting.NewMachineHealthCheck("noNodeRefs"),
machines: []*mapiv1beta1.Machine{
maotesting.NewMachine("test", ""),
maotesting.NewMachine("test2", ""),
},
expectedRequests: []reconcile.Request{},
},
}
for _, tc := range testCases {
objects := []runtime.Object{}
for i := range tc.machines {
objects = append(objects, runtime.Object(tc.machines[i]))
}
objects = append(objects, runtime.Object(&tc.mhc))
r := newFakeReconciler(objects...)
o := handler.MapObject{
Meta: tc.mhc.GetObjectMeta(),
Object: &tc.mhc,
}
requests := r.nodeRequestsFromMachineHealthCheck(o)
if !reflect.DeepEqual(requests, tc.expectedRequests) {
t.Errorf("Expected: %v, got: %v", tc.expectedRequests, requests)
}
}
}
9 changes: 6 additions & 3 deletions pkg/util/testing/testing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func NewNode(name string, ready bool) *corev1.Node {

// NewMachine returns new machine object that can be used for testing
func NewMachine(name string, nodeName string) *mapiv1.Machine {
return &mapiv1.Machine{
m := &mapiv1.Machine{
TypeMeta: metav1.TypeMeta{Kind: "Machine"},
ObjectMeta: metav1.ObjectMeta{
Annotations: make(map[string]string),
Expand All @@ -114,13 +114,16 @@ func NewMachine(name string, nodeName string) *mapiv1.Machine {
OwnerReferences: []metav1.OwnerReference{{Kind: "MachineSet"}},
},
Spec: mapiv1.MachineSpec{},
Status: mapiv1.MachineStatus{
}
if nodeName != "" {
m.Status = mapiv1.MachineStatus{
NodeRef: &corev1.ObjectReference{
Name: nodeName,
Namespace: metav1.NamespaceNone,
},
},
}
}
return m
}

// NewMachineHealthCheck returns new MachineHealthCheck object that can be used for testing
Expand Down