Skip to content
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

Jump to bottom

🏃 Update status.controlPlaneInitialized from cluster controller #1356

Merged
merged 1 commit into from
Sep 12, 2019
Merged

🏃 Update status.controlPlaneInitialized from cluster controller #1356

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
86 changes: 86 additions & 0 deletions controllers/cluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package controllers

import (
"context"
"fmt"
"path"
"sync"
"time"
Expand All @@ -27,6 +28,7 @@ import (
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
kerrors "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/client-go/tools/record"
"k8s.io/klog"
Expand All @@ -38,7 +40,9 @@ import (
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/source"
)

const (
Expand Down Expand Up @@ -66,6 +70,10 @@ type ClusterReconciler struct {
func (r *ClusterReconciler) SetupWithManager(mgr ctrl.Manager, options controller.Options) error {
c, err := ctrl.NewControllerManagedBy(mgr).
For(&clusterv1.Cluster{}).
Watches(
&source.Kind{Type: &clusterv1.Machine{}},
&handler.EnqueueRequestsFromMapFunc{ToRequests: handler.ToRequestsFunc(r.controlPlaneMachineToCluster)},
).
WithOptions(options).
Build(r)

Expand Down Expand Up @@ -129,6 +137,7 @@ func (r *ClusterReconciler) reconcile(ctx context.Context, cluster *clusterv1.Cl
reconciliationErrors := []error{
r.reconcileInfrastructure(ctx, cluster),
r.reconcileKubeconfig(ctx, cluster),
r.reconcileControlPlaneInitialized(ctx, cluster),
}

// Parse the errors, making sure we record if there is a RequeueAfterError.
Expand Down Expand Up @@ -287,3 +296,80 @@ func splitMachineList(list *clusterv1.MachineList) (*clusterv1.MachineList, *clu
}
return controlplanes, nodes
}

func (r *ClusterReconciler) reconcileControlPlaneInitialized(ctx context.Context, cluster *clusterv1.Cluster) error {
if cluster.Status.ControlPlaneInitialized {
return nil
}

machines, err := r.getMachinesInCluster(ctx, cluster.Namespace, cluster.Name)
if err != nil {
r.Log.Error(err, "error getting machines in cluster", "cluster", cluster.Name, "namespace", cluster.Namespace)
return err
}

for _, m := range machines {
if util.IsControlPlaneMachine(m) && m.Status.NodeRef != nil {
cluster.Status.ControlPlaneInitialized = true
return nil
}
}

return nil
}

// getMachinesInCluster returns all of the Machine objects that belong to the cluster
func (r *ClusterReconciler) getMachinesInCluster(ctx context.Context, namespace, name string) ([]*clusterv1.Machine, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vincepri @ncdc I created this function similar to this in machine controller. should move this as a common function elsewhere?

cluster-api/controllers/machine_controller.go

Lines 264 to 284 in 2a55573

func (r *MachineReconciler) getMachinesInCluster(ctx context.Context, namespace, name string) ([]*clusterv1.Machine, error) {
if name == "" {
return nil, nil
}
machineList := &clusterv1.MachineList{}
labels := map[string]string{clusterv1.MachineClusterLabelName: name}
if err := r.Client.List(ctx, machineList, client.InNamespace(namespace), client.MatchingLabels(labels)); err != nil {
return nil, errors.Wrap(err, "failed to list machines")
}
machines := []*clusterv1.Machine{}
for i := range machineList.Items {
m := &machineList.Items[i]
if m.DeletionTimestamp.IsZero() {
machines = append(machines, m)
}
}
return machines, nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please! You can change the signature so it takes in the Client. As for where to put it... I'm trying to avoid generic files/packages like util or helpers. Even though I just said let's avoid "helpers", maybe we put it in controllers/machine_helpers.go? @vincepri wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Either way should be fine, I'm also ok adding it in util/util.go for now and remove it later, there are a bunch of other similar functions that take a Controller Runtime Client and return something, so this wouldn't be anything new.

If we want to make it private, controllers/helpers.go or controllers/machine_helpers.go as you suggested sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lets address it in a separate pr after it is merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

Choose a reason for hiding this comment

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

#1414

if name == "" {
return nil, nil
}

machineList := &clusterv1.MachineList{}
labels := map[string]string{clusterv1.MachineClusterLabelName: name}

if err := r.Client.List(ctx, machineList, client.InNamespace(namespace), client.MatchingLabels(labels)); err != nil {
return nil, errors.Wrap(err, "failed to list machines")
}

machines := []*clusterv1.Machine{}
for i := range machineList.Items {
m := &machineList.Items[i]
if m.DeletionTimestamp.IsZero() {
machines = append(machines, m)
}
}
return machines, nil
}

// controlPlaneMachineToCluster is a handler.ToRequestsFunc to be used to enqueue requests for reconciliation
// for Cluster to update its status.controlPlaneInitialized field
func (r *ClusterReconciler) controlPlaneMachineToCluster(o handler.MapObject) []ctrl.Request {
m, ok := o.Object.(*clusterv1.Machine)
if !ok {
r.Log.Error(errors.New("did not get machine"), "got", fmt.Sprintf("%T", o.Object))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm almost hesitant to suggest this, but I wonder if we should consider a panic here - if we don't get the expected type, it's because of programmer error and the implication is that we'll never set ControlPlaneInitialized to true. It's probably easier to diagnose programmer error with a panic vs. digging through logs. Am I crazy? 😄

Copy link
Member

Choose a reason for hiding this comment

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

I don't disagree, I'm +1 to use Fatal or panic

Copy link
Member

Choose a reason for hiding this comment

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

This is what we are doing in other places, so we'd need to update all the existing use cases as well if we wanted to go down that path. (Not saying we shouldn't, just outside the scope of this PR).

return nil
}
if !util.IsControlPlaneMachine(m) {
return nil
}
if m.Status.NodeRef == nil {
Copy link
Member

Choose a reason for hiding this comment

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

We can do an OR and merge the two ifs above

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to leave it like this

return nil
}

cluster, err := util.GetClusterFromMetadata(context.TODO(), r.Client, m.ObjectMeta)
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this query and just enqueue the request from the label name, and assume the same namespace?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a code copy from the machine controller. The get is here so we can check control plane initialized below on line 365

Copy link
Member

Choose a reason for hiding this comment

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

The method name threw me off a little controlPlaneMachineToCluster, I'm fine leaving like this, but we could save an extra call and just reconcile again. In theory, it should be a noop.

Copy link
Contributor

Choose a reason for hiding this comment

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

It pulls from the cache, so it's not a network hit. I guess it comes down to whether or not you want to avoid enqueuing if you know up front it's not relevant (non control plane machine).

if err != nil {
r.Log.Error(err, "failed to get cluster", "machine", m.Name, "cluster", m.ClusterName, "namespace", m.Namespace)
return nil
}

if cluster.Status.ControlPlaneInitialized {
return nil
}

return []ctrl.Request{{
NamespacedName: types.NamespacedName{
Namespace: cluster.Namespace,
Name: cluster.Name,
},
}}
}
202 changes: 202 additions & 0 deletions controllers/cluster_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,21 @@ limitations under the License.
package controllers

import (
"reflect"
"testing"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/pointer"
clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha2"
"sigs.k8s.io/cluster-api/util/patch"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/log"
)

var _ = Describe("Cluster Reconciler", func() {
Expand Down Expand Up @@ -211,4 +219,198 @@ var _ = Describe("Cluster Reconciler", func() {
return instance.Finalizers
}, timeout).Should(BeEmpty())
})

It("Should successfully set Status.ControlPlaneInitialized on the cluster object if controlplane is ready", func() {
cluster := &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
GenerateName: "test-",
Namespace: v1.NamespaceDefault,
},
}

Expect(k8sClient.Create(ctx, cluster)).To(BeNil())
key := client.ObjectKey{Name: cluster.Name, Namespace: cluster.Namespace}
defer k8sClient.Delete(ctx, cluster)

// Wait for reconciliation to happen.
Eventually(func() bool {
if err := k8sClient.Get(ctx, key, cluster); err != nil {
return false
}
return len(cluster.Finalizers) > 0
}, timeout).Should(BeTrue())

machine := &clusterv1.Machine{
ObjectMeta: metav1.ObjectMeta{
GenerateName: "test-",
Namespace: v1.NamespaceDefault,
Labels: map[string]string{
clusterv1.MachineControlPlaneLabelName: cluster.Name,
clusterv1.MachineClusterLabelName: cluster.Name,
},
},
Spec: clusterv1.MachineSpec{
ProviderID: pointer.StringPtr("aws:///id-node-1"),
Bootstrap: clusterv1.Bootstrap{
Data: pointer.StringPtr(""),
},
},
}

Expect(k8sClient.Create(ctx, machine)).To(BeNil())
key = client.ObjectKey{Name: machine.Name, Namespace: machine.Namespace}
defer k8sClient.Delete(ctx, machine)

// wait for machine to be ready
Eventually(func() bool {
if err := k8sClient.Get(ctx, key, machine); err != nil {
return false
}
return len(machine.Finalizers) > 0
}, timeout).Should(BeTrue())

// patch machine noderef
patchHelper, err := patch.NewHelper(machine, k8sClient)
Expect(err).NotTo(HaveOccurred())

machine.Status.NodeRef = &v1.ObjectReference{Kind: "Node", Name: "test-node"}
Expect(patchHelper.Patch(ctx, machine)).Should(BeNil())

// Assertion
key = client.ObjectKey{Name: cluster.Name, Namespace: cluster.Namespace}
Eventually(func() bool {
if err := k8sClient.Get(ctx, key, cluster); err != nil {
return false
}
return cluster.Status.ControlPlaneInitialized
}, timeout).Should(BeTrue())
})
})

func TestClusterReconciler_machineToCluster(t *testing.T) {
cluster := &clusterv1.Cluster{
TypeMeta: metav1.TypeMeta{
Kind: "Cluster",
},
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Namespace: "test",
},
Spec: clusterv1.ClusterSpec{},
Status: clusterv1.ClusterStatus{},
}

controlPlaneWithNoderef := &clusterv1.Machine{
TypeMeta: metav1.TypeMeta{
Kind: "Machine",
},
ObjectMeta: metav1.ObjectMeta{
Name: "controlPlaneWithNoderef",
Labels: map[string]string{
clusterv1.MachineClusterLabelName: cluster.Name,
clusterv1.MachineControlPlaneLabelName: cluster.Name,
},
},
Status: clusterv1.MachineStatus{
NodeRef: &v1.ObjectReference{
Kind: "Node",
Namespace: "test-node",
},
},
}
controlPlaneWithoutNoderef := &clusterv1.Machine{
TypeMeta: metav1.TypeMeta{
Kind: "Machine",
},
ObjectMeta: metav1.ObjectMeta{
Name: "controlPlaneWithoutNoderef",
Labels: map[string]string{
clusterv1.MachineClusterLabelName: cluster.Name,
clusterv1.MachineControlPlaneLabelName: cluster.Name,
},
},
}
nonControlPlaneWithNoderef := &clusterv1.Machine{
TypeMeta: metav1.TypeMeta{
Kind: "Machine",
},
ObjectMeta: metav1.ObjectMeta{
Name: "nonControlPlaneWitNoderef",
Labels: map[string]string{
clusterv1.MachineClusterLabelName: cluster.Name,
},
},
Status: clusterv1.MachineStatus{
NodeRef: &v1.ObjectReference{
Kind: "Node",
Namespace: "test-node",
},
},
}
nonControlPlaneWithoutNoderef := &clusterv1.Machine{
TypeMeta: metav1.TypeMeta{
Kind: "Machine",
},
ObjectMeta: metav1.ObjectMeta{
Name: "nonControlPlaneWithoutNoderef",
Labels: map[string]string{
clusterv1.MachineClusterLabelName: cluster.Name,
},
},
}

tests := []struct {
name string
o handler.MapObject
want []ctrl.Request
}{
{
name: "controlplane machine, noderef is set, should return cluster",
o: handler.MapObject{
Meta: controlPlaneWithNoderef.GetObjectMeta(),
Object: controlPlaneWithNoderef,
},
want: []ctrl.Request{
{NamespacedName: client.ObjectKey{
Name: cluster.Name,
Namespace: cluster.Namespace,
}},
},
},
{
name: "controlplane machine, noderef is not set",
o: handler.MapObject{
Meta: controlPlaneWithoutNoderef.GetObjectMeta(),
Object: controlPlaneWithoutNoderef,
},
want: nil,
},
{
name: "not controlplane machine, noderef is set",
o: handler.MapObject{
Meta: nonControlPlaneWithNoderef.GetObjectMeta(),
Object: nonControlPlaneWithNoderef,
},
want: nil,
},
{
name: "not controlplane machine, noderef is not set",
o: handler.MapObject{
Meta: nonControlPlaneWithoutNoderef.GetObjectMeta(),
Object: nonControlPlaneWithoutNoderef,
},
want: nil,
},
tahsinrahman marked this conversation as resolved.
Show resolved Hide resolved
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
r := &ClusterReconciler{
Client: fake.NewFakeClient(cluster, controlPlaneWithNoderef, controlPlaneWithoutNoderef, nonControlPlaneWithNoderef, nonControlPlaneWithoutNoderef),
Log: log.Log,
}
if got := r.controlPlaneMachineToCluster(tt.o); !reflect.DeepEqual(got, tt.want) {
t.Errorf("controlPlaneMachineToCluster() = %v, want %v", got, tt.want)
}
})
}
}
1 change: 0 additions & 1 deletion controllers/machine_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,6 @@ func (r *MachineReconciler) reconcile(ctx context.Context, cluster *clusterv1.Cl
r.reconcileBootstrap(ctx, m),
r.reconcileInfrastructure(ctx, m),
r.reconcileNodeRef(ctx, cluster, m),
r.reconcileClusterStatus(ctx, cluster, m),
}

