Problem/Motivation

The migration system is well suited for periodic, automated re-runs but if a migration run fails, the status never gets reset. Blindly forcing a reset is not ideal because there's not really a good way to detect whether a migration is really running (short of stracing the relevant process but I highly doubt we want to resort to that).

Also, the current system is race prone because the PRE_IMPORT event fires between status check and status set. We have seen races even when the check and set were two instructions, one after the other, with an entire event between the two it's wide open.

Proposed resolution

We have a subsystem for this problem. The lock subsystem releases locks during shutdown and on top of that, they expire, too. Since backwards compatibility is a must, a new optional lock section can be added to the migration definition with the following keys:

lock:
  # This switches off the current behaviour of using status for locking
  use_status: FALSE
  # This switches on using the lock service.
  use_lock: TRUE 
  # This is the lock timeout for the source iterator initalization.
  initial_timeout: 60
  # This is the lock timeout for every row.
  timeout: 15

Remaining tasks

User interface changes

None at all.

API changes

New, optional section added to migration definitions.

Data model changes

None.

Release notes snippet

TODO.

CommentFileSizeAuthor
#40 3066277-40.patch7.88 KBandypost
#40 interdiff.txt1.63 KBandypost
#39 3066277-39.patch7.76 KBheddn
#39 interdiff.txt4.64 KBheddn
#38 3066277-38.patch7.87 KBandypost
#38 interdiff.txt3.43 KBandypost
#37 3066277_37.patch8.03 KBGhost of Drupal Past
#34 3066277_34.patch7.76 KBGhost of Drupal Past
#33 3066277_33.patch7.84 KBGhost of Drupal Past
#31 3066277_31.patch6.35 KBGhost of Drupal Past
#30 3066277_30.patch4.37 KBGhost of Drupal Past
#28 3066277_28.patch4.36 KBGhost of Drupal Past
#27 3066277_27.patch4.38 KBGhost of Drupal Past
#24 3066277_24.patch2.23 KBGhost of Drupal Past
#13 interdiff.txt3 KBGhost of Drupal Past
#13 3066277_13.patch14.98 KBGhost of Drupal Past
#12 3066277_12.patch13.17 KBGhost of Drupal Past
#8 3066277_8.patch7.06 KBGhost of Drupal Past
#7 3066277_6.patch7.26 KBGhost of Drupal Past
#2 3066277_2.patch4.1 KBGhost of Drupal Past
#3 3066277_3.patch5.72 KBGhost of Drupal Past
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Charlie ChX Negyesi created an issue. See original summary.

Ghost of Drupal Past’s picture

Ghost of Drupal Past’s picture

Issue summary: View changes
FileSize
5.72 KB
Ghost of Drupal Past’s picture

Issue summary: View changes

The last submitted patch, 2: 3066277_2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 3: 3066277_3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Ghost of Drupal Past’s picture

Status: Needs work » Needs review
FileSize
7.26 KB
Ghost of Drupal Past’s picture

FileSize
7.06 KB

Doh, I forgot to pass in the newly minted mocked lock.

The last submitted patch, 7: 3066277_6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

vijaycs85’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -144,18 +153,24 @@ protected function getEventDispatcher() {
    -    if ($this->migration->getStatus() !== MigrationInterface::STATUS_IDLE) {
    

    AFAICS, the STATUS_IDLE exists to serve just this purpose. Does it mean, we could deprecate and remove it?

  2. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -196,6 +211,13 @@ public function import() {
    +      $newTime = time();
    +      if ($newTime - $time > self::TIMEOUT/2) {
    +        if (!$this->lock()->acquire($this->migration->id(), self::TIMEOUT)) {
    +          return $this->displayError();
    +        }
    +        $time = $newTime;
    +      }
    
    @@ -305,6 +328,13 @@ public function rollback() {
    +      $newTime = time();
    +      if ($newTime - $time > self::TIMEOUT/2) {
    +        if (!$this->lock()->acquire($this->migration->id(), self::TIMEOUT)) {
    +          return $this->displayError();
    +        }
    +        $time = $newTime;
    +      }
    

    Not sure:
    a) why we need to do this
    b) part of this issue scope
    c) if we have to:
    1. Add comment to explain (esp why timeout/2).
    2. probably add test coverage for cover this scenario?
    d) Wonder if we need CR for this change.

  3. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -22,6 +23,8 @@
    +  const TIMEOUT = 30;
    

    Missing docblock.

Ghost of Drupal Past’s picture

AFAICS, the STATUS_IDLE exists to serve just this purpose. Does it mean, we could deprecate and remove it?

Not sure. UIs report the status -- drush surely does -- but I guess lockMayBeAvailable can be used to detect idleness.

