longhorn-manager icon indicating copy to clipboard operation
longhorn-manager copied to clipboard

Update BackupTarget condition for connection error

Open ejweber opened this issue 1 year ago • 3 comments

Which issue(s) this PR fixes:

longhorn/longhorn#8210

What this PR does / why we need it:

See https://github.com/longhorn/longhorn/issues/8210#issuecomment-2010429584.

Special notes for your reviewer:

Considerations:

We have had a few issues in this area of code previously, so I'm hoping to refactor a bit to make things easier.

Key considerations:

  • We need to be sure NOT to update lastSyncedAt if the backup target is unavailable for communication. (Maintain the fix from https://github.com/longhorn/longhorn/issues/8210#issuecomment-2010429584.)
  • We need to be sure NOT to return an error from reconcile if the backup target is unavailable for communication. Instead we need to log the error and update the status if necessary. (Maintain previous behavior and fix the regression in longhorn/longhorn#8210.)

We have three functions that all independently communicate with the backup target. If they fail to communicate with the backup target, we need to handle things one way. If they fail for any other reason, we need to handle things another way. In addition, each function only needs to get some information from the backup target once. It seems to me we can simplify by getting all the information we need from the backup target once (and handling it appropriately if we fail). Then, the other functions can just use that information and not have any difficult error related logic.

ejweber avatar Mar 20 '24 19:03 ejweber