Added reconcilation logic if spec.ApplicationSet is removed #670

rishabh625 · 2022-06-02T08:26:53Z

Signed-off-by: rishabh625 [email protected]

What type of PR is this?

Uncomment only one /kind line, and delete the rest.
For example, > /kind bug would simply become: /kind bug

/kind bug
/kind chore
/kind cleanup
/kind failing-test
/kind enhancement
/kind documentation
/kind code-refactoring

/kind bug

What does this PR do / why we need it:
This PR deletes application set resources if disabled.

Have you updated the necessary documentation?

Documentation update is required by this PR.
Documentation has been updated.

Which issue(s) this PR fixes:

Fixes #627
Fixes #628

How to test changes / Special notes to the reviewer:

Signed-off-by: rishabh625 <[email protected]>

iam-veeramalla · 2022-06-03T07:11:28Z

controllers/argocd/util.go

@@ -1092,8 +1092,31 @@ func (r *ReconcileArgoCD) setResourceWatches(bldr *builder.Builder, clusterResou
 		},
 	}

+	// Add new predicate to delete Notifications Resources. The predicate watches the Argo CD CR for changes to the `.spec.Notifications.Enabled`


Please update the comments to modify Notifications as ApplicationSet. To be changed in multiple places, please take care

iam-veeramalla · 2022-06-03T07:14:36Z

controllers/argocd/applicationset.go

@@ -454,3 +470,45 @@ func setAppSetLabels(obj *metav1.ObjectMeta) {
 	obj.Labels["app.kubernetes.io/part-of"] = "argocd-applicationset"
 	obj.Labels["app.kubernetes.io/component"] = "controller"
 }
+
+//deleteApplicationSetResources ... Triggers reconcillation of application set resources,logic for deletion of applicationset is in reconcile methods this method triggers the cleanup of resources using the reconcilation logic as CR changes


If please fix the below in comments.

Provide space after //

remove ...

typo in reconciliation

May be
// deleteApplicationSetResources triggers the cleanup of application set resources.

iam-veeramalla · 2022-06-03T07:16:48Z

controllers/argocd/applicationset.go

-
+		if cr.Spec.ApplicationSet == nil {
+			log.Info(fmt.Sprintf("Deleting Deployment %s as applicationset is disabled", existing.Name))
+			return r.Client.Delete(context.TODO(), existing)


When I removed the applicationSet entry from Argo CD CR, controller crashed with the below error. I edited the Argo CD CR to remove the .spec.applicationSet entry.

1.6542400631086211e+09 INFO controller_argocd reconciling ApplicationSet deployment E0603 12:37:43.108966 18003 runtime.go:78] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference) goroutine 506 [running]: k8s.io/apimachinery/pkg/util/runtime.logPanic({0x237e8a0?, 0x3477f10}) /Users/aveerama/go/src/github.com/iam-veeramalla/argocd-operator/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:74 +0x86 k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0x8?}) /Users/aveerama/go/src/github.com/iam-veeramalla/argocd-operator/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:48 +0x75 panic({0x237e8a0, 0x3477f10}) /usr/local/go/src/runtime/panic.go:838 +0x207 github.com/argoproj-labs/argocd-operator/controllers/argocd.getArgoApplicationSetCommand(0xc000b80000) /Users/aveerama/go/src/github.com/iam-veeramalla/argocd-operator/controllers/argocd/applicationset.go:47 +0x285 github.com/argoproj-labs/argocd-operator/controllers/argocd.applicationSetContainer(_) /Users/aveerama/go/src/github.com/iam-veeramalla/argocd-operator/controllers/argocd/applicationset.go:190 +0x152

iam-veeramalla

Operator Crashed when ApplicationSet entry is removed from CR. I followed below steps

Created the Argo CD CR using

apiVersion: argoproj.io/v1alpha1
kind: ArgoCD
metadata:
  name: example-argocd
  labels:
    example: ingress
spec:
  applicationSet: {}
  server:
    grpc:
      ingress:
        enabled: true
    ingress:
      enabled: true
    insecure: true

After a while(once all the resources are created), I removed the entry for ApplicationSet from CR using oc edit

Please add steps to reproduce or test the PR in the description section.

rishabh625 · 2022-06-03T07:24:14Z

Sure will look at all Thanks @iam-veeramalla

Signed-off-by: rishabh625 <[email protected]>

jaideepr97 · 2022-06-10T21:29:33Z

controllers/argocd/applicationset_test.go