As for re-locking: while I could grab the lock with a very long timeout, I don't want to do that as it would lessen the advantages of using the lock subsystem. Rather, I grab the lock for a shorter period of time, renew it well before it expires , the half timeout is just as good as anything else, I could've chosen .8 or .9 instead of .5 but it doesn't matter much. Because we hold the lock only for half a minute, if PHP dies so abrutly the locks aren't released, the migration is ready to re-run in thirty seconds. I am not sure what change record is necessary here -- maybe there is , migration no longer need a status reset for sure but there's no harm in status resetting. I guess the CR is necessary to explain if you are really fast in re-run the status reset is no longer adequate -- either wait for the lock to time out or delete it from the database. I guess drush can add the latter to the status reset. Pity the generic lock backend doesn't offer a break functionality.

Ghost of Drupal Past’s picture

Status: Needs work » Needs review
FileSize
13.17 KB

I haven't deprecated STATUS_IDLE yet but if we want to, I can. This has a unit test, I factored out testImportWithValidRow and used it for two new tests. Is this the first usage of the splat operator in core? It's PHP 5.6 and we are 7.0 so I am not worried.

Ghost of Drupal Past’s picture

Issue summary: View changes
FileSize
14.98 KB
3 KB

mikeleutz raised a concern on Slack where the current system is actually useful and one wants to check things before manually resetting the status. So we have two different use cases here which calls for an option. Here.

heddn’s picture

