Restart poller after all CES are reconciled after nephe restart. #269

archanapholla · 2023-07-05T20:46:36Z

No description provided.

Anandkumar26 · 2023-07-08T15:27:55Z

pkg/accountmanager/manager.go

+			if exists {
+				accPoller.restartPoller(accNamespacedName)
+				// wait for polling to complete after restart.
+				retError = accPoller.waitForPollDone(accNamespacedName)


I believe the error from accPoller.waitForPollDone(accNamespacedName) will be lost.
Can you simulate an error for this scenario and verify if the CES status is updated?

Error from "accPoller.waitForPollDone(accNamespacedName)" is not lost as we can have this kind of wait function in defer function.
However, I saw an issue setting return value of AddResourceFiltersToAccount() from defer function.
I had to return the address.
Also, I tested the negative scenario, we should be good.

Anandkumar26 · 2023-07-08T15:28:12Z

pkg/accountmanager/manager.go

@@ -159,31 +164,56 @@ func (a *AccountManager) RemoveAccount(namespacedName *types.NamespacedName) err
 // restart poller once done.
 func (a *AccountManager) AddResourceFiltersToAccount(accNamespacedName *types.NamespacedName,
 	selectorNamespacedName *types.NamespacedName, selector *crdv1alpha1.CloudEntitySelector, replay bool) (bool, error) {
+	var retError error = nil


No need to initialise it to nil.

Anandkumar26 · 2023-07-10T12:33:50Z

pkg/accountmanager/manager.go

@@ -61,6 +61,8 @@ type AccountManager struct {
 	Inventory        inventory.Interface
 	accPollers       map[types.NamespacedName]*accountPoller
 	accountConfigMap map[types.NamespacedName]*accountConfig
+	// accountToNumSelectors stores number of configured CloudEntitySelector CRs for a given account, required for handling nephe restart case.


nit: split comment into two lines.

Anandkumar26 · 2023-07-10T12:35:30Z

pkg/accountmanager/manager.go

-			"provider type not found", selectorNamespacedName, accNamespacedName))
+		retError = fmt.Errorf(fmt.Sprintf("failed to add or update selector %v, account %v: "+
+			"provider type not found", *selectorNamespacedName, *accNamespacedName))
+		return false, retError


What is the use of creating a new error variable here vs directly returning the error?

we want the defer function to get executed too which can alter the return value.

Anandkumar26 · 2023-07-10T12:38:05Z

pkg/apiserver/webhook/cloudentityselector_webhook.go

@@ -83,10 +83,10 @@ func (v *CESMutator) Handle(_ context.Context, req admission.Request) admission.
 	referencedAccount := &v1alpha1.CloudProviderAccount{}
 	err = v.Client.Get(context.TODO(), *accountNameSpacedName, referencedAccount)


nit: Change to if err = (); err != nil {}

Anandkumar26 · 2023-07-10T12:39:26Z

pkg/cloudprovider/plugins/aws/aws_ec2.go

@@ -127,7 +127,7 @@ func (ec2Cfg *ec2ServiceConfig) getManagedVpcIDs() map[string]struct{} {
 		awsPluginLogger().V(4).Info("Cache snapshot nil", "type", providerType, "account", ec2Cfg.accountNamespacedName)
 		return vpcIDsCopy
 	}
-	vpcIDsSet := snapshot.(*ec2ResourcesCacheSnapshot).managedVpcIDs
+	vpcIDsSet := snapshot.(*ec2ResourcesCacheSnapshot).managedVpcIds


nit: vpcIdsSet, vpcIdsCopy.
If you are renaming the variables, please rename all the above.

Fixed all in azure_compute.go and aws_ec2.go .

Anandkumar26 · 2023-07-10T12:41:22Z

pkg/controllers/cloudentityselector/controller.go

@@ -69,11 +69,10 @@ func (r *CloudEntitySelectorReconciler) Reconcile(ctx context.Context, req ctrl.
 		return ctrl.Result{}, err


nit: return ctrl.Result{}, r.processDelete(&req.NamespacedName) and remove L68

reachjainrahul · 2023-07-11T19:42:14Z

pkg/accountmanager/manager.go

@@ -158,32 +164,57 @@ func (a *AccountManager) RemoveAccount(namespacedName *types.NamespacedName) err
 // AddResourceFiltersToAccount add/update cloud plugin for a given account to include the new selector and
 // restart poller once done.
 func (a *AccountManager) AddResourceFiltersToAccount(accNamespacedName *types.NamespacedName,
-	selectorNamespacedName *types.NamespacedName, selector *crdv1alpha1.CloudEntitySelector, replay bool) (bool, error) {
+	selectorNamespacedName *types.NamespacedName, selector *crdv1alpha1.CloudEntitySelector, replay bool) (bool, *error) {


Why is error used as pointer.. Consider changing it

Without it being a pointer, we cannot set return value in defer function. If it is a regular variable then the retError set in defer function doesn't get returned to the caller. As a side-effect status setting in CES wouldn't work.

reachjainrahul · 2023-07-11T19:43:34Z

pkg/accountmanager/manager.go

@@ -102,11 +106,13 @@ func (a *AccountManager) AddAccount(namespacedName *types.NamespacedName, accoun
 	// Create an account poller for polling cloud inventory.
 	accPoller, exists := a.addAccountPoller(cloudInterface, namespacedName, account)
 	if !exists {
-		if !crd.DoesCesCrExistsForAccount(a.Client, namespacedName) {
+		numCes := crd.GetNumberOfCesCrForAccount(a.Client, namespacedName)


GetCesCrForAccount

reachjainrahul · 2023-07-13T20:32:54Z

pkg/controllers/cloudentityselector/controller.go

@@ -138,13 +136,13 @@ func (r *CloudEntitySelectorReconciler) processCreateOrUpdate(selector *crdv1alp
 		Name:      selector.Spec.AccountName,
 	}
 	r.selectorToAccountMap[*selectorNamespacedName] = *accountNamespacedName
-	retry, err := r.AccManager.AddResourceFiltersToAccount(accountNamespacedName, selectorNamespacedName,
+	// No errors can be retried for a CES.
+	_, err := r.AccManager.AddResourceFiltersToAccount(accountNamespacedName, selectorNamespacedName,


@Anandkumar26 Can you confirm this?

Yes. The error while processing CES can only occur if an account is either not initialised successfully or account is in a bad state. I don't think there is any benefit in retrying the errors forever.

So we capture the error and store in a CES status field. (which can be replayed when account goes from an uninitialised to initialised state OR it would need a user intervention)

reachjainrahul · 2023-07-18T17:26:05Z

/nephe-test-e2e-agentless

reachjainrahul · 2023-07-21T22:53:28Z

pkg/controllers/cloudentityselector/controller.go

+	// Make sure reconciler retriable error cannot occur during restart, it can affect the logic in UpdatePendingCes function.
+	if count := r.AccManager.UpdatePendingCesCount(accountNamespacedName); count == 0 {
+		if err := r.AccManager.WaitForPollDone(accountNamespacedName); err != nil {
+			r.Log.Error(err, "Inventory poll failed", "account", accountNamespacedName, "error", err)


nit: inventory

reachjainrahul · 2023-07-21T23:04:00Z

pkg/controllers/cloudentityselector/controller.go

@@ -144,6 +143,12 @@ func (r *CloudEntitySelectorReconciler) processCreateOrUpdate(selector *crdv1alp
 		return err
 	}
 	r.updateStatus(selectorNamespacedName, err)
+	// Make sure reconciler retriable error cannot occur during restart, it can affect the logic in UpdatePendingCes function.


add detail comment on what it means..

added a TODO as discussed

reachjainrahul · 2023-07-21T23:52:13Z

/nephe-test-e2e-agentless-upgrade

- On nephe restart, In order to avoid poller restart on CPA and every CES add, postpone restart till all CES attached to the CPA are reconciled. - Fix logging in CES webhook. Signed-off-by: Archana Holla <harchana@vmware.com>

reachjainrahul · 2023-07-22T04:26:29Z

/nephe-test-e2e-agentless

reachjainrahul · 2023-07-22T04:27:06Z

/nephe-test-e2e-agentless

reachjainrahul

/LGTM

archanapholla force-pushed the multi_ces_restart branch from ac3581f to 54c66b3 Compare July 6, 2023 18:55

archanapholla changed the title ~~[DO NOT REVIEW] Improve UT coverage for azure plugin.~~ Restart poller after all CES are reconciled after nephe restart. Jul 6, 2023

archanapholla changed the title ~~Restart poller after all CES are reconciled after nephe restart.~~ [DO NOT REVIEW] Restart poller after all CES are reconciled after nephe restart. Jul 6, 2023

archanapholla force-pushed the multi_ces_restart branch from 54c66b3 to 1358e5d Compare July 7, 2023 16:36

archanapholla changed the title ~~[DO NOT REVIEW] Restart poller after all CES are reconciled after nephe restart.~~ Restart poller after all CES are reconciled after nephe restart. Jul 7, 2023

archanapholla force-pushed the multi_ces_restart branch from 1358e5d to e89b6c5 Compare July 10, 2023 12:14

Anandkumar26 reviewed Jul 10, 2023

View reviewed changes

archanapholla force-pushed the multi_ces_restart branch from e89b6c5 to 0900b10 Compare July 11, 2023 18:54

reachjainrahul reviewed Jul 11, 2023

View reviewed changes

archanapholla force-pushed the multi_ces_restart branch from 0900b10 to 0f7562d Compare July 12, 2023 22:34

reachjainrahul reviewed Jul 13, 2023

View reviewed changes

reachjainrahul force-pushed the multi_ces_restart branch from 0f7562d to 129405b Compare July 18, 2023 17:17

archanapholla force-pushed the multi_ces_restart branch 4 times, most recently from 196f9d9 to a395391 Compare July 21, 2023 20:15

reachjainrahul reviewed Jul 21, 2023

View reviewed changes

archanapholla force-pushed the multi_ces_restart branch from c6395dc to 57c1cb1 Compare July 21, 2023 23:55

archanapholla force-pushed the multi_ces_restart branch from 57c1cb1 to 68fcff0 Compare July 21, 2023 23:57

reachjainrahul approved these changes Jul 22, 2023

View reviewed changes

reachjainrahul merged commit f384874 into main Jul 22, 2023

reachjainrahul deleted the multi_ces_restart branch July 22, 2023 05:41

reachjainrahul added this to the Nephe v0.6.0 release milestone Jul 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restart poller after all CES are reconciled after nephe restart. #269

Restart poller after all CES are reconciled after nephe restart. #269

archanapholla commented Jul 5, 2023

Anandkumar26 Jul 8, 2023

archanapholla Jul 11, 2023

Anandkumar26 Jul 8, 2023

Anandkumar26 Jul 10, 2023

archanapholla Jul 11, 2023

Anandkumar26 Jul 10, 2023

archanapholla Jul 11, 2023

Anandkumar26 Jul 10, 2023

archanapholla Jul 11, 2023

Anandkumar26 Jul 10, 2023

archanapholla Jul 11, 2023

Anandkumar26 Jul 10, 2023

reachjainrahul Jul 11, 2023

archanapholla Jul 11, 2023

reachjainrahul Jul 11, 2023 •

edited

Loading

reachjainrahul Jul 13, 2023

Anandkumar26 Jul 14, 2023

reachjainrahul commented Jul 18, 2023

reachjainrahul Jul 21, 2023

archanapholla Jul 21, 2023

reachjainrahul Jul 21, 2023

archanapholla Jul 21, 2023

reachjainrahul commented Jul 21, 2023

reachjainrahul commented Jul 22, 2023

reachjainrahul commented Jul 22, 2023

reachjainrahul left a comment

		@@ -83,10 +83,10 @@ func (v *CESMutator) Handle(_ context.Context, req admission.Request) admission.
		referencedAccount := &v1alpha1.CloudProviderAccount{}
		err = v.Client.Get(context.TODO(), *accountNameSpacedName, referencedAccount)

		@@ -69,11 +69,10 @@ func (r *CloudEntitySelectorReconciler) Reconcile(ctx context.Context, req ctrl.
		return ctrl.Result{}, err

Restart poller after all CES are reconciled after nephe restart. #269

Restart poller after all CES are reconciled after nephe restart. #269

Conversation

archanapholla commented Jul 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reachjainrahul Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reachjainrahul commented Jul 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reachjainrahul commented Jul 21, 2023

reachjainrahul commented Jul 22, 2023

reachjainrahul commented Jul 22, 2023

reachjainrahul left a comment

Choose a reason for hiding this comment

reachjainrahul Jul 11, 2023 •

edited

Loading