From c1809405b3748d03005b8706ebbb4016a71dfaca Mon Sep 17 00:00:00 2001 From: Chris Bono Date: Fri, 28 Apr 2023 14:46:24 -0500 Subject: [PATCH] Backport "Add backoffLimit to KubernetesScheduler (#392)" See spring-cloud/spring-cloud-deployer-kubernetes#407 --- .../KubernetesDeployerProperties.java | 10 ++ .../spi/kubernetes/KubernetesScheduler.java | 43 ++++-- .../spi/kubernetes/KubernetesSchedulerIT.java | 127 ++++++++++++++---- 3 files changed, 146 insertions(+), 34 deletions(-) diff --git a/src/main/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesDeployerProperties.java b/src/main/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesDeployerProperties.java index 835cabbd..7df08892 100755 --- a/src/main/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesDeployerProperties.java +++ b/src/main/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesDeployerProperties.java @@ -1492,6 +1492,8 @@ public static class CronConfig { private Integer ttlSecondsAfterFinished; + private Integer backoffLimit; + public String getConcurrencyPolicy() { return concurrencyPolicy; } @@ -1507,6 +1509,14 @@ public Integer getTtlSecondsAfterFinished() { public void setTtlSecondsAfterFinished(Integer ttlSecondsAfterFinished) { this.ttlSecondsAfterFinished = ttlSecondsAfterFinished; } + + public Integer getBackoffLimit() { + return backoffLimit; + } + + public void setBackoffLimit(Integer backoffLimit) { + this.backoffLimit = backoffLimit; + } } public Boolean getShareProcessNamespace() { diff --git a/src/main/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesScheduler.java b/src/main/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesScheduler.java index 6b927563..fb67646e 100644 --- a/src/main/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesScheduler.java +++ b/src/main/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesScheduler.java @@ -57,6 +57,8 @@ public class KubernetesScheduler extends AbstractKubernetesDeployer implements S static final String KUBERNETES_DEPLOYER_CRON_TTL_SECONDS_AFTER_FINISHED = KubernetesDeployerProperties.KUBERNETES_DEPLOYER_PROPERTIES_PREFIX + ".cron.ttlSecondsAfterFinished"; + static final String KUBERNETES_DEPLOYER_CRON_BACKOFF_LIMIT = KubernetesDeployerProperties.KUBERNETES_DEPLOYER_PROPERTIES_PREFIX + ".cron.backoffLimit"; + public KubernetesScheduler(KubernetesClient client, KubernetesDeployerProperties properties) { Assert.notNull(client, "KubernetesClient must not be null"); @@ -212,7 +214,7 @@ protected CronJob createCronJob(ScheduleRequest scheduleRequest) { if (!StringUtils.hasText(concurrencyPolicy)) { concurrencyPolicy = this.properties.getCron().getConcurrencyPolicy(); } - if(concurrencyPolicy==null) { + if (concurrencyPolicy == null) { concurrencyPolicy = "Allow"; } @@ -225,6 +227,15 @@ protected CronJob createCronJob(ScheduleRequest scheduleRequest) { ttlSecondsAfterFinished = this.properties.getCron().getTtlSecondsAfterFinished(); } + final Integer backoffLimit; + String backoffLimitString = schedulerProperties.get(KUBERNETES_DEPLOYER_CRON_BACKOFF_LIMIT); + if (StringUtils.hasText(backoffLimitString)) { + backoffLimit = Integer.parseInt(backoffLimitString); + } + else { + backoffLimit = this.properties.getCron().getBackoffLimit(); + } + PodSpec podSpec = createPodSpec(new ScheduleRequest(scheduleRequest.getDefinition(),schedulerProperties, scheduleRequest.getCommandlineArguments(), scheduleRequest.getScheduleName(),scheduleRequest.getResource())); String taskServiceAccountName = this.deploymentPropertiesResolver.getTaskServiceAccountName(schedulerProperties); taskServiceAccountName = taskServiceAccountName != null ? taskServiceAccountName : KubernetesDeployerProperties.DEFAULT_TASK_SERVICE_ACCOUNT_NAME; @@ -234,13 +245,29 @@ protected CronJob createCronJob(ScheduleRequest scheduleRequest) { Map annotations = this.deploymentPropertiesResolver.getPodAnnotations(schedulerProperties); labels.putAll(this.deploymentPropertiesResolver.getDeploymentLabels(schedulerProperties)); - CronJob cronJob = new CronJobBuilder().withNewMetadata().withName(scheduleRequest.getScheduleName()) - .withLabels(labels).withAnnotations(this.deploymentPropertiesResolver.getJobAnnotations(schedulerProperties)).endMetadata() - .withNewSpec().withSchedule(schedule).withConcurrencyPolicy(concurrencyPolicy).withNewJobTemplate() - .withNewSpec().withTtlSecondsAfterFinished(ttlSecondsAfterFinished) - .withNewTemplate().withNewMetadata().addToAnnotations(annotations).addToLabels(labels) - .endMetadata().withSpec(podSpec).endTemplate().endSpec() - .endJobTemplate().endSpec().build(); + CronJob cronJob = new CronJobBuilder() + .withNewMetadata() + .withName(scheduleRequest.getScheduleName()) + .withLabels(labels) + .withAnnotations(this.deploymentPropertiesResolver.getJobAnnotations(schedulerProperties)) + .endMetadata() + .withNewSpec() + .withSchedule(schedule) + .withConcurrencyPolicy(concurrencyPolicy) + .withNewJobTemplate() + .withNewSpec() + .withBackoffLimit(backoffLimit) + .withTtlSecondsAfterFinished(ttlSecondsAfterFinished) + .withNewTemplate() + .withNewMetadata() + .addToAnnotations(annotations).addToLabels(labels) + .endMetadata() + .withSpec(podSpec) + .endTemplate() + .endSpec() + .endJobTemplate() + .endSpec() + .build(); setImagePullSecret(scheduleRequest, cronJob); diff --git a/src/test/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesSchedulerIT.java b/src/test/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesSchedulerIT.java index 2af702c7..9ac9832d 100644 --- a/src/test/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesSchedulerIT.java +++ b/src/test/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesSchedulerIT.java @@ -49,6 +49,8 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.autoconfigure.EnableAutoConfiguration; @@ -83,6 +85,8 @@ @ContextConfiguration(classes = {KubernetesSchedulerIT.Config.class}) public class KubernetesSchedulerIT extends AbstractSchedulerIntegrationJUnit5Tests { + private static final Logger LOGGER = LoggerFactory.getLogger(KubernetesSchedulerIT.class); + @Autowired private Scheduler scheduler; @@ -315,7 +319,7 @@ public void testWithExecEntryPoint(boolean isDeprecated) { assertThat(container.getEnv()).as("Environment variables should only have SPRING_CLOUD_APPLICATION_GUID") .hasSize(1); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -348,7 +352,7 @@ public void testWithShellEntryPoint(boolean isDeprecated) { assertThat(container.getEnv()).as("Environment variables should not be null").isNotNull(); assertThat(container.getEnv()).as("Invalid number of environment variables").hasSizeGreaterThan(1); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -389,7 +393,7 @@ public void testWithBootEntryPoint(boolean isDeprecated) throws IOException { assertThat(springApplicationJsonValues).as("SPRING_APPLICATION_JSON should not be null").isNotNull(); assertThat(springApplicationJsonValues).as("Invalid number of SPRING_APPLICATION_JSON entries").hasSize(2); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @Test @@ -468,7 +472,7 @@ public void testEntryPointStyleOverride(boolean isDeprecated) throws Exception { assertThat(springApplicationJsonValues).as("SPRING_APPLICATION_JSON should not be null").isNotNull(); assertThat(springApplicationJsonValues).as("Invalid number of SPRING_APPLICATION_JSON entries").hasSize(2); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -498,7 +502,7 @@ public void testEntryPointStyleDefault(boolean isDeprecated) { .hasSize(1); assertThat(container.getArgs()).as("Command line arguments should not be empty").isNotEmpty(); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -531,7 +535,7 @@ public void testImagePullPolicyOverride(boolean isDeprecated) { assertThat(container.getImagePullPolicy()).as("Unexpected image pull policy").isEqualTo("Always"); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -566,7 +570,7 @@ public void testJobAnnotationsAndLabelsFromSchedulerProperties(boolean isDepreca assertThat(cronJob.getSpec().getJobTemplate().getSpec().getTemplate().getMetadata().getLabels() .get("label2")).as("Pod Label2 is not set").isEqualTo("value2"); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @Test @@ -592,7 +596,7 @@ public void testDefaultLabel() { .size()).as("Should have one label").isEqualTo(1); assertThat(cronJob.getSpec().getJobTemplate().getSpec().getTemplate().getMetadata().getLabels().get( KubernetesScheduler.SPRING_CRONJOB_ID_KEY)).as("Default label is not set").isNotNull(); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -642,7 +646,7 @@ public void testJobAnnotationsAndLabelsFromSchedulerRequest(boolean isDeprecated assertThat(cronJob.getSpec().getJobTemplate().getSpec().getTemplate().getMetadata().getLabels() .get("label2")).as("Pod Label2 is not set").isEqualTo("value2"); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -673,7 +677,7 @@ public void testJobAnnotationsOverride(boolean isDeprecated) { assertThat(cronJob.getMetadata().getAnnotations().get("test1")).as("Job annotation is not set") .isEqualTo("value2"); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -701,7 +705,7 @@ public void testImagePullPolicyDefault(boolean isDeprecated) { assertThat(ImagePullPolicy.relaxedValueOf(container.getImagePullPolicy())).as("Unexpected default image pull policy") .isEqualTo(ImagePullPolicy.IfNotPresent); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -734,7 +738,7 @@ public void testImagePullSecret(boolean isDeprecated) { .getImagePullSecrets(); assertThat(secrets.get(0).getName()).as("Unexpected image pull secret").isEqualTo(secretName); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -762,7 +766,7 @@ public void testImagePullSecretDefault(boolean isDeprecated) { .getImagePullSecrets(); assertThat(secrets).as("There should be no secrets").isEmpty(); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -793,7 +797,7 @@ public void testImagePullSecretFromSchedulerProperties(boolean isDeprecated) { .getImagePullSecrets(); assertThat(secrets.get(0).getName()).as("Unexpected image pull secret").isEqualTo(secretName); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -911,7 +915,7 @@ private void testEnvironmentVariables(KubernetesDeployerProperties kubernetesDep assertThat(container.getEnv()).contains(expectedVars); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -945,7 +949,7 @@ public void testTaskServiceAccountNameOverride(boolean isDeprecated) { .getServiceAccountName(); assertThat(serviceAccountName).as("Unexpected service account name").isEqualTo(taskServiceAccountName); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -976,7 +980,7 @@ public void testTaskServiceAccountNameDefault(boolean isDeprecated) { assertThat(serviceAccountName).as("Unexpected service account name") .isEqualTo(KubernetesSchedulerProperties.DEFAULT_TASK_SERVICE_ACCOUNT_NAME); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -1001,7 +1005,7 @@ public void testConcurrencyPolicy(String concurrencyPolicy) { assertThat(cronJob.getSpec().getConcurrencyPolicy()).isEqualTo(concurrencyPolicy); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @Test @@ -1025,7 +1029,7 @@ public void testConcurrencyPolicyDefault() { assertThat(cronJob.getSpec().getConcurrencyPolicy()).isEqualTo("Allow"); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @Test @@ -1050,7 +1054,7 @@ public void testConcurrencyPolicyFromServerProperties() { assertThat(cronJob.getSpec().getConcurrencyPolicy()).isEqualTo("Forbid"); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -1078,7 +1082,7 @@ public void testTtlSecondsAfterFinished(String ttlSecondsAfterFinished) { assertThat(cronJob.getSpec().getJobTemplate().getSpec().getTtlSecondsAfterFinished()) .isEqualTo(Integer.parseInt(ttlSecondsAfterFinished)); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @ParameterizedTest @@ -1118,7 +1122,7 @@ public void testTtlSecondsAfterFinishedDefault() { assertThat(cronJob.getSpec().getJobTemplate().getSpec().getTtlSecondsAfterFinished()) .isNull(); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @Test @@ -1143,7 +1147,69 @@ public void testTtlSecondsAfterFinishedFromServerProperties() { assertThat(cronJob.getSpec().getJobTemplate().getSpec().getTtlSecondsAfterFinished()).isEqualTo(86400); - kubernetesScheduler.unschedule(cronJob.getMetadata().getName()); + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); + } + + @Test + public void testBackoffLimit() { + KubernetesDeployerProperties kubernetesDeployerProperties = new KubernetesDeployerProperties(); + if (kubernetesDeployerProperties.getNamespace() == null) { + kubernetesDeployerProperties.setNamespace("default"); + } + KubernetesClient kubernetesClient = new DefaultKubernetesClient() + .inNamespace(kubernetesDeployerProperties.getNamespace()); + KubernetesScheduler kubernetesScheduler = new KubernetesScheduler(kubernetesClient, kubernetesDeployerProperties); + + AppDefinition appDefinition = new AppDefinition(randomName(), getAppProperties()); + + Map schedulerProperties = new HashMap<>(getSchedulerProperties()); + schedulerProperties.put(KubernetesScheduler.KUBERNETES_DEPLOYER_CRON_BACKOFF_LIMIT, "5"); + + ScheduleRequest scheduleRequest = new ScheduleRequest(appDefinition, schedulerProperties, + getCommandLineArgs(), randomName(), testApplication()); + CronJob cronJob = kubernetesScheduler.createCronJob(scheduleRequest); + assertThat(cronJob.getSpec().getJobTemplate().getSpec().getBackoffLimit()).isEqualTo(5); + + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); + } + + @Test + public void testBackoffLimitDefault() { + KubernetesDeployerProperties kubernetesDeployerProperties = new KubernetesDeployerProperties(); + if (kubernetesDeployerProperties.getNamespace() == null) { + kubernetesDeployerProperties.setNamespace("default"); + } + KubernetesClient kubernetesClient = new DefaultKubernetesClient() + .inNamespace(kubernetesDeployerProperties.getNamespace()); + KubernetesScheduler kubernetesScheduler = new KubernetesScheduler(kubernetesClient, kubernetesDeployerProperties); + + AppDefinition appDefinition = new AppDefinition(randomName(), getAppProperties()); + ScheduleRequest scheduleRequest = new ScheduleRequest(appDefinition, getSchedulerProperties(), + getCommandLineArgs(), randomName(), testApplication()); + CronJob cronJob = kubernetesScheduler.createCronJob(scheduleRequest); + assertThat(cronJob.getSpec().getJobTemplate().getSpec().getBackoffLimit()).isNull(); + + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); + } + + @Test + public void testBackoffLimitFromServerProperties() { + KubernetesDeployerProperties kubernetesDeployerProperties = new KubernetesDeployerProperties(); + if (kubernetesDeployerProperties.getNamespace() == null) { + kubernetesDeployerProperties.setNamespace("default"); + } + kubernetesDeployerProperties.getCron().setBackoffLimit(7); + KubernetesClient kubernetesClient = new DefaultKubernetesClient() + .inNamespace(kubernetesDeployerProperties.getNamespace()); + KubernetesScheduler kubernetesScheduler = new KubernetesScheduler(kubernetesClient, kubernetesDeployerProperties); + + AppDefinition appDefinition = new AppDefinition(randomName(), getAppProperties()); + ScheduleRequest scheduleRequest = new ScheduleRequest(appDefinition, getSchedulerProperties(), + getCommandLineArgs(), randomName(), testApplication()); + CronJob cronJob = kubernetesScheduler.createCronJob(scheduleRequest); + assertThat(cronJob.getSpec().getJobTemplate().getSpec().getBackoffLimit()).isEqualTo(7); + + safeUnschedule(kubernetesScheduler, cronJob.getMetadata().getName()); } @AfterAll @@ -1160,11 +1226,20 @@ public static void cleanup() { List scheduleInfos = kubernetesScheduler.list(); for (ScheduleInfo scheduleInfo : scheduleInfos) { - kubernetesScheduler.unschedule(scheduleInfo.getScheduleName()); + safeUnschedule(kubernetesScheduler, scheduleInfo.getScheduleName()); } // Cleanup the schedules that aren't part of the list() - created from listScheduleWithExternalCronJobs test - kubernetesScheduler.unschedule("job2"); - kubernetesScheduler.unschedule("job3"); + safeUnschedule(kubernetesScheduler, "job2"); + safeUnschedule(kubernetesScheduler, "job3"); + } + + private static void safeUnschedule(KubernetesScheduler scheduler, String scheduleName) { + try { + scheduler.unschedule(scheduleName); + } + catch (Exception ex) { + LOGGER.warn("Failed to unschedule '" + scheduleName + "'", ex); + } } @Configuration