+func TestReconcileApplicationSet_DeleteDeployment(t *testing.T) {
+	logf.SetLogger(ZapLogger(true))
+	a := makeTestArgoCD()
+	a.Spec.ApplicationSet = &v1alpha1.ArgoCDApplicationSet{}
+	r := makeTestReconciler(t, a)
+	a.Spec.ApplicationSet = nil
+	checkdeployment := &appsv1.Deployment{}
+	assert.Error(t, r.Client.Get(
+		context.TODO(),
+		types.NamespacedName{
+			Name:      "argocd-applicationset-controller",
+			Namespace: a.Namespace,
+		},
+		checkdeployment))
+	assert.Equal(t, checkdeployment.Name, "")
+	checkrole := &v1.Role{}
+	assert.Error(t, r.Client.Get(
+		context.TODO(),
+		types.NamespacedName{
+			Name:      "argocd-applicationset-controller",
+			Namespace: a.Namespace,
+		},
+		checkrole))
+	assert.Equal(t, checkrole.Name, "")
+
+	checksa := &corev1.ServiceAccount{}
+	assert.Error(t, r.Client.Get(
+		context.TODO(),
+		types.NamespacedName{
+			Name:      "argocd-applicationset-controller",
+			Namespace: a.Namespace,
+		},
+		checksa))
+	assert.Equal(t, checksa.Name, "")
+	checkroleBinding := &v1.RoleBinding{}
+	assert.Error(t, r.Client.Get(
+		context.TODO(),
+		types.NamespacedName{
+			Name:      "argocd-applicationset-controller",
+			Namespace: a.Namespace,
+		},
+		checkroleBinding))
+	assert.Equal(t, checkroleBinding.Name, "")
+}


@rishabh625
are these resources actually getting created in the first place? I don't see a call to reconcile these resources
I think we should make sure to first check that the resources exist and before setting a.spec.applicationset to nil in order to be sure the test is accurate and that resources are in fact being deleted as expected

jaideepr97 · 2022-06-10T21:44:46Z

@rishabh625
I think the nil pointer reference @iam-veeramalla mentioned is coming from here https://github.com/argoproj-labs/argocd-operator/pull/670/files#diff-547555dfe97d80ddf6097444f1cc82263cce61c48fbb67f6bb8e08c657873bc6L46
we should check if cr.spec.applicationset != nil first

…r_appset

Signed-off-by: rishabh625 <[email protected]>

….com/rishabh625/argocd-operator into create_routes_svc_ingress_for_appset

Signed-off-by: rishabh625 <[email protected]>

rishabh625 · 2022-06-14T19:36:51Z

@rishabh625 I think the nil pointer reference @iam-veeramalla mentioned is coming from here https://github.com/argoproj-labs/argocd-operator/pull/670/files#diff-547555dfe97d80ddf6097444f1cc82263cce61c48fbb67f6bb8e08c657873bc6L46 we should check if cr.spec.applicationset != nil first

thanks @jaideepr97 will look into this, was waiting for discussion

Signed-off-by: rishabh625 <[email protected]>

…r_appset

….com/rishabh625/argocd-operator into create_routes_svc_ingress_for_appset

…ishabh625/argocd-operator into bug/reconcile_disabling_appset

…e_disabling_appset

…ishabh625/argocd-operator into bug/reconcile_disabling_appset

Signed-off-by: rishabh625 <[email protected]>

….com/rishabh625/argocd-operator into bug/reconcile_disabling_appset

Signed-off-by: rishabh625 <[email protected]>

.github/workflows/test.yaml

api/v1alpha1/argocd_types.go

controllers/argocd/applicationset.go

Signed-off-by: rishabh625 <[email protected]>

…ishabh625/argocd-operator into bug/reconcile_disabling_appset

Signed-off-by: rishabh625 <[email protected]>

…ishabh625/argocd-operator into bug/reconcile_disabling_appset

rishabh625 added 2 commits June 2, 2022 00:56

Added reconcilation logic if spec.ApplicationSet is removed

221c57d

Signed-off-by: rishabh625 <[email protected]>

Corrected deletion test for appset

330517e

Signed-off-by: rishabh625 <[email protected]>

iam-veeramalla reviewed Jun 3, 2022

View reviewed changes

iam-veeramalla requested changes Jun 3, 2022

View reviewed changes

iam-veeramalla self-assigned this Jun 3, 2022

Added routes,ingress,svc code

4f7d368

Signed-off-by: rishabh625 <[email protected]>

jaideepr97 reviewed Jun 10, 2022

View reviewed changes

rishabh625 and others added 4 commits June 15, 2022 00:49

Merge branch 'argoproj-labs:master' into create_routes_svc_ingress_fo…

625d729

…r_appset

Added svc ingress route logic

7abc8eb

Signed-off-by: rishabh625 <[email protected]>

Merge branch 'create_routes_svc_ingress_for_appset' of https://github…

a66efaf

….com/rishabh625/argocd-operator into create_routes_svc_ingress_for_appset

Added enable ingress in test

c2e09c8

Signed-off-by: rishabh625 <[email protected]>

rishabh625 and others added 14 commits June 15, 2022 11:05

Added correct version of operator

2205214

Signed-off-by: rishabh625 <[email protected]>

Merge branch 'argoproj-labs:master' into create_routes_svc_ingress_fo…

d1f608e

…r_appset

ran make bundle

