> 2. infrastructural issue - the AddVmFromTemplate command isn't marked as one
> requires compensation context, yet one is still created (that's handled by
> the infrastructure).
> 
> 3. AddVmFromTemplate should be using a compensation context (so it should be
> marked as command using it).
> 
> I'm cloning this bug to a new bug to tackle to infrastructural issues, this
> flow should tackle the flow related issues.

AddVmFromTemplate is not using compensation although it's calling parent AddVmCommand, which is marked as @NonTransactiveCommandAttribute and which is using compensations properly.
Arik, could you please take a look?

(In reply to Martin Perina from comment #1)
> AddVmFromTemplate is not using compensation although it's calling parent
> AddVmCommand, which is marked as @NonTransactiveCommandAttribute and which
> is using compensations properly.
> Arik, could you please take a look?

Indeed, seems like a bug.

Hi Martin/Arik - I've left the original bug to tackle the flow issue.
This one is about the infrastructural issue - the current code of CommandBase.createCompensationContext() is attached ([1]). You can see that if the transaction scope isn't Suppress the default compensation context is always created even if the command wasn't marked to use compensation (in that case the NoOpCompensationContext should be returned).

[1]
    private CompensationContext createCompensationContext(TransactionScopeOption transactionScopeOption,
            boolean forceCompensation) {
        if (transactionScopeOption == TransactionScopeOption.Suppress && !forceCompensation) {
            return NoOpCompensationContext.getInstance();
        }

        return createDefaultCompensationContext();
    }

Liron, createCompensationContext() is called from handleTransactivity() and if command is marked with @NonTransactiveCommandAttribute then we always set TranscationScope to Suppress:

    private void handleTransactivity() {
        scope =
                (getParameters() != null) ? getParameters().getTransactionScopeOption()
                        : TransactionScopeOption.Required;
        endActionScope = scope;
        boolean forceCompensation = getForceCompensation();
        // @NonTransactiveAttribute annotation overrides the scope passed by the
        // command parameters
        if (!getTransactive()) {
            scope = TransactionScopeOption.Suppress;

            // Set the end action scope to suppress only for non-compensating commands, or the end action for commands
            // will run without transaction but compensation is not supported for end action.
            endActionScope = forceCompensation ? endActionScope : scope;
        }

        if (getCompensationContext() == null) {
            context.withCompensationContext(createCompensationContext(scope, forceCompensation));
        }
    }

So the only issue here is, that AddVmFromTemplate have to be marked with @NonTransactiveCommandAttribute, right? Or am I missing something?

Martin, I'll elaborate-
Let's assume we have a command X which isn't marked with any annotation, in that case DefaultCompensationContext will be created for the command instead of NoOpCompensationContext (as this command isn't marked to be using compensation - there's no annotation).

I've left the original bug (that this one was cloned from) to fix AddVmFromTemplate.

(In reply to Liron Aravot from comment #5)
> Martin, I'll elaborate-
> Let's assume we have a command X which isn't marked with any annotation, in
> that case DefaultCompensationContext will be created for the command instead
> of NoOpCompensationContext (as this command isn't marked to be using
> compensation - there's no annotation).

I've discussed with Moti:

1. If command is annotated @NonTransactiveCommandAttribute(forceCompensation = true) then we create default compensation context and execute compensation flow for it

2. If the command is annotated only with @NonTransactiveCommandAttribute, then we create with NoOpCompensationContext

3. TransactionScopeOption can be also defined in command parameters, so if the command is using TransactionScopeOption.Suppress, we will create it with NoCompensationScope (by default TransactionScopeOption is set to Required)


Liron, what you are saying is true, but hypothetical. Or do you have any valid use case for that?

I'm asking because at the moment we don't have any reliable verification steps or test cases that changing createCompensationContext() will not break anything in current code. So it's IMO it's better to fix problematic commands then doing change to fix hypothetical issue. So suggesting to close this one as WONTFIX.

> 
> I've left the original bug (that this one was cloned from) to fix
> AddVmFromTemplate.

Arik, please fix AddVmFromTemplace using BZ1399191

(In reply to Liron Aravot from comment #5)
> Martin, I'll elaborate-
> Let's assume we have a command X which isn't marked with any annotation, in
> that case DefaultCompensationContext will be created for the command instead
> of NoOpCompensationContext (as this command isn't marked to be using
> compensation - there's no annotation).
> 
> I've left the original bug (that this one was cloned from) to fix
> AddVmFromTemplate.

That means the only case for no-op compensation context occurs when running command in suppress mode without forcing compensation.

Any other combination, i.e. required or required-new transaction will expect any sort of compensation context, which will be used *only* if the command actually took a snapshot of the compensating entities.

So for commands, it makes less sense not to declare the no-transaction annotation and to expect compensation to exist.

I wouldn't consider this a bug, since if we're dealing with a child-command, and we wish not to invoke any compensation for it, we should set it to no-op when we're invoke that child-command internally (assuming not compensation data was recorded by the command, and the only reason for it is inheriting it from the parent command).

4.0.6 has been the last oVirt 4.0 release, please re-target this bug.

> 
> Liron, what you are saying is true, but hypothetical. Or do you have any
> valid use case for that?
> 
> I'm asking because at the moment we don't have any reliable verification
> steps or test cases that changing createCompensationContext() will not break
> anything in current code. So it's IMO it's better to fix problematic
> commands then doing change to fix hypothetical issue. So suggesting to close
> this one as WONTFIX.
> 
> > 
> > I've left the original bug (that this one was cloned from) to fix
> > AddVmFromTemplate.
> 

Thanks Martin,
Yes - there are use cases, for example bz 1418648 is caused by that issue (without getting into whether the command code should be changed).
We are in a complex situation, perhaps we can do the following -
a. add a RequireCompensationContext annotation which will enforce creation of the default compensation context.
b. use that annotation in the current flows affected by this bug.
c. change the implementation of handleTransactivity() to not create the compensation context on those cases.

that way we'll be able to know that new flows aren't relying on that behavior while we'll be able to fix/inspect the old flows one by one and to remove the annotation usage.

I do agree that it may be an overkill - but we do have such cases (intentional or not..) - we need to scope the amount of work required for that.

(In reply to Moti Asayag from comment #7
> 
> That means the only case for no-op compensation context occurs when running
> command in suppress mode without forcing compensation.
> 
> Any other combination, i.e. required or required-new transaction will expect
> any sort of compensation context, which will be used *only* if the command
> actually took a snapshot of the compensating entities.

Indeed, the problem is that many times the code is shared between compensation/non compensating commands- causing to the CompensationContext.snapshot*() calls.

> I wouldn't consider this a bug, since if we're dealing with a child-command,
> and we wish not to invoke any compensation for it, we should set it to no-op
> when we're invoke that child-command internally (assuming not compensation
> data was recorded by the command, and the only reason for it is inheriting
> it from the parent command).

I'm not really in favor of it, that means that every parent will have to remember to set the compensation context when executing a specific child command to not encounter that bug.

After offline discussion we agreed that changing the code as suggested above is too dangerous, because we have no reliable way how to verify that we won't break existing flows. Because the issue caused by current status is corner case and it could be avoided by more careful reviews.

Due to the delicate nature of the code around the compensation and transaction handling, we (me and Liron) decided to fix bug 1418648 by changing the specific command within it and to leave the compensation framework intact.