Skip to content
This repository has been archived by the owner on Oct 12, 2023. It is now read-only.

Update #11

Merged
merged 7 commits into from
Jul 16, 2020
Merged

Update #11

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
84b34c2
add update and owner reference
OmerKahani Jul 9, 2020
991c628
add delete
OmerKahani Jul 11, 2020
8330ca4
Merge branch 'master' of https://github.com/argoproj-labs/application…
OmerKahani Jul 11, 2020
64a7159
add doc
OmerKahani Jul 11, 2020
a98f1a9
refactor to two functions
OmerKahani Jul 13, 2020
72610f1
fix logs
OmerKahani Jul 15, 2020
21b2a62
add argocd Finalizer
OmerKahani Jul 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
74 changes: 59 additions & 15 deletions pkg/controllers/applicationset_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"
"github.com/argoproj-labs/applicationset/pkg/generators"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/tools/record"
"k8s.io/kubernetes/pkg/apis/core"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
Expand Down Expand Up @@ -78,17 +79,34 @@ func (r *ApplicationSetReconciler) Reconcile(req ctrl.Request) (ctrl.Result, err

}

if err := r.createApplications(ctx, applicationSetInfo, desiredApplications); err != nil {
log.Infof("Unable to create applications applicationSetInfo %e", err)
return ctrl.Result{}, err
}
r.createOrUpdateInCluster(ctx, applicationSetInfo, desiredApplications)
r.deleteInCluster(ctx, applicationSetInfo, desiredApplications)
Copy link
Member

Choose a reason for hiding this comment

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

Should we have some if conditions, like if finalizers is set , we should delete this cluster?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is what you meant, we definitely will want to add a finalizer to ApplicationSets, and if deleting the appset we must cleanup all Applications before removing that finalizer. Deletion cleanup should probably be a separate PR. I can try to find some time for this one this week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function deletes old applications from the cluster (deleted record from a list)

Deletion of ApplicationSets is implemented with owner reference so the Kubernetes GC will catch it and delete the applications

Copy link
Contributor Author

Choose a reason for hiding this comment

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

going over the proposal, every application should have resources-finalizer.argocd.argoproj.io entry in metadata.finalizers - Added it, thanks!

Do we also want to support the option to delete a record from a list / directory from git, and not run the finalizers in the application (something like kubectl delete app NAME --cascade=false) ?

Copy link
Member

Choose a reason for hiding this comment

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

Do we also want to support the option to delete a record from a list / directory from git, and not run the finalizers in the application (something like kubectl delete app NAME --cascade=false) ?

I think we don't need to implement this for now

Copy link
Contributor

Choose a reason for hiding this comment

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

this function deletes old applications from the cluster (deleted record from a list)

Deletion of ApplicationSets is implemented with owner reference so the Kubernetes GC will catch it and delete the applications

My mistake, forgot about that!


return ctrl.Result{}, nil
}