// Parse the errors, making sure we record if there is a RequeueAfterError.
Expand Down
27 changes: 0 additions & 27 deletions controllers/machine_controller_phases.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import (
"sigs.k8s.io/cluster-api/controllers/external"
capierrors "sigs.k8s.io/cluster-api/errors"
"sigs.k8s.io/cluster-api/util"
"sigs.k8s.io/cluster-api/util/patch"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/source"
Expand Down Expand Up @@ -236,29 +235,3 @@ func (r *MachineReconciler) reconcileInfrastructure(ctx context.Context, m *clus
m.Status.InfrastructureReady = true
return nil
}

// reconcileClusterStatus reconciles the status on the Cluster associated with Machines.
func (r *MachineReconciler) reconcileClusterStatus(ctx context.Context, cluster *clusterv1.Cluster, m *clusterv1.Machine) error {
if cluster == nil {
return nil
}

// If the Machine is a control plane, it has a NodeRef and it's ready,
// set the Status.ControlPlaneInitialized on the Cluster.
if util.IsControlPlaneMachine(m) && m.Status.NodeRef != nil {
if !cluster.Status.ControlPlaneInitialized {
patchHelper, err := patch.NewHelper(cluster, r.Client)
if err != nil {
return errors.Wrapf(err, "failed to create patch helper for Cluster %q in namespace %q",
cluster.Name, cluster.Namespace)
}
cluster.Status.ControlPlaneInitialized = true
if err := patchHelper.Patch(ctx, cluster); err != nil {
return errors.Wrapf(err, "failed to set Status.ControlPlaneInitialized on Cluster %q in namespace %q",
cluster.Name, cluster.Namespace)
}
}
}

return nil
}