fix: change periodic_remove condition and fix parseCondorStatus

Author: aldbr

src/DIRAC/Resources/Computing/BatchSystems/Condor.py

@@ -23,7 +23,7 @@
     5: "Failed",
 }
 
-HOLD_REASON_SUBCODE = 55
+HOLD_REASON_SUBCODE = "55"
 
 subTemplate = """
 # Environment
@@ -71,7 +71,7 @@
 # A random subcode to identify who put the job on hold
 on_exit_hold_subcode = %(holdReasonSubcode)s
 # Jobs are then deleted from the system after N days if they are not running
-period_remove = (JobStatus != 2) && (time() - EnteredCurrentStatus) > (%(daysToKeepRemoteLogs)s * 24 * 3600)
+period_remove = (JobStatus != 1) && (JobStatus != 2) && ((time() - EnteredCurrentStatus) > (%(daysToKeepRemoteLogs)s * 24 * 3600))
 
 # Specific options
 # ----------------
@@ -94,13 +94,17 @@ def parseCondorStatus(lines, jobID):
     :returns: Status as known by DIRAC, and a reason if the job is being held
     """
     jobID = str(jobID)
+
+    holdReason = ""
     for line in lines:
         l = line.strip().split()
+
+        # Make sure the status is present and is an integer
         try:
             status = int(l[1])
         except (ValueError, IndexError):
             continue
-        holdReason = ""
+
         if l[0] == jobID:
             # A job can be held for many various reasons, we need to further investigate with the holdReasonCode & holdReasonSubCode
             # Details in:
@@ -110,22 +114,22 @@ def parseCondorStatus(lines, jobID):
                 # By default, a held (5) job is defined as Aborted, but there might be some exceptions
                 status = 3
                 try:
-                    holdReasonCode = int(l[2])
-                    holdReasonSubcode = int(l[3])
-                    holdReason = l[4:]
-                except (ValueError, IndexError):
+                    holdReasonCode = l[2]
+                    holdReasonSubcode = l[3]
+                    holdReason = " ".join(l[4:])
+                except IndexError:
                     # This should not happen in theory
                     # Just set the status to unknown such as
                     status = -1
-                    holdReasonCode = -1
-                    holdReasonSubcode = -1
+                    holdReasonCode = "undefined"
+                    holdReasonSubcode = "undefined"
 
                 # If holdReasonCode is 3 (The PERIODIC_HOLD expression evaluated to True. Or, ON_EXIT_HOLD was true)
                 # And subcode is HOLD_REASON_SUBCODE, then it means the job failed by itself, it needs to be marked as Failed
-                if holdReasonCode == 3 and holdReasonSubcode == HOLD_REASON_SUBCODE:
+                if holdReasonCode == "3" and holdReasonSubcode == HOLD_REASON_SUBCODE:
                     status = 5
                 # If holdReasonCode is 16 (Input files are being spooled), the job should be marked as Waiting
-                elif holdReasonCode == 16:
+                elif holdReasonCode == "16":
                     status = 1
 
             return (STATES_MAP.get(status, "Unknown"), holdReason)

src/DIRAC/Resources/Computing/HTCondorCEComputingElement.py

@@ -272,7 +272,7 @@ def _executeCondorCommand(self, cmd, keepTokenFile=False):
             self.tokenFile = None
             # We have got a non-zero status code
             errorString = stderr if stderr else stdout
-            return S_ERROR(f"Command", f"{cmd} failed with: {status} - {errorString.strip()}")
+            return S_ERROR(f"Command {cmd} failed with: {status} - {errorString.strip()}")
 
         # Remove token file if we do not want to keep it
         self.tokenFile = self.tokenFile if keepTokenFile else None
@@ -413,7 +413,7 @@ def getJobStatus(self, jobIDList):
 
         qList = []
         for _condorIDs in breakListIntoChunks(condorIDs.values(), 100):
-            # This will return a list of 1245.75 3
+            # This will return a list of 1245.75 3 undefined undefined undefined
             cmd = ["condor_q"]
             cmd.extend(self.remoteScheddOptions.strip().split(" "))
             cmd.extend(_condorIDs)

src/DIRAC/Resources/Computing/test/Test_HTCondorCEComputingElement.py

@@ -61,7 +61,7 @@ def test_parseCondorStatus():
         "foo": "Unknown",
         "104096.1": "Aborted",
         "104096.2": "Aborted",
