Update dependent CPAs for Secret add/modify/delete operation #210
Conversation
Anandkumar26
commented
Apr 13, 2023
•
Anandkumar26
commented
- Add: Updates account only if its error state with error as Secret does not exist.
- Update/Delete: Updates account to recompute account state, as Secret is changed
- Cleanup account state when account add/update fails.
Anandkumar26
changed the title
Anandkumar26
changed the title
Anandkumar26
force-pushed
the
updatecpa
branch
3 times, most recently
from
48299b7 to
ecd2061
Compare
Anandkumar26
changed the title
|
/nephe-test-e2e-kind |
| continue | ||
| } | ||
| } | ||
| if err := r.processCreateOrUpdate(accountNamespacedName, cpa); err != nil { |
There was a problem hiding this comment.
processCreateOrUpdate function can be called from reconciler too. Let's say, before this function acquires the lock, reconciler calls processCreateOrUpdate for the same CPA(CPA attached to the secret in this function).
When the execution is complete and this function acquires the lock, this might be operating on stale secret event.
Do you see any issue in this scenario?
There was a problem hiding this comment.
At any point of time only one of them will acquire the lock and update the account's internal state.
You are talking about a scenario where both events occur at the same time.
- update of CPA object, which triggers reconciler.
- update on the Secret object.
In this particular scenario, processCreateOrUpdate will be called twice(once via Secret and once via reconciler), as a result we would restart poller and perform polling twice(which cannot be avoided).
I don't see any issue here.
| } | ||
| } | ||
| } | ||
| return cpaItems, nil | ||
| } | ||
|
|
||
| // watchSecret watch the Secret objects. | ||
| // updateDependentCpaBySecretNamespace updates all dependent accounts for a given Secret Namespace. |
There was a problem hiding this comment.
Should the comment state "for a given Secret" instead of Secret namespace
There was a problem hiding this comment.
No. Secret watcher watches for Secrets only from POD_NAMESPACE. This function will be called only for the Secret namespace.
pkg/accountmanager/poller.go
Outdated
| @@ -199,8 +196,7 @@ func (p *accountPoller) updateAccountStatus(cloudInterface common.CloudInterface | |||
|
|
|||
| if account.Status != discoveredStatus { | |||
| account.Status.Error = discoveredStatus.Error | |||
| e = p.Client.Status().Update(context.TODO(), account) | |||
| if e != nil { | |||
| if e = p.Client.Status().Update(context.TODO(), account); e != nil { | |||
| } | ||
| if err = r.AccManager.AddAccount(namespacedName, accountCloudType, account); err != nil { | ||
| account.Status.Error = err.Error() | ||
| r.Log.Info("Setting account status", "account", namespacedName, "err", err) | ||
| if err = r.Client.Status().Update(context.TODO(), account); err != nil { | ||
| return fmt.Errorf("failed to update account status, account %v err %v", namespacedName, err) | ||
| } | ||
| _ = r.AccManager.RemoveAccount(namespacedName) |
There was a problem hiding this comment.
do we really need to call removal? the AddAccount will be retried right?
There was a problem hiding this comment.
Yes. We don't want to reconciler to keep trying whenever there is an error returned by CPA.
If for any reason, we are not able to process CPA request, we will set the error status on the CPA object and clean up our internal state.
| } | ||
| namespacedName := &types.NamespacedName{Namespace: secret.Namespace, Name: secret.Name} | ||
| r.Log.WithName("Secret").Info("Received request", "Secret", namespacedName, "operation", event.Type) | ||
| r.updateDependentCpaBySecretNamespace(namespacedName, event.Type) |
There was a problem hiding this comment.
call it processSecretUpdateEvent..
| // For add operation, update CPA only when status contains an error ErrorMsgSecretDoesNotExist. | ||
| if eventType == watch.Added { | ||
| if !strings.Contains(cpa.Status.Error, util.ErrorMsgSecretDoesNotExist) { | ||
| continue |
- Add: Updates account only if its error state with error as Secret does not exist. - Update/Delete: Updates account to recompute account state, as Secret is changed - Cleanup account state when account add/update fails. - Fix imports Signed-off-by: Anand Kumar <[email protected]>
|
/nephe-test-e2e-aws |
