Fix backup post hook issue

[ywk253100]

Fix backup post hook issue

Fixes #8159

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)

Please indicate you've done the following:

• Accepted the DCO. Commits without the DCO will delay acceptance.
• Created a changelog file (make new-changelog) or comment /kind changelog-not-required on this PR.
• Updated the corresponding documentation in site/content/docs/main.

[codecov]

Codecov Report

Attention: Patch coverage is 55.71429% with 31 lines in your changes missing coverage. Please review.

Project coverage is 59.04%. Comparing base (cb7758f) to head (c43fc42).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/backup/backup.go	52.30%	26 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8509      +/-   ##
==========================================
+ Coverage   59.02%   59.04%   +0.01%     
==========================================
  Files         369      369              
  Lines       39101    39180      +79     
==========================================
+ Hits        23078    23132      +54     
- Misses      14560    14581      +21     
- Partials     1463     1467       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sseago]

We need to do something other than assuming that all volumes use fs-backup when defaultVolumesToFsBackup is true and no volumes use it when false. If it's true and all volumes in an itemBlock either opt out of fs-backup via annotation (for snapshotting) or use volume policies to snapshot, then we'll be grabbing the PVB list unnecessarily. That's harmless enough. However the other case is more problematic -- if defaultVolumesToFsBackup is false but some volumes in the ItemBlock opt in via annotation or are selected for fs-backup via volume policies, then we won't wait for them to complete to run hooks.

[Lyndon-Li]

BackupWithResolvers calls this Stat by its end, but BackupWithResolvers doesn't know details of each itemBlock, it just calls backupItemBlock for all itemBlocks.
On the other hand, PVBs in each itemBlock belong to the itemBlock, so looks like this semantic is more rational --- for each backupItemBlock call, start all PVBs of the itemBlock->wait all PVB complete->run post hooks.

However, at present, when backupItemBlock exits, the PVBs have not actually completed, BackupWithResolvers needs to call hooker's Stat to wait them indirectly.

Therefore, I suggest we make the waiting for PVB completion into backupItemBlock and call post hooks in the same place.

[Lyndon-Li]

We can do this in 1.16 only, as in 1.15, we don't support concurrent backupItemBlock, if we put the wait inside backupItemBlock, PVBs among itemBlocks have to be done in sequence, this is not helpful practically.

[Lyndon-Li]

@sseago Just let us know your thinking

[sseago]

@Lyndon-Li "Therefore, I suggest we make the waiting for PVB completion into backupItemBlock and call post hooks in the same place."

So right now, we invoke the "wait for PVB completion and then call hooks" with go kb.handleItemBlockPostHooks -- is your suggestion to replace the goroutine call with a direct func call? This would effectively mean that the specific BackupItemBlock call would not return until 1) PVB processing completes and 2) post-backup hooks are done.

With the current codebase (i.e. concurrent backupItemBlock is not yet implemented), that might, in the short term, have a negative performance impact -- but it may make more sense to handle it that way once we have concurrent backupItemBlock processing. However, there still may be edge cases where this has a negative performance impact -- in particular, with large clusters with a large number of node agents, each of which enables multiple parallel PVB processing -- the number of concurrent PVBs we can process may exceed the number of parallel ItemBlocks configured, in which case parallel item block processing will become the bottleneck, not allowing full use of the node agent pods to handle many volumes at once.

[Lyndon-Li]

the number of concurrent PVBs we can process may exceed the number of parallel ItemBlocks configured

If so, can we recommend users to adjust the concurrent number of ItemBlocks? We don't need to consider the number of CPU cores to fit the thread pool, as in this case, the work is actually done by node-agent, the itemBlock threads just need to wait the completion

[Lyndon-Li]

With the current codebase (i.e. concurrent backupItemBlock is not yet implemented), that might, in the short term, have a negative performance impact

Yes, agree. This is why we suggest to do this only in 1.16 where concurrent itemBlock processing is fully supported.

[ywk253100]

@Lyndon-Li @sseago
I'm going to merge this PR and cherry pick the changes to branch 1.15 to unblock the release 1.15.1.
Let's continue the discussion on issue #8516