core: Fails to start an incremental backup when the checkpoint id does not exist

[ArtiomDivak]

core: Fails to start an incremental backup when the checkpoint id does not exist

If a user tries to start an incremental backup with a checkpoint id that does not exist it will not fail from now on. The backup will be full instead of incremental.

Bug-Url: https://bugzilla.redhat.com/2097863 Signed-off-by: Artiom Divak [email protected]

[bennyz]

I'd still handle the checkpointId == null branch, just to log (or maybe even an audit log) for debugging

[barpavel]

I'd still handle the checkpointId == null branch, just to log (or maybe even an audit log) for debugging

I think audit log is preferred, and then keep the test (update to expect validation success) and maybe even add to the existing method audit log verification.

A better solution could be to add some flag like "ignore incremental backup failure" / "ignore missing checkpoint" that will allow user to decide if he/she wants to receive the error or ignore it (that is proceed to a full backup instead of incremental). But since we are towards EOL, it might be an overkill.

[bennyz]

A better solution could be to add some flag like "ignore incremental backup failure" / "ignore missing checkpoint" that will allow user to decide if he/she wants to receive the error or ignore it (that is proceed to a full backup instead of incremental). But since we are towards EOL, it might be an overkill.

It's not better. If you're thinking about the OVF failure flag, this scenario is different.

[barpavel]

A better solution could be to add some flag like "ignore incremental backup failure" / "ignore missing checkpoint" that will allow user to decide if he/she wants to receive the error or ignore it (that is proceed to a full backup instead of incremental). But since we are towards EOL, it might be an overkill.

It's not better. If you're thinking about the OVF failure flag, this scenario is different.

I think you misunderstood me. The existing checks were good and tested the right things. But the bug wants to solve "Customer uses some backup software for a vm backup" case and to allow automatic tools to run and succeed even if incremental backup failed for some reason by performing the full backup instead. So for those tools (or similar cases) I thought about adding some flag that can be specified if you just want the backup and are not interested whether it was incremental (as you requested) or full (due to some checkpoint issues). That is you are just interested in the end result and not how it was reached :)

[bennyz]

A better solution could be to add some flag like "ignore incremental backup failure" / "ignore missing checkpoint" that will allow user to decide if he/she wants to receive the error or ignore it (that is proceed to a full backup instead of incremental). But since we are towards EOL, it might be an overkill.

It's not better. If you're thinking about the OVF failure flag, this scenario is different.

I think you misunderstood me. The existing checks were good and tested the right things. But the bug wants to solve "Customer uses some backup software for a vm backup" case and to allow automatic tools to run and succeed even if incremental backup failed for some reason by performing the full backup instead. So for those tools (or similar cases) I thought about adding some flag that can be specified if you just want the backup and are not interested whether it was incremental (as you requested) or full (due to some checkpoint issues). That is you are just interested in the end result and not how it was reached :)

But that's not the point of the requested fix. The backup applications can already verify the existence of the checkpoint themselves as it's something available via API and do same thing this patch does, which they'll have to do anyway as it's not going in 4.5.3.

[barpavel]

A better solution could be to add some flag like "ignore incremental backup failure" / "ignore missing checkpoint" that will allow user to decide if he/she wants to receive the error or ignore it (that is proceed to a full backup instead of incremental). But since we are towards EOL, it might be an overkill.

It's not better. If you're thinking about the OVF failure flag, this scenario is different.

I think you misunderstood me. The existing checks were good and tested the right things. But the bug wants to solve "Customer uses some backup software for a vm backup" case and to allow automatic tools to run and succeed even if incremental backup failed for some reason by performing the full backup instead. So for those tools (or similar cases) I thought about adding some flag that can be specified if you just want the backup and are not interested whether it was incremental (as you requested) or full (due to some checkpoint issues). That is you are just interested in the end result and not how it was reached :)

But that's not the point of the requested fix. The backup applications can already verify the existence of the checkpoint themselves as it's something available via API and do same thing this patch does, which they'll have to do anyway as it's not going in 4.5.3.

Maybe I misunderstood this comment then :) https://bugzilla.redhat.com/show_bug.cgi?id=2097863#c8 "Because in the end the customer wants the vm backup and he doens't care about underhood details."

[ahadas]

yeah, I think it's for the best that we didn't get it in to 4.5.3. I wrote a detailed comment on the bug before closing it but in short, I think that from an API point of view it's ok that we return an error when failing to operate on the given parameters and the backup application should decide how to react to that if they want to change the user experience