func (r *ApplicationSetReconciler) SetupWithManager(mgr ctrl.Manager) error {
if err := mgr.GetFieldIndexer().IndexField(&argov1alpha1.Application{}, ".metadata.controller", func(rawObj runtime.Object) []string {
// grab the job object, extract the owner...
app := rawObj.(*argov1alpha1.Application)
owner := metav1.GetControllerOf(app)
if owner == nil {
return nil
}
// ...make sure it's a application set...
if owner.APIVersion != argoprojiov1alpha1.GroupVersion.String() || owner.Kind != "ApplicationSet" {
return nil
}

// ...and if so, return it
return []string{owner.Name}
}); err != nil {
return err
}

return ctrl.NewControllerManagedBy(mgr).
For(&argoprojiov1alpha1.ApplicationSet{}).
Owns(&argov1alpha1.Application{}).
Watches(
&source.Kind{Type: &corev1.Secret{}},
&clusterSecretEventHandler{
Expand All @@ -99,34 +117,60 @@ func (r *ApplicationSetReconciler) SetupWithManager(mgr ctrl.Manager) error {
Complete(r)
}

func (r *ApplicationSetReconciler) createApplications(ctx context.Context, applicationSetInfo argoprojiov1alpha1.ApplicationSet, appList []argov1alpha1.Application) error {
// createOrUpdateInCluster will create / update application resources in the cluster.
// For new application it will call create
// For application that need to update it will call update
// The function also adds owner reference to all applications, and uses it for delete them.
func (r *ApplicationSetReconciler) createOrUpdateInCluster(ctx context.Context, applicationSet argoprojiov1alpha1.ApplicationSet, desiredApplications []argov1alpha1.Application) {

for _, app := range appList {
app.Namespace = applicationSetInfo.Namespace
//create or updates the application in appList
for _, app := range desiredApplications {
app.Namespace = applicationSet.Namespace

found := app
action, err := ctrl.CreateOrUpdate(ctx, r.Client, &found, func() error {
found.Spec = app.Spec
return controllerutil.SetControllerReference(&applicationSetInfo, &found, r.Scheme)
return controllerutil.SetControllerReference(&applicationSet, &found, r.Scheme)
})

if err != nil {
log.Error(err, fmt.Sprintf("failed to get Application %s resource for applicationSet %s", app.Name, applicationSetInfo.Name))
log.Error(err, fmt.Sprintf("failed to CreateOrUpdate Application %s resource for applicationSet %s", app.Name, applicationSet.Name))
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a suggestion for using logrus and structured logging:

Define a logger with context within your loop:

appLog := log.WithFields(log.Fields{"app": app.Name, "appSet": applicationSet.Name})

Then use appLog within the loop.

appLog.WithError(err).Error("failed to CreateOrUpdate Application")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added thanks!

continue
}

r.Recorder.Eventf(&applicationSetInfo, core.EventTypeNormal, "Created", "Created Application %q", app.Name)
log.Infof("%s Application %s resource for applicationSet %s", action, app.Name, applicationSetInfo.Name)
r.Recorder.Eventf(&applicationSet, core.EventTypeNormal, fmt.Sprint(action), "%s Application %q", action, app.Name)
log.Infof("%s Application %s resource for applicationSet %s", action, app.Name, applicationSet.Name)
}
}

return nil
// deleteInCluster will delete application that are current in the cluster but not in appList.
// The function must be called after all generators had been called and generated applications
func (r *ApplicationSetReconciler) deleteInCluster(ctx context.Context, applicationSet argoprojiov1alpha1.ApplicationSet, desiredApplications []argov1alpha1.Application) {

}
// Save current applications to be able to delete the ones that are not in appList
var current argov1alpha1.ApplicationList
_ = r.Client.List(context.Background(), &current, client.MatchingFields{".metadata.controller": applicationSet.Name})

func (r *ApplicationSetReconciler) getApplications(ctx context.Context, applicationSetInfo argoprojiov1alpha1.ApplicationSet) ([]argov1alpha1.Application, error) {
m := make(map[string]bool) // Will holds the app names in appList for the deletion process

return nil, nil
for _, app := range desiredApplications {
m[app.Name] = true
}

// Delete apps that are not in m[string]bool
Copy link
Member

Choose a reason for hiding this comment

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

I think we should separate out this function , delete application shouldn't exists in apply function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

for _, app := range current.Items {
_, exists := m[app.Name]

if exists == false {
err := r.Client.Delete(ctx, &app)
if err != nil {
log.Error(err, fmt.Sprintf("failed to delete Application %s resource for applicationSet %s", app.Name, applicationSet.Name))
continue
}
r.Recorder.Eventf(&applicationSet, core.EventTypeNormal, "Deleted", "Deleted Application %q", app.Name)
log.Infof("Deleted Application %s resource for applicationSet %s", app.Name, applicationSet.Name)
}
}
}

var _ handler.EventHandler = &clusterSecretEventHandler{}
207 changes: 204 additions & 3 deletions pkg/controllers/applicationset_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package controllers

import (
"context"
"fmt"
argov1alpha1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1"
"github.com/stretchr/testify/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -15,7 +16,7 @@ import (
argoprojiov1alpha1 "github.com/argoproj-labs/applicationset/api/v1alpha1"
)

func TestCreateApplications(t *testing.T) {
func TestCreateOrUpdateApplications(t *testing.T) {

scheme := runtime.NewScheme()
argoprojiov1alpha1.AddToScheme(scheme)
Expand Down Expand Up @@ -113,6 +114,63 @@ func TestCreateApplications(t *testing.T) {
},
},
},
{
appSet: argoprojiov1alpha1.ApplicationSet{
ObjectMeta: metav1.ObjectMeta{
Name: "name",
Namespace: "namespace",
},
Spec: argoprojiov1alpha1.ApplicationSetSpec{
Template: argoprojiov1alpha1.ApplicationSetTemplate{
Spec: argov1alpha1.ApplicationSpec{
Project: "project",
},
},
},
},
existsApps: []argov1alpha1.Application{
argov1alpha1.Application{
TypeMeta: metav1.TypeMeta{
Kind: "Application",
APIVersion: "argoproj.io/v1alpha1",
},
ObjectMeta: metav1.ObjectMeta{
Name: "app1",
Namespace: "namespace",
ResourceVersion: "2",
},
Spec: argov1alpha1.ApplicationSpec{
Project: "test",
},
},
},
apps: []argov1alpha1.Application{
{
ObjectMeta: metav1.ObjectMeta{
Name: "app2",
},
Spec: argov1alpha1.ApplicationSpec{
Project: "project",
},
},
},
expected: []argov1alpha1.Application{
{
TypeMeta: metav1.TypeMeta{
Kind: "Application",
APIVersion: "argoproj.io/v1alpha1",
},
ObjectMeta: metav1.ObjectMeta{
Name: "app2",
Namespace: "namespace",
ResourceVersion: "1",
},
Spec: argov1alpha1.ApplicationSpec{
Project: "project",
},
},
},
},
} {
initObjs := []runtime.Object{&c.appSet}
for _, a := range c.existsApps {
Expand All @@ -125,10 +183,143 @@ func TestCreateApplications(t *testing.T) {
r := ApplicationSetReconciler{
Client: client,
Scheme: scheme,
Recorder: record.NewFakeRecorder(1),
Recorder: record.NewFakeRecorder(len(initObjs) + len(c.expected)),
}

r.createOrUpdateInCluster(context.TODO(), c.appSet, c.apps)

for _, obj := range c.expected {
got := &argov1alpha1.Application{}
_ = client.Get(context.Background(), crtclient.ObjectKey{
Namespace: obj.Namespace,
Name: obj.Name,
}, got)

controllerutil.SetControllerReference(&c.appSet, &obj, r.Scheme)

assert.Equal(t, obj, *got)
}
}

}

func TestDeleteApplications(t *testing.T) {

scheme := runtime.NewScheme()
argoprojiov1alpha1.AddToScheme(scheme)
argov1alpha1.AddToScheme(scheme)

for _, c := range []struct {
appSet argoprojiov1alpha1.ApplicationSet
existsApps []argov1alpha1.Application
apps []argov1alpha1.Application
expected []argov1alpha1.Application
notExpected []argov1alpha1.Application
}{
{
appSet: argoprojiov1alpha1.ApplicationSet{
ObjectMeta: metav1.ObjectMeta{
Name: "name",
Namespace: "namespace",
},
Spec: argoprojiov1alpha1.ApplicationSetSpec{
Template: argoprojiov1alpha1.ApplicationSetTemplate{
Spec: argov1alpha1.ApplicationSpec{
Project: "project",
},
},
},
},
existsApps: []argov1alpha1.Application{
{
TypeMeta: metav1.TypeMeta{
Kind: "Application",
APIVersion: "argoproj.io/v1alpha1",
},
ObjectMeta: metav1.ObjectMeta{
Name: "delete",
Namespace: "namespace",
ResourceVersion: "2",
},
Spec: argov1alpha1.ApplicationSpec{
Project: "project",
},
},
{
TypeMeta: metav1.TypeMeta{
Kind: "Application",
APIVersion: "argoproj.io/v1alpha1",
},
ObjectMeta: metav1.ObjectMeta{
Name: "keep",
Namespace: "namespace",
ResourceVersion: "2",
},
Spec: argov1alpha1.ApplicationSpec{
Project: "project",
},
},
},
apps: []argov1alpha1.Application{
{
ObjectMeta: metav1.ObjectMeta{
Name: "keep",
},
Spec: argov1alpha1.ApplicationSpec{
Project: "project",
},
},
},
expected: []argov1alpha1.Application{
{
TypeMeta: metav1.TypeMeta{
Kind: "Application",
APIVersion: "argoproj.io/v1alpha1",
},
ObjectMeta: metav1.ObjectMeta{
Name: "keep",
Namespace: "namespace",
ResourceVersion: "2",
},
Spec: argov1alpha1.ApplicationSpec{
Project: "project",
},
},
},
notExpected: []argov1alpha1.Application{
{
TypeMeta: metav1.TypeMeta{
Kind: "Application",
APIVersion: "argoproj.io/v1alpha1",
},
ObjectMeta: metav1.ObjectMeta{
Name: "delete",
Namespace: "namespace",
ResourceVersion: "1",
},
Spec: argov1alpha1.ApplicationSpec{
Project: "project",
},
},
},
},
} {
initObjs := []runtime.Object{&c.appSet}
for _, a := range c.existsApps {
temp := a
controllerutil.SetControllerReference(&c.appSet, &temp, scheme)
initObjs = append(initObjs, &temp)
}

client := fake.NewFakeClientWithScheme(scheme, initObjs...)

r := ApplicationSetReconciler{
Client: client,
Scheme: scheme,
Recorder: record.NewFakeRecorder(len(initObjs) + len(c.expected)),
}

r.createApplications(context.TODO(), c.appSet, c.apps)
r.deleteInCluster(context.TODO(), c.appSet, c.apps)

for _, obj := range c.expected {
got := &argov1alpha1.Application{}
Expand All @@ -141,6 +332,16 @@ func TestCreateApplications(t *testing.T) {

assert.Equal(t, obj, *got)
}

for _, obj := range c.notExpected {
got := &argov1alpha1.Application{}
err := client.Get(context.Background(), crtclient.ObjectKey{
Namespace: obj.Namespace,
Name: obj.Name,
}, got)

assert.EqualError(t, err, fmt.Sprintf("applications.argoproj.io \"%s\" not found", obj.Name))
}
}

}