-        "104096.3": "Unknown",
+        "104096.3": "Aborted",
         "104096.4": "Unknown",
     }
 
@@ -73,7 +73,7 @@ def test_parseCondorStatus():
 def test_getJobStatus(mocker):
     """Test HTCondorCE getJobStatus"""
     mocker.patch(
-        MODNAME + ".HTCondorCEComputingElement._executeCondorCommand",
+        MODNAME + ".executeGridCommand",
         side_effect=[
             S_OK((0, "\n".join(STATUS_LINES), "")),
             S_OK((0, "\n".join(HISTORY_LINES), "")),
@@ -82,8 +82,12 @@ def test_getJobStatus(mocker):
         ],
     )
     mocker.patch(MODNAME + ".HTCondorCEComputingElement._HTCondorCEComputingElement__cleanup")
+    mocker.patch(MODNAME + ".HTCondorCEComputingElement._prepareProxy", return_value=S_OK())
 
     htce = HTCE.HTCondorCEComputingElement(12345)
+    # Need to initialize proxy because it is required by executeCondorCommand()
+    htce.proxy = "dumb_proxy"
+
     ret = htce.getJobStatus(
         [
             "htcondorce://condorce.foo.arg/123.0:::abc321",
@@ -99,6 +103,7 @@ def test_getJobStatus(mocker):
         "htcondorce://condorce.foo.arg/123.2": "Aborted",
         "htcondorce://condorce.foo.arg/333.3": "Unknown",
     }
+    print(ret)
     assert ret["OK"] is True
     assert expectedResults == ret["Value"]
 
@@ -153,18 +158,18 @@ def test__writeSub(mocker, localSchedd, optionsNotExpected, optionsExpected):
 
 
 @pytest.mark.parametrize(
-    "localSchedd, expected", [(False, "-pool condorce.cern.ch:9619 -name condorce.cern.ch"), (True, "")]
+    "localSchedd, expected", [(False, "-pool condorce.cern.ch:9619 -name condorce.cern.ch "), (True, "")]
 )
 def test_reset(setUp, localSchedd, expected):
     ceParameters = setUp
 
     htce = HTCE.HTCondorCEComputingElement(12345)
     htce.ceParameters = ceParameters
-    htce.useLocalSchedd = True
+    htce.useLocalSchedd = localSchedd
     ceName = "condorce.cern.ch"
     htce.ceName = ceName
     htce._reset()
-    assert htce.remoteScheddOptions == ""
+    assert htce.remoteScheddOptions == expected
 
 
 @pytest.mark.parametrize(
@@ -179,12 +184,12 @@ def test_submitJob(setUp, mocker, localSchedd, expected):
     htce = HTCE.HTCondorCEComputingElement(12345)
     htce.ceParameters = ceParameters
     htce.useLocalSchedd = localSchedd
+    htce.proxy = "dumb_proxy"
     ceName = "condorce.cern.ch"
     htce.ceName = ceName
 
-    execMock = mocker.patch(
-        MODNAME + ".HTCondorCEComputingElement._executeCondorCommand", return_value=S_OK((0, "123.0 - 123.0", ""))
-    )
+    execMock = mocker.patch(MODNAME + ".executeGridCommand", return_value=S_OK((0, "123.0 - 123.0", "")))
+    mocker.patch(MODNAME + ".HTCondorCEComputingElement._prepareProxy", return_value=S_OK())
     mocker.patch(
         MODNAME + ".HTCondorCEComputingElement._HTCondorCEComputingElement__writeSub", return_value="dirac_pilot"
     )
@@ -212,13 +217,13 @@ def test_killJob(setUp, mocker, jobIDList, jobID, ret, success, local):
     ceParameters = setUp
     htce = HTCE.HTCondorCEComputingElement(12345)
     htce.ceName = "condorce.foo.arg"
+    htce.proxy = "dumb_proxy"
     htce.useLocalSchedd = local
     htce.ceParameters = ceParameters
     htce._reset()
 
-    execMock = mocker.patch(
-        MODNAME + ".HTCondorCEComputingElement._executeCondorCommand", return_value=S_OK((ret, "", ""))
-    )
+    execMock = mocker.patch(MODNAME + ".executeGridCommand", return_value=S_OK((ret, "", "")))
+    mocker.patch(MODNAME + ".HTCondorCEComputingElement._prepareProxy", return_value=S_OK())
 
     ret = htce.killJob(jobIDList=jobIDList)
     assert ret["OK"] == success