From a5f5b5df8c0b17c2e11def35c0827babc1904af8 Mon Sep 17 00:00:00 2001 From: Anil Kedia Date: Wed, 17 Feb 2021 22:39:32 +0000 Subject: [PATCH 1/4] Fix to check job condition to determine if job has failed in addition to checking status. --- .../kubernetes/operator/JobWatcher.java | 38 +++++++++++++++---- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/operator/src/main/java/oracle/kubernetes/operator/JobWatcher.java b/operator/src/main/java/oracle/kubernetes/operator/JobWatcher.java index f3c32342f87..31626ec91ab 100644 --- a/operator/src/main/java/oracle/kubernetes/operator/JobWatcher.java +++ b/operator/src/main/java/oracle/kubernetes/operator/JobWatcher.java @@ -3,6 +3,7 @@ package oracle.kubernetes.operator; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; @@ -10,6 +11,7 @@ import java.util.concurrent.ThreadFactory; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Consumer; +import javax.annotation.Nonnull; import io.kubernetes.client.openapi.ApiException; import io.kubernetes.client.openapi.models.V1Job; @@ -137,16 +139,38 @@ static boolean isFailed(V1Job job) { return false; } - V1JobStatus status = job.getStatus(); - if (status != null) { - if (status.getFailed() != null && status.getFailed() > 0) { - LOGGER.severe(MessageKeys.JOB_IS_FAILED, job.getMetadata().getName()); - return true; - } + if (isStatusFailed(job) || isConditionFailed(job)) { + LOGGER.severe(MessageKeys.JOB_IS_FAILED, job.getMetadata().getName()); + return true; } return false; } + private static boolean isStatusFailed(V1Job job) { + return Optional.ofNullable(job.getStatus()).map(V1JobStatus::getFailed).map(failed -> (failed > 0)).orElse(false); + } + + private static boolean isConditionFailed(V1Job job) { + return getJobConditions(job).stream().anyMatch(JobWatcher::isJobConditionFailed); + } + + private static List getJobConditions(@Nonnull V1Job job) { + return Optional.ofNullable(job.getStatus()).map(V1JobStatus::getConditions).orElse(Collections.emptyList()); + } + + private static boolean isJobConditionFailed(V1JobCondition jobCondition) { + return getType(jobCondition).equals("Failed") && getStatus(jobCondition).equals("True"); + } + + private static String getType(V1JobCondition jobCondition) { + return Optional.ofNullable(jobCondition).map(V1JobCondition::getType).orElse(""); + } + + private static String getStatus(V1JobCondition jobCondition) { + return Optional.ofNullable(jobCondition).map(V1JobCondition::getStatus).orElse(""); + } + + static String getFailedReason(V1Job job) { V1JobStatus status = job.getStatus(); if (status != null && status.getConditions() != null) { @@ -305,4 +329,4 @@ private long getJobStartedSeconds() { return -1; } } -} +} \ No newline at end of file From 4e59b9c9c3fa8714f5fc38feffc03d2e53b5ac3b Mon Sep 17 00:00:00 2001 From: Anil Kedia Date: Wed, 17 Feb 2021 23:52:48 +0000 Subject: [PATCH 2/4] Added unit tests. --- .../kubernetes/operator/JobWatcherTest.java | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/operator/src/test/java/oracle/kubernetes/operator/JobWatcherTest.java b/operator/src/test/java/oracle/kubernetes/operator/JobWatcherTest.java index 3a197d4f23b..031498559a9 100644 --- a/operator/src/test/java/oracle/kubernetes/operator/JobWatcherTest.java +++ b/operator/src/test/java/oracle/kubernetes/operator/JobWatcherTest.java @@ -4,6 +4,8 @@ package oracle.kubernetes.operator; import java.math.BigInteger; +import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Function; @@ -128,6 +130,28 @@ public void whenJobConditionStatusFalse_reportNotComplete() { assertThat(JobWatcher.isComplete(cachedJob), is(false)); } + @Test + public void whenJobConditionTypeFailedWithTrueStatus_reportFailed() { + cachedJob.status(new V1JobStatus().addConditionsItem(new V1JobCondition().type("Failed").status("True"))); + + assertThat(JobWatcher.isFailed(cachedJob), is(true)); + } + + @Test + public void whenJobConditionTypeFailedWithNoStatus_reportNotFailed() { + cachedJob.status(new V1JobStatus().addConditionsItem(new V1JobCondition().type("Failed").status(""))); + + assertThat(JobWatcher.isFailed(cachedJob), is(false)); + } + + @Test + public void whenJobHasStatusWithNoConditionsAndNotFailed_reportNotFailed() { + cachedJob.status(new V1JobStatus().conditions(Collections.emptyList())); + + assertThat(JobWatcher.isFailed(cachedJob), is(false)); + } + + @Test public void whenJobRunningAndReadyConditionIsTrue_reportComplete() { markJobCompleted(cachedJob); @@ -151,6 +175,10 @@ private V1Job markJobFailed(V1Job job) { return setFailedWithReason(job, null); } + private V1Job markJobConditionFailed(V1Job job) { + return setFailedConditionWithReason(job, null); + } + private V1Job markJobTimedOut(V1Job job) { return markJobTimedOut(job, "DeadlineExceeded"); } @@ -163,11 +191,22 @@ private V1Job setFailedWithReason(V1Job job, String reason) { return job.status(new V1JobStatus().failed(1).addConditionsItem(createCondition("Failed").reason(reason))); } + private V1Job setFailedConditionWithReason(V1Job job, String reason) { + return job.status(new V1JobStatus().conditions( + new ArrayList<>(Arrays.asList(new V1JobCondition().type("Failed").status("True").reason(reason))))); + } + @Test public void whenJobHasNoStatus_reportNotFailed() { assertThat(JobWatcher.isFailed(cachedJob), is(false)); } + @Test + public void whenJobHasNoStatusAndFailedCondition_reportFailed() { + markJobFailed(cachedJob); + assertThat(JobWatcher.isFailed(cachedJob), is(true)); + } + @Test public void whenJobHasFailedCount_reportFailed() { cachedJob.status(new V1JobStatus().failed(1)); @@ -248,6 +287,13 @@ public void whenWaitForReadyAppliedToFailedJob_performNextStep() { assertThat(terminalStep.wasRun(), is(true)); } + @Test + public void whenWaitForReadyAppliedToJobWithFailedCondition_performNextStep() { + startWaitForReady(this::markJobConditionFailed); + + assertThat(terminalStep.wasRun(), is(true)); + } + // Starts the waitForReady step with job modified as needed private void startWaitForReady(Function jobFunction) { AtomicBoolean stopping = new AtomicBoolean(false); From 4b5fad8306d19b93c35111b9028121d6c650a569 Mon Sep 17 00:00:00 2001 From: Anil Kedia Date: Wed, 17 Feb 2021 23:59:08 +0000 Subject: [PATCH 3/4] Minor changes. --- .../test/java/oracle/kubernetes/operator/JobWatcherTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/operator/src/test/java/oracle/kubernetes/operator/JobWatcherTest.java b/operator/src/test/java/oracle/kubernetes/operator/JobWatcherTest.java index 031498559a9..615a18f11f4 100644 --- a/operator/src/test/java/oracle/kubernetes/operator/JobWatcherTest.java +++ b/operator/src/test/java/oracle/kubernetes/operator/JobWatcherTest.java @@ -132,7 +132,7 @@ public void whenJobConditionStatusFalse_reportNotComplete() { @Test public void whenJobConditionTypeFailedWithTrueStatus_reportFailed() { - cachedJob.status(new V1JobStatus().addConditionsItem(new V1JobCondition().type("Failed").status("True"))); + markJobConditionFailed(cachedJob); assertThat(JobWatcher.isFailed(cachedJob), is(true)); } @@ -203,7 +203,7 @@ public void whenJobHasNoStatus_reportNotFailed() { @Test public void whenJobHasNoStatusAndFailedCondition_reportFailed() { - markJobFailed(cachedJob); + markJobConditionFailed(cachedJob); assertThat(JobWatcher.isFailed(cachedJob), is(true)); } From 1633df06d0f63efd7a8b3f83d261d63472b1a7d4 Mon Sep 17 00:00:00 2001 From: Anil Kedia Date: Thu, 18 Feb 2021 00:08:57 +0000 Subject: [PATCH 4/4] Remove duplicate test. --- .../java/oracle/kubernetes/operator/JobWatcherTest.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/operator/src/test/java/oracle/kubernetes/operator/JobWatcherTest.java b/operator/src/test/java/oracle/kubernetes/operator/JobWatcherTest.java index 615a18f11f4..90c18c17aac 100644 --- a/operator/src/test/java/oracle/kubernetes/operator/JobWatcherTest.java +++ b/operator/src/test/java/oracle/kubernetes/operator/JobWatcherTest.java @@ -201,12 +201,6 @@ public void whenJobHasNoStatus_reportNotFailed() { assertThat(JobWatcher.isFailed(cachedJob), is(false)); } - @Test - public void whenJobHasNoStatusAndFailedCondition_reportFailed() { - markJobConditionFailed(cachedJob); - assertThat(JobWatcher.isFailed(cachedJob), is(true)); - } - @Test public void whenJobHasFailedCount_reportFailed() { cachedJob.status(new V1JobStatus().failed(1));