fb0e1d0

Applied reviewer suggestions, removed ingress for metrics

d00d5d7

Merge branch 'create_routes_svc_ingress_for_appset' of https://github…

53d8f52

….com/rishabh625/argocd-operator into create_routes_svc_ingress_for_appset

Applied reviewer suggestions, removed ingress for metrics

6ae79e4

Removed unwanted code

b5cc4c4

fixed make generate

592e09c

Add kuttl test for applicationset

ed326a4

Renamed kuttl test file

21ab8a4

corrected names

7252079

fix kuttl test

b6e3019

Merged create svc,route appset webhook branch

83c5cf8

corrected comments for method

ca76ede

rishabh625 added 10 commits July 1, 2022 19:15

corrected test

55e9965

corrected text for applicationset webhook

e095474

ran make bundle

06f67b7

Added e2e test and status for applicationSet

fb084b2

ran make bundle

f522095

add appset in alm-examples

2cc12aa

Merge branch 'bug/reconcile_disabling_appset' of https://github.com/r…

c4e4795

…ishabh625/argocd-operator into bug/reconcile_disabling_appset

Merge branch 'create_routes_svc_ingress_for_appset' into bug/reconcil…

fe4de2f

…e_disabling_appset

add 0.5.0 csv

041d116

ran make bundle

ebc6df0

rishabh625 requested a review from iam-veeramalla July 4, 2022 12:43

rishabh625 and others added 9 commits July 6, 2022 12:54

Test to check reconcilation of appset

ba138fd

Merge branch 'bug/reconcile_disabling_appset' of https://github.com/r…

9b419e9

…ishabh625/argocd-operator into bug/reconcile_disabling_appset

corrected argocd cr name in test

b725999

Signed-off-by: rishabh625 <[email protected]>

Test

e3b08c3

Merge branch 'master' into create_routes_svc_ingress_for_appset

0ebde17

merged master branch and resolved conflicts

4825291

Signed-off-by: rishabh625 <[email protected]>

Merge branch 'create_routes_svc_ingress_for_appset' of https://github…

ebee8a8

….com/rishabh625/argocd-operator into bug/reconcile_disabling_appset

Merge branch 'master' of https://github.com/rishabh625/argocd-operator

158ae33

merged master branch and resolved conflicts

57d5f63

Signed-off-by: rishabh625 <[email protected]>

rishabh625 requested a review from jaideepr97 August 17, 2022 12:45

jaideepr97 reviewed Aug 17, 2022

View reviewed changes

.github/workflows/test.yaml Outdated Show resolved Hide resolved

jaideepr97 reviewed Aug 17, 2022

View reviewed changes

api/v1alpha1/argocd_types.go Outdated Show resolved Hide resolved

jaideepr97 reviewed Aug 17, 2022

View reviewed changes

controllers/argocd/applicationset.go Outdated Show resolved Hide resolved

rishabh625 added 6 commits August 17, 2022 23:15

merged master branch and resolved conflicts

515ad71

Signed-off-by: rishabh625 <[email protected]>

Merge branch 'bug/reconcile_disabling_appset' of https://github.com/r…

9ef5191

…ishabh625/argocd-operator into bug/reconcile_disabling_appset

ran make bundle

a6906d7

applied reviewers suggestion

dd0da90

Signed-off-by: rishabh625 <[email protected]>

Merge branch 'bug/reconcile_disabling_appset' of https://github.com/r…

f537a4c

…ishabh625/argocd-operator into bug/reconcile_disabling_appset

ran make bundle

5184508

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added reconcilation logic if spec.ApplicationSet is removed #670

Added reconcilation logic if spec.ApplicationSet is removed #670

rishabh625 commented Jun 2, 2022 •

edited

Loading

iam-veeramalla Jun 3, 2022

iam-veeramalla Jun 3, 2022

iam-veeramalla Jun 3, 2022

iam-veeramalla left a comment

rishabh625 commented Jun 3, 2022

jaideepr97 Jun 10, 2022 •

edited

Loading

jaideepr97 commented Jun 10, 2022

rishabh625 commented Jun 14, 2022

Added reconcilation logic if spec.ApplicationSet is removed #670

Are you sure you want to change the base?

Added reconcilation logic if spec.ApplicationSet is removed #670

Conversation

rishabh625 commented Jun 2, 2022 • edited Loading

iam-veeramalla Jun 3, 2022

Choose a reason for hiding this comment

iam-veeramalla Jun 3, 2022

Choose a reason for hiding this comment

iam-veeramalla Jun 3, 2022

Choose a reason for hiding this comment

iam-veeramalla left a comment

Choose a reason for hiding this comment

rishabh625 commented Jun 3, 2022

jaideepr97 Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

jaideepr97 commented Jun 10, 2022

rishabh625 commented Jun 14, 2022

rishabh625 commented Jun 2, 2022 •

edited

Loading

jaideepr97 Jun 10, 2022 •

edited

Loading