Re: [PATCH ha-manager 1/7] fix #8059: manager: auto rebalance: back off failed motions and add per-resource cooldown

[Michael Ryom <Michael@RyomHerold.dk>]

Hi Daniel,

thanks a lot for the thorough review.

> Most of these are inferrable by the guest config alone, so I'm
> working on a patch series which automatically encodes these
> constraints obtained from the guest's config files

Good to hear. I had drafted a precheck that mirrors the hard failures
of QemuMigrate/LXC::Migrate (local CD-ROM/files/devices, storage
availability per node) as a get_migration_blockers() plugin method,
evaluated once per balancing round. I won't send it, as yours will
cover it - but if any part of it is useful to you, I'm happy to share
it or test your series on the cluster where the issue showed up.

> This makes me believe that the exponential backoff isn't solving this
> problem well [...] I think it would be better to just remove the
> migration candidates that failed in previous attempts in the same
> series of trying out a rebalancing migration alltogether and only
> retry them in another series of rebalancing migrations.

You're right - I could reproduce the fa:109 cycle exactly as you
described it (node2, node3, node2, ... six failed attempts before
vm:102 was tried). v2 implements your suggestion: a failed motion is
not tried again in the same rebalancing attempt, it is considered again
in the next attempt, and if it fails again, it is skipped for 2, 4, 8,
... (at most 32) of the following attempts. An attempt is concluded
when a rebalance motion succeeds, when there is nothing left to try or
when the imbalance drops below the threshold. The fa:109 variant is now
the test case added in patch 3/14, and a second test case covers the case
where the failing motion is the only one possible.

> Otherwise, I'd tend to record any failed HA resource motion when it
> was reported as unsuccessful by the LRM in
> next_state_migrate_relocate().

Done, restricted to motions issued by load_balance(). As every
resource moved by the balancer is recorded, this also covers the
dependent resources of a bundle, which addresses your point about a
leader change resetting the backoff: a bundle candidate is skipped for
a target node if any of its resources failed to move there.

> The separate cooldown for previous rebalancing migrations should go
> in its own patch

Done (patches 5-6), and it is now an option (ha-auto-rebalance-
cooldown) which defaults to 0, so existing setups behave as before.

> IMO a patch series that changes some behavior of an existing system
> should add test cases in a patch before that change

Done for all behavior changes in v2.

> nit: I'd prefer `my sub name {}` [...]
> This should be factored out from the similar
> any_resource_motion_queued_or_running() [...]
> nit: use two lines for this

All done.

Best regards,
Michael