Skip to content

Commit

Permalink
Address PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
JoelSpeed committed Jan 29, 2020
1 parent 19cac02 commit a89504f
Show file tree
Hide file tree
Showing 6 changed files with 46 additions and 49 deletions.
4 changes: 2 additions & 2 deletions api/v1alpha3/machinedeployment_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
"testing"

. "github.com/onsi/gomega"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/pointer"
)

Expand Down Expand Up @@ -85,7 +85,7 @@ func TestMachineDeploymentValidation(t *testing.T) {
g := NewWithT(t)
md := &MachineDeployment{
Spec: MachineDeploymentSpec{
Selector: v1.LabelSelector{
Selector: metav1.LabelSelector{
MatchLabels: tt.selectors,
},
Template: MachineTemplateSpec{
Expand Down
6 changes: 3 additions & 3 deletions api/v1alpha3/machinehealthcheck_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ type MachineHealthCheckSpec struct {
// +kubebuilder:validation:MinItems=1
UnhealthyConditions []UnhealthyCondition `json:"unhealthyConditions"`

// Any farther remediation is only allowed if at most "MaxUnhealthy" machines selected by
// Any further remediation is only allowed if at most "MaxUnhealthy" machines selected by
// "selector" are not healthy.
// +optional
MaxUnhealthy *intstr.IntOrString `json:"maxUnhealthy,omitempty"`
Expand Down Expand Up @@ -73,11 +73,11 @@ type UnhealthyCondition struct {
type MachineHealthCheckStatus struct {
// total number of machines counted by this machine health check
// +kubebuilder:validation:Minimum=0
ExpectedMachines *int `json:"expectedMachines"`
ExpectedMachines int `json:"expectedMachines"`

// total number of healthy machines counted by this machine health check
// +kubebuilder:validation:Minimum=0
CurrentHealthy *int `json:"currentHealthy" protobuf:"varint,4,opt,name=currentHealthy"`
CurrentHealthy int `json:"currentHealthy"`
}

// ANCHOR_END: MachineHealthCheckStatus
Expand Down
12 changes: 1 addition & 11 deletions api/v1alpha3/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion config/crd/bases/cluster.x-k8s.io_machinehealthchecks.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ spec:
anyOf:
- type: integer
- type: string
description: Any farther remediation is only allowed if at most "MaxUnhealthy"
description: Any further remediation is only allowed if at most "MaxUnhealthy"
machines selected by "selector" are not healthy.
x-kubernetes-int-or-string: true
selector:
Expand Down
27 changes: 12 additions & 15 deletions controllers/machinehealthcheck_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,14 @@ func (r *MachineHealthCheckReconciler) Reconcile(req ctrl.Request) (_ ctrl.Resul
}

// Error reading the object - requeue the request.
logger.Error(err, "Failed to fetch MachineHealthCheck")
return ctrl.Result{}, err
}

cluster, err := util.GetClusterByName(ctx, r.Client, m.ObjectMeta.Namespace, m.Spec.ClusterName)
cluster, err := util.GetClusterByName(ctx, r.Client, m.Namespace, m.Spec.ClusterName)
if err != nil {
return ctrl.Result{}, errors.Wrapf(err, "failed to get cluster %q for machine %q in namespace %q",
logger.Error(err, "Failed to fetch Cluster for MachineHealthCheck")
return ctrl.Result{}, errors.Wrapf(err, "failed to get Cluster %q for MachineHealthCheck %q in namespace %q",
m.Spec.ClusterName, m.Name, m.Namespace)
}

Expand All @@ -91,6 +93,7 @@ func (r *MachineHealthCheckReconciler) Reconcile(req ctrl.Request) (_ ctrl.Resul
// Initialize the patch helper
patchHelper, err := patch.NewHelper(m, r.Client)
if err != nil {
logger.Error(err, "Failed to build patch helper")
return ctrl.Result{}, err
}

Expand All @@ -117,19 +120,13 @@ func (r *MachineHealthCheckReconciler) Reconcile(req ctrl.Request) (_ ctrl.Resul
}

func (r *MachineHealthCheckReconciler) reconcile(_ context.Context, cluster *clusterv1.Cluster, m *clusterv1.MachineHealthCheck) (ctrl.Result, error) {
// If the Machine belongs to a cluster, add an owner reference.
if r.shouldAdopt(m) {
m.OwnerReferences = util.EnsureOwnerRef(m.OwnerReferences, metav1.OwnerReference{
APIVersion: clusterv1.GroupVersion.String(),
Kind: "Cluster",
Name: cluster.Name,
UID: cluster.UID,
})
}
// Ensure the MachineHealthCheck is owned by the Cluster it belongs to
m.OwnerReferences = util.EnsureOwnerRef(m.OwnerReferences, metav1.OwnerReference{
APIVersion: clusterv1.GroupVersion.String(),
Kind: "Cluster",
Name: cluster.Name,
UID: cluster.UID,
})

return ctrl.Result{}, fmt.Errorf("controller not yet implemented")
}

func (r *MachineHealthCheckReconciler) shouldAdopt(m *clusterv1.MachineHealthCheck) bool {
return !util.HasOwner(m.OwnerReferences, clusterv1.GroupVersion.String(), []string{"Cluster"})
}
44 changes: 27 additions & 17 deletions controllers/machinehealthcheck_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,27 +31,36 @@ import (
)

var _ = Describe("MachineHealthCheck Reconciler", func() {
namespace := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "mhc-test"}}
testCluster := &clusterv1.Cluster{ObjectMeta: metav1.ObjectMeta{Namespace: namespace.Name, Name: "test-cluster"}}
var namespace *corev1.Namespace
var testCluster *clusterv1.Cluster

var clusterName = "test-cluster"
var namespaceName = "mhc-test"

BeforeEach(func() {
namespace = &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespaceName}}
testCluster = &clusterv1.Cluster{ObjectMeta: metav1.ObjectMeta{Namespace: namespace.Name, Name: clusterName}}

By("Ensuring the namespace exists")
if err := k8sClient.Get(ctx, client.ObjectKey{Name: namespace.GetName()}, namespace.DeepCopy()); apierrors.IsNotFound(err) {
Expect(k8sClient.Create(ctx, namespace.DeepCopy())).To(Succeed())
err := k8sClient.Get(ctx, client.ObjectKey{Name: namespace.GetName()}, namespace)
if err != nil && apierrors.IsNotFound(err) {
Expect(k8sClient.Create(ctx, namespace)).To(Succeed())
} else {
Expect(err).ToNot(HaveOccurred())
}
By("Creating the Cluster")
Expect(k8sClient.Create(ctx, testCluster.DeepCopy())).To(Succeed())
Expect(k8sClient.Create(ctx, testCluster)).To(Succeed())
})

AfterEach(func() {
By("Deleting any MachineHealthChecks")
Expect(cleanupTestMachineHealthChecks(ctx, k8sClient)).To(Succeed())
By("Deleting the Cluster")
Expect(k8sClient.Delete(ctx, testCluster.DeepCopy())).To(Succeed())
Expect(k8sClient.Delete(ctx, testCluster)).To(Succeed())
// Ensure the cluster is actually gone before moving on
Eventually(func() error {
c := &clusterv1.Cluster{}
err := k8sClient.Get(ctx, client.ObjectKey{Namespace: testCluster.GetNamespace(), Name: testCluster.GetName()}, c)
err := k8sClient.Get(ctx, client.ObjectKey{Namespace: namespaceName, Name: clusterName}, c)
if err != nil && apierrors.IsNotFound(err) {
return nil
} else if err != nil {
Expand All @@ -72,11 +81,11 @@ var _ = Describe("MachineHealthCheck Reconciler", func() {
mhcToCreate := &clusterv1.MachineHealthCheck{
ObjectMeta: metav1.ObjectMeta{
Name: "test-mhc",
Namespace: namespace.ObjectMeta.Name,
Namespace: namespaceName,
Labels: ltc.original,
},
Spec: clusterv1.MachineHealthCheckSpec{
ClusterName: testCluster.ObjectMeta.Name,
ClusterName: clusterName,
UnhealthyConditions: []clusterv1.UnhealthyCondition{
{
Type: corev1.NodeReady,
Expand All @@ -99,15 +108,15 @@ var _ = Describe("MachineHealthCheck Reconciler", func() {
},
Entry("when no existing labels exist", labelTestCase{
original: map[string]string{},
expected: map[string]string{clusterv1.ClusterLabelName: testCluster.GetName()},
expected: map[string]string{clusterv1.ClusterLabelName: clusterName},
}),
Entry("when the label has the wrong value", labelTestCase{
original: map[string]string{clusterv1.ClusterLabelName: "wrong"},
expected: map[string]string{clusterv1.ClusterLabelName: testCluster.GetName()},
expected: map[string]string{clusterv1.ClusterLabelName: clusterName},
}),
Entry("without modifying other labels", labelTestCase{
original: map[string]string{"other": "label"},
expected: map[string]string{"other": "label", clusterv1.ClusterLabelName: testCluster.ObjectMeta.Name},
expected: map[string]string{"other": "label", clusterv1.ClusterLabelName: clusterName},
}),
)

Expand All @@ -123,11 +132,11 @@ var _ = Describe("MachineHealthCheck Reconciler", func() {
mhcToCreate := &clusterv1.MachineHealthCheck{
ObjectMeta: metav1.ObjectMeta{
Name: "test-mhc",
Namespace: namespace.ObjectMeta.Name,
Namespace: namespaceName,
OwnerReferences: ortc.original,
},
Spec: clusterv1.MachineHealthCheckSpec{
ClusterName: testCluster.ObjectMeta.Name,
ClusterName: clusterName,
UnhealthyConditions: []clusterv1.UnhealthyCondition{
{
Type: corev1.NodeReady,
Expand Down Expand Up @@ -171,7 +180,8 @@ func cleanupTestMachineHealthChecks(ctx context.Context, c client.Client) error
return err
}
for _, mhc := range mhcList.Items {
err = c.Delete(ctx, mhc.DeepCopy())
m := mhc
err = c.Delete(ctx, &m)
if err != nil {
return err
}
Expand All @@ -181,8 +191,8 @@ func cleanupTestMachineHealthChecks(ctx context.Context, c client.Client) error

func ownerReferenceForCluster(ctx context.Context, c *clusterv1.Cluster) metav1.OwnerReference {
// Fetch the cluster to populate the UID
cc := c.DeepCopy()
Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: cc.GetNamespace(), Name: cc.GetName()}, cc)).To(Succeed())
cc := &clusterv1.Cluster{}
Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: c.GetNamespace(), Name: c.GetName()}, cc)).To(Succeed())

return metav1.OwnerReference{
APIVersion: clusterv1.GroupVersion.String(),
Expand Down

0 comments on commit a89504f

Please sign in to comment.