Page MenuHomePhabricator

scap update-patch can abort and leave /srv/patches in a mess
Closed, ResolvedPublic

Description

Aborting: Failed to update patch: [Errno 1] Operation not permitted

^ This isn't really enough information to know how to act on it...

Details

Related Changes in GitLab:
TitleReferenceAuthorSource BranchDest Branch
patches.py: replace `shutil.copy2` with `shutil.copyfile`repos/releng/scap!1201jnucheT428316master
patches.py: replace `shutil.copy2` with `shutil.copy`repos/releng/scap!1200jnucheT428316master
Customize query in GitLab

Event Timeline

Looks like pretrain is "messing" this up again every night... trying to re-apply/move sec patches forward?

[17:01:35] <Krinkle> looks like it was indeed already applied to  1.47.0-wmf.5 on June 2nd, including the latest version
[17:01:53] <Krinkle> I will note that it is now dirty again on /srv/patches/next
[17:02:39] <Krinkle> and ls exactly at 02:00 again last night
[17:02:43] <Krinkle> that can't be a coincidence?
[17:03:22] <Krinkle> so something is either git stash popping (unlikely) or trying to make it match wmf.5 or something daily at 02:00 UTC?
[17:03:29] <Krinkle> and then leaving it conflicted
[18:00:57] <Reedy> It's probably the pre-train job doing something

The pretrain is indeed trying to copy patches from the latest version (1.47.0-wmf.5 in this case) to next every morning at 2:00 UTC, and Wednesday last week it started failing due to a permission error:

02:00:02 Copying <patch_path> to /srv/patches/next/<patch_path>
02:00:02 Finished scap prep next (duration: 00m 00s)
02:00:02 Unhandled error:
Traceback (most recent call last):
[...]
PermissionError: [Errno 1] Operation not permitted
02:00:02 prep failed: <PermissionError> [Errno 1] Operation not permitted (scap version: 4.267.0) (duration: 00m 00s)

That coincided with a new version of the T426631 patches, but I can't see what the mechanism for the failure is. I'm looking at the permissions of both 1.47.0-wmf.5 and next and they seem correct AFAICT.

The pretrain runs and fails with the permission error above which leaves things in a dirty state, then in subsequent runs it fails upon detecting the dirty git state. Folks in T426631 then stash the state manually to clean things up and the pretrain goes back to failing with the original error on its next run, leaving the repo dirty again.

I would try to run the pretrain now to try to troubleshoot but there's a backport window coming up. I will try to find time later to look into, we need to fix this for the next train. In the meantime I'm bringing this to the attention of @dancy

It seems like the cleanest option for me would be to make the prep job abort in case /srv/patches is dirty (or has stashed changes, as those cause problems?) as both of those usually but not always indicate that the changes should just be committed?

It seems like the cleanest option for me would be to make the prep job abort in case /srv/patches is dirty (or has stashed changes, as those cause problems?) as both of those usually but not always indicate that the changes should just be committed?

scap prep does abort when the repo is dirty:

02:00:07 Started scap prep next
02:00:07 Finished scap prep next (duration: 00m 00s)
Aborting: git is not clean: /srv/patches

I think we have two different issues here:

  • Prep should clean things up after itself on failure
  • Currently it's failing to copy the T426631 patches from 1.47.0-wmf.5 to next

scap prep ran successfully last night, that's one problem down.

We can go back to the less urgent issue originally reported in this task: "scap update-patch can abort and leave /srv/patches in a mess".

thcipriani assigned this task to jnuche.
thcipriani subscribed.

scap prep ran successfully last night, that's one problem down.

We can go back to the less urgent issue originally reported in this task: "scap update-patch can abort and leave /srv/patches in a mess".

scap update-patch leaving a mess is the same problem you solved. The first step of update-patch is the same as scap prep—ensure /srv/patches is up-to-date.

tl;dr:

  • Failure: scap update-patch is run as mwpresync. When scap update-patch copied files to /srv/patches/next it used shutil.copy2. shutil.copy2(src, target) updates target metadata (mtime, etc.) to match source. If target existed and was owned by someone other than mwpresync, updating metadata of the file failed.
  • Fix: use shutil.copyfile which changes a files contents, without updating mtime/permissions.

Lower-level details/example failure

  • /srv/patches/next/core/01-T123.patch exists, owned by thcipriani
  • thcipriani runs scap update-patch for /srv/patches/<previous-version>/core/01-T123.patch which works, patch is now different than what's in next and has a new update-time.
  • thcipraini runs scap update-patch for /srv/patches/next/core/01-T123.patch
    • First step: ensure the next directory is up-to-date with the previous version
    • Copy /srv/patches/<previous-version>/core/01-T123.patch/srv/patches/next/core/01-T123.patch (already exists; owned by thcipriani)
      • mwpresync runs shutil.copy2('/srv/patches/<previous-version>/core/01-T123.patch', '/srv/patches/next/core/01-T123.patch')
      • successfully copies the contents (since it can write the file).
      • The git workdir is now dirty.
      • Raises exception updating /srv/patches/next/core/01-T123.patch metadata, since mwpresync is not the owner of that file.
      • git commit never happens. State is dirty.

scap update-patch leaving a mess is the same problem you solved

I solved the immediate cause, not the root problem.

The first step of update-patch is the same as scap prep—ensure /srv/patches is up-to-date.

That was the immediate cause. If scap prep (or any another patches-related command) fails in the future again while performing patch file operations for whatever reason it can leave git in /srv/patches in a dirty state, causing a knock-on failure on e.g. backports. Scap should clean after itself in case of failure to avoid this problem.

I'll leave it to someone else to decide if they want to reopen this task or not.