In general I think using lock system has more pros than cons. I'm +1 in favor.

  1. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -19,9 +20,14 @@
    +   * Number of sceonds the lock is held.
    

    Nit: spelling

  2. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -102,7 +127,7 @@ class MigrateExecutable implements MigrateExecutableInterface {
    -  public function __construct(MigrationInterface $migration, MigrateMessageInterface $message = NULL, EventDispatcherInterface $event_dispatcher = NULL) {
    +  public function __construct(MigrationInterface $migration, MigrateMessageInterface $message = NULL, EventDispatcherInterface $event_dispatcher = NULL, LockBackendInterface $lock = NULL) {
    

    While strictly not part of BC, there are several multi thousand install contrib modules that extend the executable and this would hard break them.

Ghost of Drupal Past’s picture

Thanks heddn for the review and support.

The constructor is exempt: compare https://3v4l.org/sZLun to https://3v4l.org/lbP4V and the lock method fills in the lock service from Drupal::lock() if necessary.

I will fix the typo or it could be fixed on commit if there is nothing else.

heddn’s picture

Status: Needs review » Needs work

Ignore #14.2. I see that isn't an issue.

A more thorough review of the rest of the patch:

  1. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -19,9 +20,14 @@
    +  const TIMEOUT = 30;
    

    Let's call this lock_timeout; more explicit.

  2. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -144,18 +170,26 @@ protected function getEventDispatcher() {
    +    if ($this->migration->getStatus() !== MigrationInterface::STATUS_IDLE && !$this->bypassStatus) {
    
    @@ -286,9 +324,11 @@ public function import() {
    +    if ($this->migration->getStatus() !== MigrationInterface::STATUS_IDLE && !$this->bypassStatus) {
    

    This could (should?) use the getter for bypass status?

  3. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -414,6 +458,21 @@ public function processRow(Row $row, array $process = NULL, $value = NULL) {
    +  public function setBypassStatus($bypassStatus) {
    

    Nit: $bypassStatus variable name should be $bypass_status, I think.

  4. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -572,4 +631,49 @@ protected function formatSize($size) {
    +    return 0;
    

    Should this return MigrationInterface::STATUS_IDLE instead of magic number?

  5. +++ b/core/modules/migrate/src/BypassableStatusMigrateExecutableInterface.php
    @@ -0,0 +1,26 @@
    +interface BypassableStatusMigrateExecutableInterface extends MigrateExecutableInterface {
    
    +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -19,9 +20,14 @@
    +class MigrateExecutable implements BypassableStatusMigrateExecutableInterface {
    

    Since we have a single implementation of the interface, could we just added these methods to the base interface since the base class will automatically implement? I think that is still safe for BC.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Ghost of Drupal Past’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.23 KB

Rebooting with a new, configurable, backwards compatible, very simple approach: a new optional lock section can be added to the migration definition with the following keys:

lock:
  # This switches off the current behaviour of using status for locking
  use_status: FALSE
  # This switches on using the lock service.
  use_lock: TRUE 
  # This is the lock timeout for the source iterator initalization.
  initial_timeout: 60
  # This is the lock timeout for every row.
  timeout: 15

Status: Needs review » Needs work

The last submitted patch, 24: 3066277_24.patch, failed testing. View results

andypost’s picture

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -142,12 +150,26 @@ protected function getEventDispatcher() {
 
+  protected function getLock(): LockBackendInterface {
...
+    $lock_definition = $this->migration->getPluginDefinition()['lock'] ?? [];
+    if ($use_lock = !empty($lock_definition['use_lock'])) {
+      if (!$this->lock->acquire($this->migration->id(), $lock_definition['initial_timeout'] ?? 30)) {

@@ -288,6 +310,10 @@ public function import() {
+          // Extend our lock.
+          if ($use_lock) {
+            $this->lock->acquire($this->migration->id(), $lock_definition['timeout'] ?? 30);

this method needs doc-block.

is it really required to be public as looking unused.

Ghost of Drupal Past’s picture

Status: Needs work » Needs review
FileSize
4.38 KB
Ghost of Drupal Past’s picture

FileSize
4.36 KB
andypost’s picture

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -142,12 +150,31 @@ protected function getEventDispatcher() {
+  protected function getLock(): LockBackendInterface {

+++ b/core/modules/migrate/tests/src/Kernel/MigrateExecutableTest.php
@@ -75,6 +76,25 @@ public function testMigrateExecutable() {
+    $executable->setLock($lock);

+++ b/core/modules/migrate/tests/src/Kernel/TestMigrateExecutable.php
@@ -25,4 +26,8 @@ protected function getSource() {
+  public function setLock(LockBackendInterface $lock) {

still not sure the getter is needed, you can use setAccessible() via reflection for the test

Ghost of Drupal Past’s picture

FileSize
4.37 KB
Ghost of Drupal Past’s picture

FileSize
6.35 KB

I forgot to do rollback.

andypost’s picture

It looks ready just a nitpick

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -470,6 +505,35 @@ protected function processPipeline(Row $row, string $destination, array $plugins
+  public function acquireLock($lock_configuration, $is_initial): bool {

method arguments needs types as it's new

Ghost of Drupal Past’s picture

FileSize
7.84 KB
  1. There was already duplicate code at the beginning of import/rollback, this patch is adding more, I moved these into a method.
  2. In the new method I added defaults for the lock configuration, this is a convenient place to document the four new keys. Yay, documentation.
  3. Inlined getLock because it was only used once in acquireLock.
Ghost of Drupal Past’s picture

andypost’s picture

Looks ready except debatable

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -90,6 +91,13 @@ class MigrateExecutable implements MigrateExecutableInterface {
+   * The lock backend.
+   *
+   * @var \Drupal\Core\Lock\LockBackendInterface
+   */
+  protected $lock;

I think as new code it should be typed as `protected LockBackendInterface $lock;`

heddn’s picture

  1. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -470,6 +476,62 @@ protected function processPipeline(Row $row, string $destination, array $plugins
    +      $this->lock = \Drupal::lock();
    

    Can this be injected?

  2. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -470,6 +476,62 @@ protected function processPipeline(Row $row, string $destination, array $plugins
    +  protected function initialize(StoppableEventInterface $event, string $event_name): array|int {
    

    Why do we need to return an int|array? Can we return just a bool?

Ghost of Drupal Past’s picture

FileSize
8.03 KB

I tried typing it above in a patch that's when phpcs failed, let me try again. But if we are typing our properties, do we have an issue to drop absolutely useless and superfluous doxygen? There's no point in adding "the lock backend" to `protected LockBackendInterface $lock`. That's just ridiculous.

Could we inject the lock backend? Of course it's possible. But MigrateExecutable is instantiated directly, there's no factory, there's no service. So unlike other classes where a new constructor argument was added and it caused little upheaval, here every test and every runner would need to be upgraded to avoid a deprecation error. And for what? By default it's not even used.

Why do we need to return an int|array? Because of this section

          // Extend the lock.
          if ($lock_configuration) {
            $this->acquireLock($lock_configuration['timeout']);
          }

That's what makes the array part necessary. We need to convey somehow the lock is in use and how long the per row locking should be. If we so wanted we could implement a new exception thrown in initialize and caught in import/rollback. -- but that, again, seemed like a lot of work for very little benefit.

andypost’s picture

I think lock should be injected via constructor as event dispatcher doing, it allows to simplify code a bit

Meantime withConsecutive() very probably will be deprecated so better to beware it see related #3306554: InvocationMocker::withConsecutive() is deprecated in PHPUnit 9.6 and removed from PHPUnit 10

heddn’s picture

This doesn't overload the result from initialize()

andypost’s picture

Fixed CS and add doc-block for method

Status: Needs review » Needs work

The last submitted patch, 40: 3066277-40.patch, failed testing. View results

Ghost of Drupal Past’s picture

Assigned: Ghost of Drupal Past » Unassigned

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Ghost of Drupal Past’s picture

Thanks for all input. I am deploying #37 to production. That comment explains why I am against adding an argument to the constructor and as for #39, I didn't feel like I needed a separate method to get the lock configuration. This is just a feeling of course. Please do not let this issue die despite I am not going to work on it further.