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.

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

[0.1] Delete machines #1180

Merged
merged 10 commits into from
Jul 24, 2019
1 change: 1 addition & 0 deletions pkg/controller/cluster/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ go_library(
"//pkg/util:go_default_library",
"//vendor/github.com/pkg/errors:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/client-go/util/retry:go_default_library",
Expand Down
150 changes: 136 additions & 14 deletions pkg/controller/cluster/cluster_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,20 @@ package cluster

import (
"context"
"time"

"github.com/pkg/errors"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/tools/pager"
"k8s.io/client-go/util/retry"
"k8s.io/klog"
clusterv1 "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
clusterv1alpha1 "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
"sigs.k8s.io/cluster-api/pkg/client/clientset_generated/clientset/typed/cluster/v1alpha1"
controllerError "sigs.k8s.io/cluster-api/pkg/controller/error"
"sigs.k8s.io/cluster-api/pkg/util"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand All @@ -37,15 +42,30 @@ import (
"sigs.k8s.io/controller-runtime/pkg/source"
)

const deleteRequeueAfter = 5 * time.Second

var DefaultActuator Actuator

func AddWithActuator(mgr manager.Manager, actuator Actuator) error {
return add(mgr, newReconciler(mgr, actuator))
reconciler, err := newReconciler(mgr, actuator)
if err != nil {
return err
}

return add(mgr, reconciler)
}

// newReconciler returns a new reconcile.Reconciler
func newReconciler(mgr manager.Manager, actuator Actuator) reconcile.Reconciler {
return &ReconcileCluster{Client: mgr.GetClient(), scheme: mgr.GetScheme(), actuator: actuator}
func newReconciler(mgr manager.Manager, actuator Actuator) (reconcile.Reconciler, error) {
cclient, err := v1alpha1.NewForConfig(mgr.GetConfig())
if err != nil {
return nil, errors.WithStack(err)
}
return &ReconcileCluster{
Client: mgr.GetClient(),
clusterClient: cclient,
scheme: mgr.GetScheme(),
actuator: actuator}, nil
}

// add adds a new Controller to mgr with r as the reconcile.Reconciler
Expand All @@ -70,8 +90,10 @@ var _ reconcile.Reconciler = &ReconcileCluster{}
// ReconcileCluster reconciles a Cluster object
type ReconcileCluster struct {
client.Client
scheme *runtime.Scheme
actuator Actuator
// TODO: remove this once the controller-runtime Client has pagination support
clusterClient v1alpha1.ClusterV1alpha1Interface
scheme *runtime.Scheme
actuator Actuator
}

// +kubebuilder:rbac:groups=cluster.k8s.io,resources=clusters,verbs=get;list;watch;create;update;patch;delete
Expand All @@ -85,7 +107,7 @@ func (r *ReconcileCluster) Reconcile(request reconcile.Request) (reconcile.Resul
return reconcile.Result{}, nil
}
// Error reading the object - requeue the request.
return reconcile.Result{}, err
return reconcile.Result{}, errors.WithStack(err)
}

name := cluster.Name
Expand All @@ -107,7 +129,7 @@ func (r *ReconcileCluster) Reconcile(request reconcile.Request) (reconcile.Resul
if len(cluster.Finalizers) > finalizerCount {
if err := r.Update(context.Background(), cluster); err != nil {
klog.Infof("Failed to add finalizer to cluster %q: %v", name, err)
return reconcile.Result{}, err
return reconcile.Result{}, errors.WithStack(err)
}

// Since adding the finalizer updates the object return to avoid later update issues.
Expand All @@ -119,18 +141,47 @@ func (r *ReconcileCluster) Reconcile(request reconcile.Request) (reconcile.Resul
if !cluster.ObjectMeta.DeletionTimestamp.IsZero() {
// no-op if finalizer has been removed.
if !util.Contains(cluster.ObjectMeta.Finalizers, clusterv1.ClusterFinalizer) {
klog.Infof("reconciling cluster object %v causes a no-op as there is no finalizer.", name)
klog.Infof("Reconciling cluster object %v causes a no-op as there is no finalizer.", name)
return reconcile.Result{}, nil
}

klog.Infof("reconciling cluster object %v triggers delete.", name)
children, err := r.listChildren(context.Background(), cluster)
if err != nil {
klog.Infof("Failed to list dependent objects of cluster %s/%s: %v", cluster.ObjectMeta.Namespace, cluster.ObjectMeta.Name, err)
liztio marked this conversation as resolved.
Show resolved Hide resolved
return reconcile.Result{}, errors.WithStack(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need WithStack and instead we can return err directly because r.listChildren is our function, and it properly wraps/stacks all the errors it returns. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll be honest, I have no idea what WithStack does, I added it because you suggested it a few blocks up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you only need to add it once?

Copy link
Contributor

Choose a reason for hiding this comment

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

By default, errors in Go do not include any stack trace information. Without it, it makes it very difficult to determine the source of an error (i.e. source code file & line), especially if the same error can be created in multiple places. github.com/pkg/errors makes it much easier to embed that stack information, and it includes a special custom format printer to print out the stack trace information.

The general guidance I've used in the past is that every line of code you write in a project that returns an error must either already have stack info, or you must wrap it. For first class functions (those defined in your project), you can/should assume that any errors they return are already wrapped. When you are invoking functions from vendored libraries, you can either check to see if they use github.com/pkg/errors, or you can just wrap.

}

if len(children) > 0 {
klog.Infof("Deleting cluster %s: %d children still exist, will requeue", name, len(children))
for _, child := range children {
accessor, err := meta.Accessor(child)
if err != nil {
return reconcile.Result{}, errors.Wrapf(err, "couldn't create accessor for %T", child)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we aggregate all errors in this for loop so we can try to delete as much as we can?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The deletion happens in a later step, I think Accessor is unlikely to fail. But it might be a good idea to aggregate the errors for the actual deletion loop

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that Accessor is unlikely to fail, but I'd still like to include it in errList instead of returning early.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is my only remaining comment

}

if accessor.GetDeletionTimestamp() != nil {
continue
}

gvk := child.GetObjectKind().GroupVersionKind().String()

klog.V(4).Infof("Deleting cluster %s: Deleting %s %s", name, gvk, accessor.GetName())
if err := r.Delete(context.Background(), child, client.PropagationPolicy(metav1.DeletePropagationForeground)); err != nil {
return reconcile.Result{}, errors.Wrapf(err, "deleting cluster %s: failed to delete %s %s", name, gvk, accessor.GetName())
}
}

return reconcile.Result{Requeue: true, RequeueAfter: deleteRequeueAfter}, nil
}

klog.Infof("Reconciling cluster object %v triggers delete.", name)
if err := r.actuator.Delete(cluster); err != nil {
klog.Errorf("Error deleting cluster object %v; %v", name, err)
return reconcile.Result{}, err
return reconcile.Result{}, errors.WithStack(err)
}

// Remove finalizer on successful deletion.
klog.Infof("cluster object %v deletion successful, removing finalizer.", name)
klog.Infof("Cluster object %v deletion successful, removing finalizer.", name)
err = retry.RetryOnConflict(retry.DefaultRetry, func() error {
// It's possible the actuator's Delete call modified the cluster. We can't guarantee that every provider
// updated the in memory cluster object with the latest copy of the cluster, so try to get a fresh copy.
Expand All @@ -151,20 +202,91 @@ func (r *ReconcileCluster) Reconcile(request reconcile.Request) (reconcile.Resul
})
if err != nil {
klog.Errorf("Error removing finalizer from cluster object %v; %v", name, err)
return reconcile.Result{}, err
return reconcile.Result{}, errors.WithStack(err)
}

return reconcile.Result{}, nil
}

klog.Infof("reconciling cluster object %v triggers idempotent reconcile.", name)
klog.Infof("Reconciling cluster object %v triggers idempotent reconcile.", name)
if err := r.actuator.Reconcile(cluster); err != nil {
if requeueErr, ok := errors.Cause(err).(controllerError.HasRequeueAfterError); ok {
klog.Infof("Actuator returned requeue-after error: %v", requeueErr)
return reconcile.Result{Requeue: true, RequeueAfter: requeueErr.GetRequeueAfter()}, nil
}
klog.Errorf("Error reconciling cluster object %v; %v", name, err)
return reconcile.Result{}, err
return reconcile.Result{}, errors.WithStack(err)
}
return reconcile.Result{}, nil
}

// listChildren returns a list of Deployments, Sets, and Machines than have an ownerref to the given cluster
func (r *ReconcileCluster) listChildren(ctx context.Context, cluster *clusterv1.Cluster) ([]runtime.Object, error) {
var children []runtime.Object

ns := cluster.GetNamespace()
opts := metav1.ListOptions{
LabelSelector: labels.FormatLabels(
map[string]string{clusterv1.MachineClusterLabelName: cluster.GetName()},
),
}

dfunc := func(ctx context.Context, m metav1.ListOptions) (runtime.Object, error) {
return r.clusterClient.MachineDeployments(ns).List(m)
}
sfunc := func(ctx context.Context, m metav1.ListOptions) (runtime.Object, error) {
return r.clusterClient.MachineSets(ns).List(m)
}
mfunc := func(ctx context.Context, m metav1.ListOptions) (runtime.Object, error) {
return r.clusterClient.Machines(ns).List(m)
}

eachFunc := func(o runtime.Object) error {
acc, err := meta.Accessor(o)
if err != nil {
return err
}

if pointsTo(acc.GetOwnerReferences(), &cluster.ObjectMeta) {
children = append(children, o.DeepCopyObject())
liztio marked this conversation as resolved.
Show resolved Hide resolved
}

return nil
}

deployments, err := pager.New(dfunc).List(ctx, opts)
liztio marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return []runtime.Object{}, errors.Wrapf(err, "failed to list MachineDeployments in %s/%s", ns, cluster.Name)
}
if err := meta.EachListItem(deployments, eachFunc); err != nil {
return []runtime.Object{}, errors.Wrapf(err, "couldn't iterate MachinesDeployments for cluster %s/%s", ns, cluster.Name)
liztio marked this conversation as resolved.
Show resolved Hide resolved
}

sets, err := pager.New(sfunc).List(ctx, opts)
if err != nil {
return []runtime.Object{}, errors.Wrapf(err, "failed to list MachineSets in %s/%s", ns, cluster.Name)
}
if err := meta.EachListItem(sets, eachFunc); err != nil {
return []runtime.Object{}, errors.Wrapf(err, "couldn't iterate MachineSets for cluster %s/%s", ns, cluster.Name)
}

machines, err := pager.New(mfunc).List(ctx, opts)
if err != nil {
return []runtime.Object{}, errors.Wrapf(err, "failed to list Machines in %s/%s", ns, cluster.Name)
}
if err := meta.EachListItem(machines, eachFunc); err != nil {
return []runtime.Object{}, errors.Wrapf(err, "couldn't iterate Machines for cluster %s/%s", ns, cluster.Name)
liztio marked this conversation as resolved.
Show resolved Hide resolved
}

return children, nil
}

func pointsTo(refs []metav1.OwnerReference, target *metav1.ObjectMeta) bool {
liztio marked this conversation as resolved.
Show resolved Hide resolved
for _, ref := range refs {
if ref.UID == target.UID {
liztio marked this conversation as resolved.
Show resolved Hide resolved
return true
}
}

return false
}
77 changes: 77 additions & 0 deletions pkg/controller/cluster/cluster_controller_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
Copyright 2018 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package cluster

import (
"testing"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
)

func TestPointsTo(t *testing.T) {
targetID := "fri3ndsh1p"

meta := metav1.ObjectMeta{
UID: types.UID(targetID),
}

tests := []struct {
name string
refIDs []string
expected bool
}{
{
name: "empty owner list",
},
{
name: "single wrong owner ref",
refIDs: []string{"m4g1c"},
},
{
name: "single right owner ref",
refIDs: []string{targetID},
expected: true,
},
{
name: "multiple wrong refs",
refIDs: []string{"m4g1c", "h4rm0ny"},
},
{
name: "multiple refs one right",
refIDs: []string{"m4g1c", targetID},
expected: true,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
pointer := &metav1.ObjectMeta{}

for _, ref := range test.refIDs {
pointer.OwnerReferences = append(pointer.OwnerReferences, metav1.OwnerReference{
UID: types.UID(ref),
})
}

result := pointsTo(pointer.OwnerReferences, &meta)
if result != test.expected {
t.Errorf("expected %v, got %v", test.expected, result)
}
})
}
}
6 changes: 5 additions & 1 deletion pkg/controller/cluster/cluster_reconciler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,11 @@ func TestReconcile(t *testing.T) {
c = mgr.GetClient()

a := newTestActuator()
recFn, requests := SetupTestReconcile(newReconciler(mgr, a))
r, err := newReconciler(mgr, a)
if err != nil {
t.Fatalf("Couldn't create controller: %v", err)
}
recFn, requests := SetupTestReconcile(r)
if err := add(mgr, recFn); err != nil {
t.Fatalf("error adding controller to manager: %v", err)
}
Expand Down