Write helpful message to FailureMarker if script execution time was exceeded during stage life cycle

Created on 3 May 2023, over 1 year ago
Updated 16 May 2023, over 1 year ago

Problem/Motivation

Per #3357632-3: [META] Update doc comment on BatchProcessor to specify why use the batch system β†’ .

Steps to reproduce

Install an update and make it take a very long time, e.g. by having a LOT of files in place, or by reducing the max_execution_time.

Proposed resolution

Detect max execution time being exceeded and write a helpful message to the failure marker.

Remaining tasks

  1. βœ… β€” see #3 and , or via debugger:
  2. βœ…
  3. βœ…
  4. Maybe an automated test?

User interface changes

A helpful message will now appear if either there's too much I/O (lots of updates) or too slow I/O (slow network or filesystem)

API changes

None.

Data model changes

None.

πŸ“Œ Task
Status

Postponed: needs info

Version

3.0

Component

Package Manager

Created by

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Live updates comments and jobs are added and updated live.
  • Usability

    Makes Drupal easier to use. Preferred over UX, D7UX, etc.

  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Comments & Activities

  • Issue created by @wim leers
  • Open on Drupal.org β†’
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    Not currently mergeable.
  • @wim-leers opened merge request.
  • Assigned to wim leers
  • Status changed to Needs work over 1 year ago
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    765 pass
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    The commit I just pushed works in manual testing, but the automated test is very difficult to write. Will probably only finish that tomorrow.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    297 pass, 109 fail
  • Issue was unassigned.
  • Status changed to Needs review over 1 year ago
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Pushed what I got so far that AFAICT proves writing a Kernel test for this is impossible.

    Maybe I can write a functional test for this, but I've not been very successful so far 🫠

    Marking for the first commit.

  • Assigned to tedbow
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    I need to review

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    301 pass, 111 fail
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    I don't think we want to use Failure marker for this. The class doc states

    * The failure marker is a file placed in the active directory while staged
     * code is copied back into it, and then removed afterwards. This allows us to
     * know if a commit operation failed midway through, which could leave the site
     * code base in an indeterminate state -- which, in the worst case scenario,
     * might render Drupal being unable to boot.

    So this is for a very particular purpose, "commit operation failed midway through, which could leave the site
    * code base in an indeterminate state". I created πŸ› Document what the existence of the PACKAGE_MANAGER_FAILURE.yml means Fixed to make this clearer

    I don't we should reuse it for this. We stop all operations if this file exists. The purpose of this file is that you can't trust your site's code at that point.

    Of the 5 places we use this only during Apply would it leave your site's code base in an indeterminate state. But since it added to the first line of \Drupal\package_manager\StageBase::apply for all we know the time out is from very badly written PreApplyEvent subscriber. In that case we should not write the marker file. In StageBase::apply if call $this->committer->commit($stage_dir, $active_dir, $paths_to_exclude, NULL, $timeout); and we never return we will have already written the marker file and it will not be delete if the execution does not return to StageBase::apply. In that case still think we want to totally replace the file though in logExecutionTimeFailure as this MR does because it currently doesn't mention anything about restoring your site from a backup like \Drupal\package_manager\StageBase::getFailureMarkerMessage().

    I will make some more specific suggestions in the MR

  • Status changed to Needs work over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    @Wim Leers can you update the summary to state concisely what the problem is we are trying to address. The Problem/Motivation section currently just links to another issue comment and that issue comment then mentions the 1 above it and then links to yet issue

  • Status changed to Postponed: needs info over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    Postponing this because re #10 I don't have the whole context of the problem space we are trying to solve

  • Assigned to wim leers
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA
Production build 0.71.5 2024