From b4a87d98a139f0268d43b051faafce6e52a169f8 Mon Sep 17 00:00:00 2001 From: Mohamed Bilel Besrour <58034472+BBesrour@users.noreply.github.com> Date: Fri, 30 Aug 2024 10:37:06 +0300 Subject: [PATCH] Development: Remove fenced locks in integrated code lifecycle (#9180) --- gradle.properties | 3 +- .../LocalCIResultProcessingService.java | 6 - .../localci/SharedQueueManagementService.java | 108 ++++++------------ .../SharedQueueProcessingService.java | 17 +-- 4 files changed, 35 insertions(+), 99 deletions(-) diff --git a/gradle.properties b/gradle.properties index 99ad42379403..948cea0f35c8 100644 --- a/gradle.properties +++ b/gradle.properties @@ -15,8 +15,7 @@ hibernate_version=6.4.9.Final opensaml_version=4.3.2 jwt_version=0.12.6 jaxb_runtime_version=4.0.5 -# TODO: we cannot update to 5.5.0 because we currently use the CP Subsystem for fenced locks, however CP Subsystem is only available to Enterprise customers -hazelcast_version=5.4.0 +hazelcast_version=5.5.0 junit_version=5.10.2 mockito_version=5.13.0 fasterxml_version=2.17.2 diff --git a/src/main/java/de/tum/in/www1/artemis/service/connectors/localci/LocalCIResultProcessingService.java b/src/main/java/de/tum/in/www1/artemis/service/connectors/localci/LocalCIResultProcessingService.java index fd7e0cc0dc89..dfc7dcf776c2 100644 --- a/src/main/java/de/tum/in/www1/artemis/service/connectors/localci/LocalCIResultProcessingService.java +++ b/src/main/java/de/tum/in/www1/artemis/service/connectors/localci/LocalCIResultProcessingService.java @@ -20,7 +20,6 @@ import com.hazelcast.collection.ItemEvent; import com.hazelcast.collection.ItemListener; import com.hazelcast.core.HazelcastInstance; -import com.hazelcast.cp.lock.FencedLock; import com.hazelcast.map.IMap; import de.tum.in.www1.artemis.domain.BuildJob; @@ -73,8 +72,6 @@ public class LocalCIResultProcessingService { private IMap buildAgentInformation; - private FencedLock resultQueueLock; - private UUID listenerId; public LocalCIResultProcessingService(@Qualifier("hazelcastInstance") HazelcastInstance hazelcastInstance, ProgrammingExerciseGradingService programmingExerciseGradingService, @@ -97,7 +94,6 @@ public LocalCIResultProcessingService(@Qualifier("hazelcastInstance") HazelcastI public void init() { this.resultQueue = this.hazelcastInstance.getQueue("buildResultQueue"); this.buildAgentInformation = this.hazelcastInstance.getMap("buildAgentInformation"); - this.resultQueueLock = this.hazelcastInstance.getCPSubsystem().getLock("resultQueueLock"); this.listenerId = resultQueue.addItemListener(new ResultQueueListener(), true); } @@ -112,9 +108,7 @@ public void removeListener() { public void processResult() { // set lock to prevent multiple nodes from processing the same build job - resultQueueLock.lock(); ResultQueueItem resultQueueItem = resultQueue.poll(); - resultQueueLock.unlock(); if (resultQueueItem == null) { return; diff --git a/src/main/java/de/tum/in/www1/artemis/service/connectors/localci/SharedQueueManagementService.java b/src/main/java/de/tum/in/www1/artemis/service/connectors/localci/SharedQueueManagementService.java index bdaea1f46fd5..939fa41cee19 100644 --- a/src/main/java/de/tum/in/www1/artemis/service/connectors/localci/SharedQueueManagementService.java +++ b/src/main/java/de/tum/in/www1/artemis/service/connectors/localci/SharedQueueManagementService.java @@ -27,7 +27,6 @@ import com.hazelcast.collection.IQueue; import com.hazelcast.core.HazelcastInstance; -import com.hazelcast.cp.lock.FencedLock; import com.hazelcast.map.IMap; import com.hazelcast.topic.ITopic; @@ -66,11 +65,6 @@ public class SharedQueueManagementService { private IMap dockerImageCleanupInfo; - /** - * Lock to prevent multiple nodes from processing the same build job. - */ - private FencedLock sharedLock; - private ITopic canceledBuildJobsTopic; public SharedQueueManagementService(BuildJobRepository buildJobRepository, @Qualifier("hazelcastInstance") HazelcastInstance hazelcastInstance, ProfileService profileService) { @@ -86,7 +80,6 @@ public SharedQueueManagementService(BuildJobRepository buildJobRepository, @Qual public void init() { this.buildAgentInformation = this.hazelcastInstance.getMap("buildAgentInformation"); this.processingJobs = this.hazelcastInstance.getMap("processingJobs"); - this.sharedLock = this.hazelcastInstance.getCPSubsystem().getLock("buildJobQueueLock"); this.queue = this.hazelcastInstance.getQueue("buildJobQueue"); this.canceledBuildJobsTopic = hazelcastInstance.getTopic("canceledBuildJobsTopic"); this.dockerImageCleanupInfo = this.hazelcastInstance.getMap("dockerImageCleanupInfo"); @@ -148,28 +141,22 @@ public List getBuildAgentInformationWithoutRecentBuildJob * @param buildJobId id of the build job to cancel */ public void cancelBuildJob(String buildJobId) { - sharedLock.lock(); - try { - // Remove build job if it is queued - if (queue.stream().anyMatch(job -> Objects.equals(job.id(), buildJobId))) { - List toRemove = new ArrayList<>(); - for (BuildJobQueueItem job : queue) { - if (Objects.equals(job.id(), buildJobId)) { - toRemove.add(job); - } - } - queue.removeAll(toRemove); - } - else { - // Cancel build job if it is currently being processed - BuildJobQueueItem buildJob = processingJobs.remove(buildJobId); - if (buildJob != null) { - triggerBuildJobCancellation(buildJobId); + // Remove build job if it is queued + if (queue.stream().anyMatch(job -> Objects.equals(job.id(), buildJobId))) { + List toRemove = new ArrayList<>(); + for (BuildJobQueueItem job : queue) { + if (Objects.equals(job.id(), buildJobId)) { + toRemove.add(job); } } + queue.removeAll(toRemove); } - finally { - sharedLock.unlock(); + else { + // Cancel build job if it is currently being processed + BuildJobQueueItem buildJob = processingJobs.remove(buildJobId); + if (buildJob != null) { + triggerBuildJobCancellation(buildJobId); + } } } @@ -188,30 +175,17 @@ private void triggerBuildJobCancellation(String buildJobId) { * Cancel all queued build jobs. */ public void cancelAllQueuedBuildJobs() { - sharedLock.lock(); - try { - log.debug("Cancelling all queued build jobs"); - queue.clear(); - } - finally { - sharedLock.unlock(); - } + log.debug("Cancelling all queued build jobs"); + queue.clear(); } /** * Cancel all running build jobs. */ public void cancelAllRunningBuildJobs() { - sharedLock.lock(); - try { - for (BuildJobQueueItem buildJob : processingJobs.values()) { - cancelBuildJob(buildJob.id()); - } - } - finally { - sharedLock.unlock(); + for (BuildJobQueueItem buildJob : processingJobs.values()) { + cancelBuildJob(buildJob.id()); } - } /** @@ -220,13 +194,7 @@ public void cancelAllRunningBuildJobs() { * @param agentName name of the agent */ public void cancelAllRunningBuildJobsForAgent(String agentName) { - sharedLock.lock(); - try { - processingJobs.values().stream().filter(job -> Objects.equals(job.buildAgentAddress(), agentName)).forEach(job -> cancelBuildJob(job.id())); - } - finally { - sharedLock.unlock(); - } + processingJobs.values().stream().filter(job -> Objects.equals(job.buildAgentAddress(), agentName)).forEach(job -> cancelBuildJob(job.id())); } /** @@ -235,19 +203,13 @@ public void cancelAllRunningBuildJobsForAgent(String agentName) { * @param courseId id of the course */ public void cancelAllQueuedBuildJobsForCourse(long courseId) { - sharedLock.lock(); - try { - List toRemove = new ArrayList<>(); - for (BuildJobQueueItem job : queue) { - if (job.courseId() == courseId) { - toRemove.add(job); - } + List toRemove = new ArrayList<>(); + for (BuildJobQueueItem job : queue) { + if (job.courseId() == courseId) { + toRemove.add(job); } - queue.removeAll(toRemove); - } - finally { - sharedLock.unlock(); } + queue.removeAll(toRemove); } /** @@ -269,25 +231,19 @@ public void cancelAllRunningBuildJobsForCourse(long courseId) { * @param participationId id of the participation */ public void cancelAllJobsForParticipation(long participationId) { - sharedLock.lock(); - try { - List toRemove = new ArrayList<>(); - for (BuildJobQueueItem queuedJob : queue) { - if (queuedJob.participationId() == participationId) { - toRemove.add(queuedJob); - } + List toRemove = new ArrayList<>(); + for (BuildJobQueueItem queuedJob : queue) { + if (queuedJob.participationId() == participationId) { + toRemove.add(queuedJob); } - queue.removeAll(toRemove); + } + queue.removeAll(toRemove); - for (BuildJobQueueItem runningJob : processingJobs.values()) { - if (runningJob.participationId() == participationId) { - cancelBuildJob(runningJob.id()); - } + for (BuildJobQueueItem runningJob : processingJobs.values()) { + if (runningJob.participationId() == participationId) { + cancelBuildJob(runningJob.id()); } } - finally { - sharedLock.unlock(); - } } /** diff --git a/src/main/java/de/tum/in/www1/artemis/service/connectors/localci/buildagent/SharedQueueProcessingService.java b/src/main/java/de/tum/in/www1/artemis/service/connectors/localci/buildagent/SharedQueueProcessingService.java index 853b89493827..d19d0fae96ba 100644 --- a/src/main/java/de/tum/in/www1/artemis/service/connectors/localci/buildagent/SharedQueueProcessingService.java +++ b/src/main/java/de/tum/in/www1/artemis/service/connectors/localci/buildagent/SharedQueueProcessingService.java @@ -32,7 +32,6 @@ import com.hazelcast.collection.ItemEvent; import com.hazelcast.collection.ItemListener; import com.hazelcast.core.HazelcastInstance; -import com.hazelcast.cp.lock.FencedLock; import com.hazelcast.map.IMap; import de.tum.in.www1.artemis.domain.BuildLogEntry; @@ -65,11 +64,6 @@ public class SharedQueueProcessingService { private final BuildAgentSshKeyService buildAgentSSHKeyService; - /** - * Lock to prevent multiple nodes from processing the same build job. - */ - private FencedLock sharedLock; - private IQueue queue; private IQueue resultQueue; @@ -104,7 +98,6 @@ public SharedQueueProcessingService(@Qualifier("hazelcastInstance") HazelcastIns public void init() { this.buildAgentInformation = this.hazelcastInstance.getMap("buildAgentInformation"); this.processingJobs = this.hazelcastInstance.getMap("processingJobs"); - this.sharedLock = this.hazelcastInstance.getCPSubsystem().getLock("buildJobQueueLock"); this.queue = this.hazelcastInstance.getQueue("buildJobQueue"); this.resultQueue = this.hazelcastInstance.getQueue("buildResultQueue"); this.listenerId = this.queue.addItemListener(new QueuedBuildJobItemListener(), true); @@ -176,14 +169,8 @@ private void checkAvailabilityAndProcessNextBuild() { return; } - // Lock the queue to prevent multiple nodes from processing the same build job - sharedLock.lock(); - try { - buildJob = addToProcessingJobs(); - } - finally { - sharedLock.unlock(); - } + buildJob = addToProcessingJobs(); + processBuild(buildJob); } catch (RejectedExecutionException e) {