Patching: filter and apply bin and lib patches separately, and fail explicitly if patching fails (fix #238 and #249)

[rdimaio]

fixes #238 - explicitly failing container setup when patching fails fixes #249 - filtering patches by directory in order to apply bin and lib patches separately

[rdimaio]

Looking at the container files:

• bin is symlinked to usr/bin
• lib is symlinked to usr/lib So we can apply the patch directly without filtering it, but we need to tell the patch command to follow symlinks. in the last commit, I reverted the changes that used filterdiff and instead simplified the approach by using --follow-symlinks when patching.

Output log when using some sample patches:

Patches found. Trying to apply them
patching file core/replica.py
Patch /patch/collection_replicas.patch applied.
patching file core/topology.py
patching file core/transfer.py
Hunk #3 succeeded at 311 (offset 13 lines).
Hunk #4 succeeded at 1418 (offset 59 lines).
Hunk #5 FAILED at 1500.
Hunk #6 succeeded at 1578 (offset 62 lines).
1 out of 6 hunks FAILED -- saving rejects to file core/transfer.py.rej
Patch /patch/conveyor.patch could not be applied successfully (exit code 1). Exiting setup.

[rdimaio]

I think https://github.com/rucio/containers/issues/249 could clarify this a bit better as right now it just says bin and lib without specifying the full paths.

To clarify, are these the absolute paths where the patches need to be applied?

• /usr/local/bin/ for bin
• /usr/local/lib/python3.9/site-packages/rucio for lib

In the server and daemon containers, even though it doesn't seem like these symlinks are explicitly created in the Dockerfile or any other files, bin and lib are symlinked like this:

I prefer the approach of using symlinks instead of using filterdiff, so I think we could just overwrite these default symlinks to the ones we need for patching, but if that doesn't work we can definitely just use filterdiff instead.

[rdimaio]

Checked locally, RUCIO_PYTHON_PATH is /usr/local/lib/python3.9/site-packages/rucio, so that can be used for the lib patch.

For bin, found some example patches in the commit history of flux-rucio with git grep "/bin/" $(git rev-list --all -- releases/integration/common-includes) -- releases/integration/common-includes. This one seems to be the most appropriate for testing: https://gitlab.cern.ch/atlas-adc-ddm/flux-rucio/-/blob/0b8071759cf87b2206186616f4b8c74325fa4591/releases/integration/common-includes/conveyor.patch - it patches both bin and lib files.

[rcarpa]

Just make your own patch on from the current branch by editing a file in bin; one in lib. And then doing git diff The conveyor patch will not apply on the current version of rucio.

[rdimaio]

Test patch used:

diff --git a/bin/rucio-minos b/bin/rucio-minos
index 21dc385f3..2ca9a5b1f 100755
--- a/bin/rucio-minos
+++ b/bin/rucio-minos
@@ -28,7 +28,7 @@ import signal

 from rucio.daemons.badreplicas.minos import run, stop

-
+changedline
 def get_parser():
     """
     Returns the argparse parser.
diff --git a/bin/rucio-undertaker b/bin/rucio-undertaker
index 15003ff85..ad52f9ade 100755
--- a/bin/rucio-undertaker
+++ b/bin/rucio-undertaker
@@ -64,7 +64,7 @@ Check if the DID exists::
     parser.add_argument('--sleep-time', action="store", default=60, type=int, help='Concurrency control: thread sleep time after each chunk of work')
     return parser

-
+changedline
 if __name__ == "__main__":

     signal.signal(signal.SIGTERM, stop)
diff --git a/lib/rucio/core/authentication.py b/lib/rucio/core/authentication.py
index cd5775ea3..dc548ede1 100644
--- a/lib/rucio/core/authentication.py
+++ b/lib/rucio/core/authentication.py
@@ -117,7 +117,7 @@ def get_auth_token_user_pass(account, username, password, appid, ip=None, *, ses
         models.Identity.identity_type == IdentityType.USERPASS
     )
     result = session.execute(query).scalar()
-
+changedline
     db_salt = result['salt']
     db_password = result['password']

diff --git a/lib/rucio/core/importer.py b/lib/rucio/core/importer.py
index ced3ed7a6..bed6a95ae 100644
--- a/lib/rucio/core/importer.py
+++ b/lib/rucio/core/importer.py
@@ -34,7 +34,7 @@ def import_rses(rses, rse_sync_method='edit', attr_sync_method='edit', protocol_
         rse = rses[rse_name]
         if isinstance(rse.get('rse_type'), str):
             rse['rse_type'] = RSEType(rse['rse_type'])
-
+changedline
         if rse_module.rse_exists(rse_name, vo=vo, include_deleted=False, session=session):
             # RSE exists and is active
             rse_id = rse_module.get_rse_id(rse=rse_name, vo=vo, session=session)

Output:

Patches found. Trying to apply them
Applying patch /patch/testpatch.patch
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff --git a/bin/rucio-minos b/bin/rucio-minos
|index 21dc385f3..2ca9a5b1f 100755
|--- a/bin/rucio-minos
|+++ b/bin/rucio-minos
--------------------------
patching file rucio-minos
Using Plan A...
Hunk #1 succeeded at 28.
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|diff --git a/bin/rucio-undertaker b/bin/rucio-undertaker
|index 15003ff85..ad52f9ade 100755
|--- a/bin/rucio-undertaker
|+++ b/bin/rucio-undertaker
--------------------------
patching file rucio-undertaker
Using Plan A...
Hunk #1 succeeded at 64.
done
Patch /patch/testpatch.patch/bin applied.
patching file core/authentication.py
patching file core/importer.py
Patch /patch/testpatch.patch/lib applied.

[rdimaio]

Considering that the idea with follow-symlinks came from you, I thought that you'll just so something like

TMP_PATCH_DIR="$(mktemp -d)"
mkdir "$TMP_PATCH_DIR/lib/"
ln -s "$RUCIO_PYTHON_PATH"  "$TMP_PATCH_DIR/lib/rucio"
ln -s /usr/local/bin/ "$TMP_PATCH_DIR/bin"

and then apply the patch using patch --verbose -p2 --follow-symlinks -d "$TMP_PATCH_DIR".

But the current version is also OK with me.

I was thinking of doing that initially, but noticed that the man page for patch says:

   --follow-symlinks
      When looking for input files, follow symbolic links.  Replaces
      the symbolic links, instead of modifying the files the
      symbolic links point to.  Git-style patches to symbolic links
      will no longer apply.  This option exists for backwards
      compatibility with previous versions of patch; **its use is
      discouraged.**

So I went back to your original idea of using filterdiff as it seems more appropriate here

[rdimaio]

Rebased a commit to remove the --verbose flag for consistency (I was only using it for bin patching while debugging)