Re: [PATCH stable-2.16 4/7] Make finalize_migration_{src,dst} 'atomic'

['Brian Foley' via ganeti-devel]

On Thu, Oct 13, 2016 at 01:16:39PM +0100, Viktor Bachraty wrote:
>On Thu, Oct 13, 2016 at 12:48 PM, 'Brian Foley' via ganeti-devel
><[1]ganeti-devel@googlegroups.com> wrote:
> 
>  On Fri, Oct 07, 2016 at 08:38:02PM +0100, 'Viktor Bachraty' via
>  ganeti-devel wrote:
>  This should definitely help cleaning up after migrates. LGTM modulo
>  nits below.
>  > FinalizeMigrationSource and FinalizeMigrationDst should compose an
>  > 'atomic' operation consisting of 2 idempotent steps, that can be
>  always
>  I'm not sure calling this 'atomic' sets the right expecation. For
>  these
>  operations, we want to not continue unless both of them succeed, but
>  we don't
>  guarantee to 'roll back' to the previous state if one or both ops
>  fail.
> 
>I was thinking calling it 'transactional' but that could also suggest
>that it either succeeds or rolls back. What I meant to say is that they
>compose a single operation, consisting of (potentially) parallel steps,
>that should be always attempted/retried together. Maybe I could use
>just what I said?

Sounds good! Sorry for the nitpick. As before, LGTM with these changes.

Re: [PATCH stable-2.16 4/7] Make finalize_migration_{src,dst} 'atomic'

['Viktor Bachraty' via ganeti-devel]

On Thu, Oct 13, 2016 at 12:48 PM, 'Brian Foley' via ganeti-devel <
ganeti-devel@googlegroups.com> wrote:

> On Fri, Oct 07, 2016 at 08:38:02PM +0100, 'Viktor Bachraty' via
> ganeti-devel wrote:
>
> This should definitely help cleaning up after migrates. LGTM modulo nits
> below.
>
> > FinalizeMigrationSource and FinalizeMigrationDst should compose an
> > 'atomic' operation consisting of 2 idempotent steps, that can be always
>
> I'm not sure calling this 'atomic' sets the right expecation. For these
> operations, we want to not continue unless both of them succeed, but we
> don't
> guarantee to 'roll back' to the previous state if one or both ops fail.
>

I was thinking calling it 'transactional' but that could also suggest that
it either succeeds or rolls back. What I meant to say is that they compose
a single operation, consisting of (potentially) parallel steps, that should
be always attempted/retried together. Maybe I could use just what I said?


>
> > both retried, similarly how they are used in AbortMigration() Otherwise
> > we would have to remember at which step did the migration fail in order
> > to be able to rollback/recover. Also state of the record should be
> updated
> > only if both succeeded, otherwise we won't know in case of recovery if
> the
> > record corresponds to the original state or the final state.
> >
> > Signed-off-by: Viktor Bachraty 
>
> > +# Always call finalize on both source and destination, it should be
> seen as
> s/destination/target/ (we use the term target everywhere else in the
> method)
>
> > +# Update instance location only after finalize completed. This way
> if the
> > +# instance runs on both nodes we know which one was the primary
> before the
>
> Maybe reword as "This way, if either finalize fails, the config still
> stores the
> old primary location, so sysadmins can know which instance to delete if
> they
> need to clean up."
>

Sounds good.

Re: [PATCH stable-2.16 4/7] Make finalize_migration_{src,dst} 'atomic'

['Brian Foley' via ganeti-devel]

On Fri, Oct 07, 2016 at 08:38:02PM +0100, 'Viktor Bachraty' via ganeti-devel 
wrote:

This should definitely help cleaning up after migrates. LGTM modulo nits below.

> FinalizeMigrationSource and FinalizeMigrationDst should compose an
> 'atomic' operation consisting of 2 idempotent steps, that can be always

I'm not sure calling this 'atomic' sets the right expecation. For these
operations, we want to not continue unless both of them succeed, but we don't
guarantee to 'roll back' to the previous state if one or both ops fail.

> both retried, similarly how they are used in AbortMigration() Otherwise
> we would have to remember at which step did the migration fail in order
> to be able to rollback/recover. Also state of the record should be updated
> only if both succeeded, otherwise we won't know in case of recovery if the
> record corresponds to the original state or the final state.
> 
> Signed-off-by: Viktor Bachraty 

> +# Always call finalize on both source and destination, it should be seen 
> as
s/destination/target/ (we use the term target everywhere else in the method)

> +# Update instance location only after finalize completed. This way if the
> +# instance runs on both nodes we know which one was the primary before 
> the

Maybe reword as "This way, if either finalize fails, the config still stores the
old primary location, so sysadmins can know which instance to delete if they
need to clean up."