diff --git a/genie-docs/src/docs/asciidoc/_metrics.adoc b/genie-docs/src/docs/asciidoc/_metrics.adoc index 4154b48daab..7ec84312885 100644 --- a/genie-docs/src/docs/asciidoc/_metrics.adoc +++ b/genie-docs/src/docs/asciidoc/_metrics.adoc @@ -197,6 +197,31 @@ It will have to be added if one is desired otherwise metrics are just published |ArchiveStatusCleanupTask |status, exceptionClass +|genie.jobs.attachments.s3.count.distribution +|Distribution summary of the number of files attached +|count +|S3AttachmentServiceImpl +| + +|genie.jobs.attachments.s3.largestSize.distribution +|Distribution summary of the size of the largest file attached +|bytes +|S3AttachmentServiceImpl +| + +|genie.jobs.attachments.s3.totalSize.distribution +|Distribution summary of the total size of the files attached +|bytes +|S3AttachmentServiceImpl +| + +|genie.jobs.attachments.s3.upload.timer +|genie.jobs.attachments.s3.upload.timer +|Time taken to upload job attachments to S3 (only measured for jobs with attachments) +|nanoseconds +|S3AttachmentServiceImpl +|status, exceptionClass + |genie.jobs.clusters.selectors.script.select.timer |Time taken by the loaded script to select a cluster among the one passed as input |nanoseconds diff --git a/genie-docs/src/docs/asciidoc/_properties.adoc b/genie-docs/src/docs/asciidoc/_properties.adoc index f2c11b643c0..511637766ff 100644 --- a/genie-docs/src/docs/asciidoc/_properties.adoc +++ b/genie-docs/src/docs/asciidoc/_properties.adoc @@ -230,6 +230,21 @@ Health of the system is marked unhealthy if the CPU load of a system goes beyond |null |yes +|genie.jobs.attachments.location-prefix +|Common prefix where attachments are stored +|s3://genie/attachments +|no + +|genie.jobs.attachments.max-size +|Maximum size of an attachment +|100MB +|no + +|genie.jobs.attachments.max-total-size +|Maximum size of all attachments combined (Spring and Tomcat may also independently limit the size of upload) +|150MB +|no + |genie.jobs.cleanup.deleteDependencies |Whether or not to delete the dependencies directories for applications, cluster, command to save disk space after job completion |true @@ -293,7 +308,7 @@ cases is specified as part of the `Command` entity for a particular job. |genie.jobs.locations.attachments |The default root location where job attachments will be temporarily stored. Scheme should be included. Created if -doesn't exist. +doesn't exist (deprecated, see genie.jobs.attachments.* properties) |file://${java.io.tmpdir}genie/attachments/ |no diff --git a/genie-web/src/integTest/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsIntegrationTest.java b/genie-web/src/integTest/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsIntegrationTest.java index e9f1df23f94..5a090189030 100644 --- a/genie-web/src/integTest/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsIntegrationTest.java +++ b/genie-web/src/integTest/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsIntegrationTest.java @@ -64,7 +64,6 @@ import org.assertj.core.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; -import org.mockito.Mockito; import org.springframework.core.io.FileSystemResource; import org.springframework.core.io.Resource; import org.springframework.data.domain.Page; @@ -373,13 +372,9 @@ void canSaveAndVerifyJobSubmissionWithAttachments(@TempDir final Path tempDir) t totalAttachmentSize ); - Mockito - .when(this.legacyAttachmentService.saveAttachments(Mockito.anyString(), Mockito.anySet())) - .thenReturn(attachmentURIs); - // Save the job submission final String id = this.service.saveJobSubmission( - new JobSubmission.Builder(jobRequest, jobRequestMetadata).withAttachments(attachments).build() + new JobSubmission.Builder(jobRequest, jobRequestMetadata).withAttachments(attachmentURIs).build() ); // Going to assume that most other verification of parameters other than attachments is done in diff --git a/genie-web/src/main/java/com/netflix/genie/web/agent/services/impl/AgentJobServiceImpl.java b/genie-web/src/main/java/com/netflix/genie/web/agent/services/impl/AgentJobServiceImpl.java index d9d345d3061..0999607d9ba 100644 --- a/genie-web/src/main/java/com/netflix/genie/web/agent/services/impl/AgentJobServiceImpl.java +++ b/genie-web/src/main/java/com/netflix/genie/web/agent/services/impl/AgentJobServiceImpl.java @@ -29,7 +29,6 @@ import com.netflix.genie.common.internal.exceptions.unchecked.GenieIdAlreadyExistsException; import com.netflix.genie.common.internal.exceptions.unchecked.GenieJobNotFoundException; import com.netflix.genie.common.internal.exceptions.unchecked.GenieJobSpecificationNotFoundException; -import com.netflix.genie.common.internal.exceptions.unchecked.GenieRuntimeException; import com.netflix.genie.web.agent.inspectors.InspectionReport; import com.netflix.genie.web.agent.services.AgentConfigurationService; import com.netflix.genie.web.agent.services.AgentFilterService; @@ -40,7 +39,6 @@ import com.netflix.genie.web.dtos.ResolvedJob; import com.netflix.genie.web.exceptions.checked.IdAlreadyExistsException; import com.netflix.genie.web.exceptions.checked.NotFoundException; -import com.netflix.genie.web.exceptions.checked.SaveAttachmentException; import com.netflix.genie.web.services.JobResolverService; import com.netflix.genie.web.util.MetricsUtils; import io.micrometer.core.instrument.MeterRegistry; @@ -169,9 +167,6 @@ public String reserveJobId( } catch (final IdAlreadyExistsException e) { // TODO: How to handle this? throw new GenieIdAlreadyExistsException(e); - } catch (final SaveAttachmentException e) { - // this really shouldn't happen as there are no attachments with an agent cli job - throw new GenieRuntimeException(e); } } diff --git a/genie-web/src/main/java/com/netflix/genie/web/apis/rest/v3/controllers/GenieExceptionMapper.java b/genie-web/src/main/java/com/netflix/genie/web/apis/rest/v3/controllers/GenieExceptionMapper.java index 3f4841636a9..f3a84c89c0c 100644 --- a/genie-web/src/main/java/com/netflix/genie/web/apis/rest/v3/controllers/GenieExceptionMapper.java +++ b/genie-web/src/main/java/com/netflix/genie/web/apis/rest/v3/controllers/GenieExceptionMapper.java @@ -30,6 +30,7 @@ import com.netflix.genie.common.internal.exceptions.unchecked.GenieJobNotFoundException; import com.netflix.genie.common.internal.exceptions.unchecked.GenieJobSpecificationNotFoundException; import com.netflix.genie.common.internal.exceptions.unchecked.GenieRuntimeException; +import com.netflix.genie.web.exceptions.checked.AttachmentTooLargeException; import com.netflix.genie.web.exceptions.checked.IdAlreadyExistsException; import com.netflix.genie.web.exceptions.checked.JobNotFoundException; import com.netflix.genie.web.exceptions.checked.NotFoundException; @@ -135,6 +136,8 @@ public ResponseEntity handleGenieCheckedException(final G return new ResponseEntity<>(e, HttpStatus.NOT_FOUND); } else if (e instanceof PreconditionFailedException) { return new ResponseEntity<>(e, HttpStatus.BAD_REQUEST); + } else if (e instanceof AttachmentTooLargeException) { + return new ResponseEntity<>(e, HttpStatus.PAYLOAD_TOO_LARGE); } else { return new ResponseEntity<>(e, HttpStatus.INTERNAL_SERVER_ERROR); } diff --git a/genie-web/src/main/java/com/netflix/genie/web/apis/rest/v3/controllers/JobRestController.java b/genie-web/src/main/java/com/netflix/genie/web/apis/rest/v3/controllers/JobRestController.java index e2bd0511ee3..ba49706ba9b 100644 --- a/genie-web/src/main/java/com/netflix/genie/web/apis/rest/v3/controllers/JobRestController.java +++ b/genie-web/src/main/java/com/netflix/genie/web/apis/rest/v3/controllers/JobRestController.java @@ -57,10 +57,11 @@ import com.netflix.genie.web.dtos.JobSubmission; import com.netflix.genie.web.exceptions.checked.NotFoundException; import com.netflix.genie.web.properties.JobsProperties; -import com.netflix.genie.web.services.LegacyAttachmentService; +import com.netflix.genie.web.services.AttachmentService; import com.netflix.genie.web.services.JobCoordinatorService; import com.netflix.genie.web.services.JobDirectoryServerService; import com.netflix.genie.web.services.JobLaunchService; +import com.netflix.genie.web.services.LegacyAttachmentService; import com.netflix.genie.web.util.JobExecutionModeSelector; import io.micrometer.core.instrument.Counter; import io.micrometer.core.instrument.MeterRegistry; @@ -157,6 +158,7 @@ public class JobRestController { private final AgentRoutingService agentRoutingService; private final PersistenceService persistenceService; private final Environment environment; + private final AttachmentService attachmentService; // TODO: V3 Execution only private final LegacyAttachmentService legacyAttachmentService; @@ -168,7 +170,6 @@ public class JobRestController { /** * Constructor. - * * @param jobLaunchService The {@link JobLaunchService} implementation to use * @param dataServices The {@link DataServices} instance to use * @param jobCoordinatorService The job coordinator service to use. @@ -180,6 +181,7 @@ public class JobRestController { * @param registry The metrics registry to use * @param agentRoutingService Agent routing service * @param environment The application environment to pull dynamic properties from + * @param attachmentService The attachment service to use to save attachments. * @param legacyAttachmentService The attachment service to use to save attachments. * @param jobExecutionModeSelector The execution mode (agent vs. embedded) mode selector */ @@ -197,6 +199,7 @@ public JobRestController( final MeterRegistry registry, final AgentRoutingService agentRoutingService, final Environment environment, + final AttachmentService attachmentService, final LegacyAttachmentService legacyAttachmentService, final JobExecutionModeSelector jobExecutionModeSelector ) { @@ -217,6 +220,7 @@ public JobRestController( this.agentRoutingService = agentRoutingService; this.persistenceService = dataServices.getPersistenceService(); this.environment = environment; + this.attachmentService = attachmentService; // TODO: V3 Only. Remove. this.legacyAttachmentService = legacyAttachmentService; @@ -946,10 +950,13 @@ private String agentExecution( if (attachments != null) { jobSubmissionBuilder.withAttachments( - Arrays - .stream(attachments) - .map(MultipartFile::getResource) - .collect(Collectors.toSet()) + this.attachmentService.saveAttachments( + jobRequest.getId().orElse(null), + Arrays + .stream(attachments) + .map(MultipartFile::getResource) + .collect(Collectors.toSet()) + ) ); } diff --git a/genie-web/src/main/java/com/netflix/genie/web/data/services/PersistenceService.java b/genie-web/src/main/java/com/netflix/genie/web/data/services/PersistenceService.java index 57bd3d4f04b..1a66f864b6b 100644 --- a/genie-web/src/main/java/com/netflix/genie/web/data/services/PersistenceService.java +++ b/genie-web/src/main/java/com/netflix/genie/web/data/services/PersistenceService.java @@ -48,7 +48,6 @@ import com.netflix.genie.web.exceptions.checked.IdAlreadyExistsException; import com.netflix.genie.web.exceptions.checked.NotFoundException; import com.netflix.genie.web.exceptions.checked.PreconditionFailedException; -import com.netflix.genie.web.exceptions.checked.SaveAttachmentException; import com.netflix.genie.web.services.LegacyAttachmentService; import org.springframework.data.domain.Page; import org.springframework.data.domain.Pageable; @@ -726,14 +725,9 @@ long deleteJobsCreatedBefore( * either via the API or via the agent CLI * @return The unique id of the job within the Genie ecosystem * @throws IdAlreadyExistsException If the id the user requested already exists in the system for another job - * @throws SaveAttachmentException If attachments were sent in with the job submission and they were unable to be - * persisted to an underlying storage mechanism meant to share the data with an - * agent process */ @Nonnull - String saveJobSubmission(@Valid JobSubmission jobSubmission) throws - IdAlreadyExistsException, - SaveAttachmentException; + String saveJobSubmission(@Valid JobSubmission jobSubmission) throws IdAlreadyExistsException; /** * Get the original request for a job. diff --git a/genie-web/src/main/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImpl.java b/genie-web/src/main/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImpl.java index 546b9a5d896..4c3e4931ca1 100644 --- a/genie-web/src/main/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImpl.java +++ b/genie-web/src/main/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImpl.java @@ -105,8 +105,6 @@ import com.netflix.genie.web.exceptions.checked.IdAlreadyExistsException; import com.netflix.genie.web.exceptions.checked.NotFoundException; import com.netflix.genie.web.exceptions.checked.PreconditionFailedException; -import com.netflix.genie.web.exceptions.checked.SaveAttachmentException; -import com.netflix.genie.web.services.LegacyAttachmentService; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.StringUtils; @@ -208,21 +206,14 @@ public class JpaPersistenceServiceImpl implements PersistenceService { private final JpaJobRepository jobRepository; private final JpaTagRepository tagRepository; - // TODO: Maybe this should be moved to a higher place, job resolver? Not sure persistence tier is proper place for - // saving attachments? - private final LegacyAttachmentService legacyAttachmentService; - /** * Constructor. - * - * @param entityManager The {@link EntityManager} to use + * @param entityManager The {@link EntityManager} to use * @param jpaRepositories All the repositories in the Genie application - * @param legacyAttachmentService The {@link LegacyAttachmentService} implementation to use */ public JpaPersistenceServiceImpl( final EntityManager entityManager, - final JpaRepositories jpaRepositories, - final LegacyAttachmentService legacyAttachmentService + final JpaRepositories jpaRepositories ) { this.entityManager = entityManager; this.agentConnectionRepository = jpaRepositories.getAgentConnectionRepository(); @@ -233,7 +224,6 @@ public JpaPersistenceServiceImpl( this.fileRepository = jpaRepositories.getFileRepository(); this.jobRepository = jpaRepositories.getJobRepository(); this.tagRepository = jpaRepositories.getTagRepository(); - this.legacyAttachmentService = legacyAttachmentService; } //region Application APIs @@ -1686,7 +1676,7 @@ public long deleteJobsCreatedBefore( @Nonnull public String saveJobSubmission( @Valid final JobSubmission jobSubmission - ) throws IdAlreadyExistsException, SaveAttachmentException { + ) throws IdAlreadyExistsException { log.debug("[saveJobSubmission] Attempting to save job submission {}", jobSubmission); // TODO: Metrics final JobEntity jobEntity = new JobEntity(); @@ -1698,12 +1688,6 @@ public String saveJobSubmission( // Create the unique id if one doesn't already exist this.setUniqueId(jobEntity, jobRequest.getRequestedId().orElse(null)); - // Do we have attachments? Save them so the agent can access them later. - final Set attachmentURIs = this.legacyAttachmentService.saveAttachments( - jobEntity.getUniqueId(), - jobSubmission.getAttachments() - ); - jobEntity.setCommandArgs(jobRequest.getCommandArgs()); this.setJobMetadataFields( @@ -1711,7 +1695,7 @@ public String saveJobSubmission( jobRequest.getMetadata(), jobRequest.getResources().getSetupFile().orElse(null) ); - this.setJobExecutionEnvironmentFields(jobEntity, jobRequest.getResources(), attachmentURIs); + this.setJobExecutionEnvironmentFields(jobEntity, jobRequest.getResources(), jobSubmission.getAttachments()); this.setExecutionResourceCriteriaFields(jobEntity, jobRequest.getCriteria()); this.setRequestedJobEnvironmentFields(jobEntity, jobRequest.getRequestedJobEnvironment()); this.setRequestedAgentConfigFields(jobEntity, jobRequest.getRequestedAgentConfig()); diff --git a/genie-web/src/main/java/com/netflix/genie/web/dtos/JobSubmission.java b/genie-web/src/main/java/com/netflix/genie/web/dtos/JobSubmission.java index 837c905dfbb..3665ed05aeb 100644 --- a/genie-web/src/main/java/com/netflix/genie/web/dtos/JobSubmission.java +++ b/genie-web/src/main/java/com/netflix/genie/web/dtos/JobSubmission.java @@ -24,11 +24,11 @@ import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.ToString; -import org.springframework.core.io.Resource; import javax.annotation.Nullable; import javax.validation.Valid; import javax.validation.constraints.NotNull; +import java.net.URI; import java.util.Arrays; import java.util.Collection; import java.util.Set; @@ -40,22 +40,8 @@ * @since 4.0.0 */ @Getter -@EqualsAndHashCode( - doNotUseGetters = true, - of = { - // Exclude the attachments to save on scanning bytes - "jobRequest", - "jobRequestMetadata" - } -) -@ToString( - doNotUseGetters = true, - of = { - // Exclude the attachments to save on scanning bytes - "jobRequest", - "jobRequestMetadata" - } -) +@EqualsAndHashCode(doNotUseGetters = true) +@ToString(doNotUseGetters = true) @SuppressWarnings("FinalClass") public class JobSubmission { @@ -66,7 +52,7 @@ public class JobSubmission { @Valid private final JobRequestMetadata jobRequestMetadata; @NotNull - private final Set attachments; + private final Set attachments; private JobSubmission(final Builder builder) { this.jobRequest = builder.bJobRequest; @@ -83,7 +69,7 @@ private JobSubmission(final Builder builder) { public static class Builder { private final JobRequest bJobRequest; private final JobRequestMetadata bJobRequestMetadata; - private final Set bAttachments; + private final Set bAttachments; /** * Constructor with required parameters. @@ -100,10 +86,10 @@ public Builder(final JobRequest jobRequest, final JobRequestMetadata jobRequestM /** * Set the attachments associated with this submission if there were any. * - * @param attachments The attachments as {@link Resource} instances + * @param attachments The attachments {@link URI}s * @return the builder */ - public Builder withAttachments(@Nullable final Set attachments) { + public Builder withAttachments(@Nullable final Set attachments) { this.setAttachments(attachments); return this; } @@ -111,10 +97,10 @@ public Builder withAttachments(@Nullable final Set attachments) { /** * Set the attachments associated with this submission. * - * @param attachments The attachments as {@link Resource} instances + * @param attachments The attachments as {@link URI}s * @return the builder */ - public Builder withAttachments(final Resource... attachments) { + public Builder withAttachments(final URI... attachments) { this.setAttachments(Arrays.asList(attachments)); return this; } @@ -128,7 +114,7 @@ public JobSubmission build() { return new JobSubmission(this); } - private void setAttachments(@Nullable final Collection attachments) { + private void setAttachments(@Nullable final Collection attachments) { this.bAttachments.clear(); if (attachments != null) { this.bAttachments.addAll(attachments); diff --git a/genie-web/src/main/java/com/netflix/genie/web/exceptions/checked/AttachmentTooLargeException.java b/genie-web/src/main/java/com/netflix/genie/web/exceptions/checked/AttachmentTooLargeException.java new file mode 100644 index 00000000000..3b26be24031 --- /dev/null +++ b/genie-web/src/main/java/com/netflix/genie/web/exceptions/checked/AttachmentTooLargeException.java @@ -0,0 +1,61 @@ +/* + * + * Copyright 2020 Netflix, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +package com.netflix.genie.web.exceptions.checked; + +/** + * Exception thrown when the user tries to submit a job whose attachments exceed the limits. + * + * @author mprimi + * @since 4.0.0 + */ +public class AttachmentTooLargeException extends SaveAttachmentException { + /** + * Constructor. + */ + public AttachmentTooLargeException() { + super(); + } + + /** + * Constructor. + * + * @param message The detail message + */ + public AttachmentTooLargeException(final String message) { + super(message); + } + + /** + * Constructor. + * + * @param message The detail message + * @param cause The root cause of this exception + */ + public AttachmentTooLargeException(final String message, final Throwable cause) { + super(message, cause); + } + + /** + * Constructor. + * + * @param cause The root cause of this exception + */ + public AttachmentTooLargeException(final Throwable cause) { + super(cause); + } +} diff --git a/genie-web/src/main/java/com/netflix/genie/web/properties/AttachmentServiceProperties.java b/genie-web/src/main/java/com/netflix/genie/web/properties/AttachmentServiceProperties.java new file mode 100644 index 00000000000..b3747707fd2 --- /dev/null +++ b/genie-web/src/main/java/com/netflix/genie/web/properties/AttachmentServiceProperties.java @@ -0,0 +1,59 @@ +/* + * + * Copyright 2020 Netflix, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +package com.netflix.genie.web.properties; + +import lombok.Getter; +import lombok.Setter; +import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.util.unit.DataSize; +import org.springframework.validation.annotation.Validated; + +import javax.validation.constraints.NotNull; +import java.net.URI; +import java.nio.file.Path; +import java.nio.file.Paths; + +/** + * Properties for the {@link com.netflix.genie.web.services.AttachmentService}. + * + * @author mprimi + * @since 4.0.0 + */ +@ConfigurationProperties(prefix = AttachmentServiceProperties.PROPERTY_PREFIX) +@Getter +@Setter +@Validated +public class AttachmentServiceProperties { + + /** + * The property prefix for job user limiting. + */ + public static final String PROPERTY_PREFIX = "genie.jobs.attachments"; + + private static final Path SYSTEM_TMP_DIR = Paths.get(System.getProperty("java.io.tmpdir", "/tmp/")); + + @NotNull(message = "Attachment location prefix is required") + private URI locationPrefix = URI.create("file://" + SYSTEM_TMP_DIR.resolve("genie/attachments")); + + @NotNull(message = "Maximum attachment size is required") + private DataSize maxSize = DataSize.ofMegabytes(100); + + @NotNull(message = "Maximum attachments total size is required") + private DataSize maxTotalSize = DataSize.ofMegabytes(150); + +} diff --git a/genie-web/src/main/java/com/netflix/genie/web/properties/JobsLocationsProperties.java b/genie-web/src/main/java/com/netflix/genie/web/properties/JobsLocationsProperties.java index 52f4c3d146c..1ff4c80567c 100644 --- a/genie-web/src/main/java/com/netflix/genie/web/properties/JobsLocationsProperties.java +++ b/genie-web/src/main/java/com/netflix/genie/web/properties/JobsLocationsProperties.java @@ -50,6 +50,7 @@ public class JobsLocationsProperties { @NotNull(message = "Attachment storage location is required") private URI attachments = URI.create("file://" + SYSTEM_TMP_DIR + "genie/attachments/"); + @Deprecated @NotNull(message = "Default job working directory is required") private URI jobs = URI.create("file://" + SYSTEM_TMP_DIR + "genie/jobs/"); } diff --git a/genie-web/src/main/java/com/netflix/genie/web/services/impl/LocalFileSystemAttachmentServiceImpl.java b/genie-web/src/main/java/com/netflix/genie/web/services/impl/LocalFileSystemAttachmentServiceImpl.java new file mode 100644 index 00000000000..19301375871 --- /dev/null +++ b/genie-web/src/main/java/com/netflix/genie/web/services/impl/LocalFileSystemAttachmentServiceImpl.java @@ -0,0 +1,121 @@ +/* + * + * Copyright 2020 Netflix, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +package com.netflix.genie.web.services.impl; + +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; +import com.netflix.genie.web.exceptions.checked.AttachmentTooLargeException; +import com.netflix.genie.web.exceptions.checked.SaveAttachmentException; +import com.netflix.genie.web.properties.AttachmentServiceProperties; +import com.netflix.genie.web.services.AttachmentService; +import org.springframework.core.io.Resource; + +import javax.annotation.Nullable; +import java.io.IOException; +import java.io.InputStream; +import java.net.URI; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Set; +import java.util.UUID; + +/** + * Implementation of {@link AttachmentService} that saves the files to a local directory. + *

+ * N.B.: This implementation is currently used for integration tests and lacks some aspects that would make it usable in + * production environments (e.g., garbage collection of old files, metrics, etc.). + * + * @author mprimi + * @since 4.0.0 + */ +public class LocalFileSystemAttachmentServiceImpl implements AttachmentService { + private final Path attachmentsDirectoryPath; + private final AttachmentServiceProperties attachmentServiceProperties; + + /** + * Constructor. + * + * @param attachmentServiceProperties the service properties + * @throws IOException when failing to create the attachments directory + */ + public LocalFileSystemAttachmentServiceImpl( + final AttachmentServiceProperties attachmentServiceProperties + ) throws IOException { + this.attachmentServiceProperties = attachmentServiceProperties; + this.attachmentsDirectoryPath = Paths.get(attachmentServiceProperties.getLocationPrefix()); + + // Create base attachments directory + Files.createDirectories(this.attachmentsDirectoryPath); + } + + /** + * {@inheritDoc} + */ + @Override + public Set saveAttachments( + @Nullable final String jobId, + final Set attachments + ) throws SaveAttachmentException { + + if (attachments.isEmpty()) { + return Sets.newHashSet(); + } + + final Path attachmentsBasePath = this.attachmentsDirectoryPath.resolve(UUID.randomUUID().toString()); + try { + Files.createDirectories(attachmentsBasePath); + } catch (IOException e) { + throw new SaveAttachmentException("Failed to create directory for attachments: " + e.getMessage(), e); + } + + long totalSize = 0; + + final ImmutableSet.Builder setBuilder = ImmutableSet.builder(); + + for (final Resource attachment : attachments) { + try (InputStream inputStream = attachment.getInputStream()) { + final long attachmentSize = attachment.contentLength(); + final String filename = attachment.getFilename(); + + if (attachmentSize > this.attachmentServiceProperties.getMaxSize().toBytes()) { + throw new AttachmentTooLargeException("Attachment is too large: " + filename); + } + + totalSize += attachmentSize; + + if (totalSize > this.attachmentServiceProperties.getMaxTotalSize().toBytes()) { + throw new AttachmentTooLargeException("Attachments total size is too large"); + } + + final Path attachmentPath = attachmentsBasePath.resolve( + filename != null ? filename : UUID.randomUUID().toString() + ); + + Files.copy(inputStream, attachmentPath); + + setBuilder.add(attachmentPath.toUri()); + + } catch (IOException e) { + throw new SaveAttachmentException("Failed to save attachment: " + e.getMessage(), e); + } + } + + return setBuilder.build(); + } +} diff --git a/genie-web/src/main/java/com/netflix/genie/web/services/impl/S3AttachmentServiceImpl.java b/genie-web/src/main/java/com/netflix/genie/web/services/impl/S3AttachmentServiceImpl.java new file mode 100644 index 00000000000..e993333372c --- /dev/null +++ b/genie-web/src/main/java/com/netflix/genie/web/services/impl/S3AttachmentServiceImpl.java @@ -0,0 +1,225 @@ +/* + * + * Copyright 2020 Netflix, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +package com.netflix.genie.web.services.impl; + +import com.amazonaws.SdkClientException; +import com.amazonaws.services.s3.AmazonS3; +import com.amazonaws.services.s3.AmazonS3URI; +import com.amazonaws.services.s3.model.ObjectMetadata; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; +import com.netflix.genie.common.internal.aws.s3.S3ClientFactory; +import com.netflix.genie.web.exceptions.checked.AttachmentTooLargeException; +import com.netflix.genie.web.exceptions.checked.SaveAttachmentException; +import com.netflix.genie.web.properties.AttachmentServiceProperties; +import com.netflix.genie.web.services.AttachmentService; +import com.netflix.genie.web.util.MetricsUtils; +import io.micrometer.core.instrument.MeterRegistry; +import io.micrometer.core.instrument.Tag; +import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.StringUtils; +import org.springframework.core.io.Resource; + +import javax.annotation.Nullable; +import java.io.IOException; +import java.io.InputStream; +import java.net.URI; +import java.net.URISyntaxException; +import java.util.Set; +import java.util.UUID; +import java.util.concurrent.TimeUnit; + +/** + * Implementation of the AttachmentService interface which saves attachments to AWS S3. + * + * @author mprimi + * @since 4.0.0 + */ +@Slf4j +public class S3AttachmentServiceImpl implements AttachmentService { + + private static final String METRICS_PREFIX = "genie.jobs.attachments.s3"; + private static final String COUNT_DISTRIBUTION = METRICS_PREFIX + ".count.distribution"; + private static final String LARGEST_SIZE_DISTRIBUTION = METRICS_PREFIX + ".largest.distribution"; + private static final String TOTAL_SIZE_DISTRIBUTION = METRICS_PREFIX + ".totalSize.distribution"; + private static final String SAVE_TIMER = METRICS_PREFIX + ".upload.timer"; + private static final Set EMPTY_SET = ImmutableSet.of(); + private static final String SLASH = "/"; + private static final String S3 = "s3"; + private final S3ClientFactory s3ClientFactory; + private final AttachmentServiceProperties properties; + private final MeterRegistry meterRegistry; + private final AmazonS3URI s3BaseURI; + + /** + * Constructor. + * + * @param s3ClientFactory the s3 client factory + * @param attachmentServiceProperties the service properties + * @param meterRegistry the meter registry + */ + public S3AttachmentServiceImpl( + final S3ClientFactory s3ClientFactory, + final AttachmentServiceProperties attachmentServiceProperties, + final MeterRegistry meterRegistry + ) { + this.s3ClientFactory = s3ClientFactory; + this.properties = attachmentServiceProperties; + this.meterRegistry = meterRegistry; + this.s3BaseURI = new AmazonS3URI(attachmentServiceProperties.getLocationPrefix()); + } + + /** + * {@inheritDoc} + */ + @Override + public Set saveAttachments( + @Nullable final String jobId, + final Set attachments + ) throws SaveAttachmentException { + + // Track number of attachments, including zeroes + this.meterRegistry.summary(COUNT_DISTRIBUTION).record(attachments.size()); + + log.debug("Saving {} attachments for job request with id: {}", attachments.size(), jobId); + + if (attachments.size() == 0) { + return EMPTY_SET; + } + + // Check for attachment size limits + this.checkLimits(attachments); + + final long start = System.nanoTime(); + final Set tags = Sets.newHashSet(); + try { + // Upload all to S3 + final Set attachmentURIs = this.uploadAllAttachments(jobId, attachments); + MetricsUtils.addSuccessTags(tags); + return attachmentURIs; + } catch (SaveAttachmentException e) { + log.error("Failed to save attachments (requested job id: {}): {}", jobId, e.getMessage(), e); + MetricsUtils.addFailureTagsWithException(tags, e); + throw e; + } finally { + this.meterRegistry + .timer(SAVE_TIMER, tags) + .record(System.nanoTime() - start, TimeUnit.NANOSECONDS); + } + } + + private void checkLimits(final Set attachments) throws SaveAttachmentException { + + final long singleSizeLimit = this.properties.getMaxSize().toBytes(); + final long totalSizeLimit = this.properties.getMaxTotalSize().toBytes(); + + long totalSize = 0; + long largestSize = 0; + for (final Resource attachment : attachments) { + final String filename = attachment.getFilename(); + + final long attachmentSize; + + try { + attachmentSize = attachment.contentLength(); + } catch (IOException e) { + throw new SaveAttachmentException( + "Failed to get size of attachment: " + filename + ": " + e.getMessage(), + e + ); + } + + if (attachmentSize > largestSize) { + largestSize = attachmentSize; + } + totalSize += attachmentSize; + } + + + if (largestSize > singleSizeLimit) { + throw new AttachmentTooLargeException( + "Size of attachment exceeds the maximum allowed" + + " (" + largestSize + " > " + singleSizeLimit + ")" + ); + } + + if (totalSize > totalSizeLimit) { + throw new AttachmentTooLargeException( + "Total size of attachments exceeds the maximum allowed" + + " (" + totalSize + " > " + totalSizeLimit + ")" + ); + } + + this.meterRegistry.summary(LARGEST_SIZE_DISTRIBUTION).record(largestSize); + this.meterRegistry.summary(TOTAL_SIZE_DISTRIBUTION).record(totalSize); + } + + private Set uploadAllAttachments( + @Nullable final String jobId, + final Set attachments + ) throws SaveAttachmentException { + final AmazonS3 s3Client = this.s3ClientFactory.getClient(this.s3BaseURI); + final String bundleId = UUID.randomUUID().toString(); + final String commonPrefix = this.s3BaseURI.getKey() + SLASH + bundleId + SLASH; + + log.debug( + "Uploading {} attachments for job request with id {} to: {}", + attachments.size(), + jobId, + commonPrefix + ); + + final Set attachmentURIs = Sets.newHashSet(); + + for (final Resource attachment : attachments) { + final String filename = attachment.getFilename(); + if (StringUtils.isBlank(filename)) { + throw new SaveAttachmentException("Attachment filename is missing"); + } + final String objectBucket = this.s3BaseURI.getBucket(); + final String objectKey = commonPrefix + filename; + + final ObjectMetadata metadata = new ObjectMetadata(); + URI attachmentURI = null; + + try (InputStream inputStream = attachment.getInputStream()) { + // Prepare object + metadata.setContentLength(attachment.contentLength()); + attachmentURI = new URI(S3, objectBucket, SLASH + objectKey, null); + // Upload + s3Client.putObject( + objectBucket, + objectKey, + inputStream, + metadata + ); + + // Add attachment URI to the set + attachmentURIs.add(attachmentURI); + + } catch (IOException | SdkClientException | URISyntaxException e) { + throw new SaveAttachmentException( + "Failed to upload attachment: " + attachmentURI + " - " + e.getMessage(), + e + ); + } + } + + return attachmentURIs; + } +} diff --git a/genie-web/src/main/java/com/netflix/genie/web/spring/autoconfigure/data/DataAutoConfiguration.java b/genie-web/src/main/java/com/netflix/genie/web/spring/autoconfigure/data/DataAutoConfiguration.java index db707a4c704..e3bc724b670 100644 --- a/genie-web/src/main/java/com/netflix/genie/web/spring/autoconfigure/data/DataAutoConfiguration.java +++ b/genie-web/src/main/java/com/netflix/genie/web/spring/autoconfigure/data/DataAutoConfiguration.java @@ -29,7 +29,6 @@ import com.netflix.genie.web.data.services.impl.jpa.repositories.JpaJobRepository; import com.netflix.genie.web.data.services.impl.jpa.repositories.JpaRepositories; import com.netflix.genie.web.data.services.impl.jpa.repositories.JpaTagRepository; -import com.netflix.genie.web.services.LegacyAttachmentService; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.domain.EntityScan; import org.springframework.context.annotation.Bean; @@ -103,7 +102,6 @@ public JpaRepositories genieJpaRepositories( * * @param entityManager The {@link EntityManager} for this application * @param jpaRepositories The {@link JpaRepositories} for Genie - * @param legacyAttachmentService The {@link LegacyAttachmentService} implementation to use * @return A {@link JpaPersistenceServiceImpl} instance which implements {@link PersistenceService} backed by * JPA and a relational database */ @@ -111,9 +109,8 @@ public JpaRepositories genieJpaRepositories( @ConditionalOnMissingBean(PersistenceService.class) public JpaPersistenceServiceImpl geniePersistenceService( final EntityManager entityManager, - final JpaRepositories jpaRepositories, - final LegacyAttachmentService legacyAttachmentService + final JpaRepositories jpaRepositories ) { - return new JpaPersistenceServiceImpl(entityManager, jpaRepositories, legacyAttachmentService); + return new JpaPersistenceServiceImpl(entityManager, jpaRepositories); } } diff --git a/genie-web/src/main/java/com/netflix/genie/web/spring/autoconfigure/services/ServicesAutoConfiguration.java b/genie-web/src/main/java/com/netflix/genie/web/spring/autoconfigure/services/ServicesAutoConfiguration.java index bfe47132335..2242cb6b169 100644 --- a/genie-web/src/main/java/com/netflix/genie/web/spring/autoconfigure/services/ServicesAutoConfiguration.java +++ b/genie-web/src/main/java/com/netflix/genie/web/spring/autoconfigure/services/ServicesAutoConfiguration.java @@ -19,6 +19,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.netflix.genie.common.exceptions.GenieException; +import com.netflix.genie.common.internal.aws.s3.S3ClientFactory; import com.netflix.genie.common.internal.services.JobArchiveService; import com.netflix.genie.common.internal.services.JobDirectoryManifestCreatorService; import com.netflix.genie.common.internal.util.GenieHostInfo; @@ -28,6 +29,7 @@ import com.netflix.genie.web.data.services.DataServices; import com.netflix.genie.web.events.GenieEventBus; import com.netflix.genie.web.jobs.workflow.WorkflowTask; +import com.netflix.genie.web.properties.AttachmentServiceProperties; import com.netflix.genie.web.properties.ExponentialBackOffTriggerProperties; import com.netflix.genie.web.properties.FileCacheProperties; import com.netflix.genie.web.properties.JobsActiveLimitProperties; @@ -41,7 +43,7 @@ import com.netflix.genie.web.selectors.ClusterSelector; import com.netflix.genie.web.selectors.CommandSelector; import com.netflix.genie.web.services.ArchivedJobService; -import com.netflix.genie.web.services.LegacyAttachmentService; +import com.netflix.genie.web.services.AttachmentService; import com.netflix.genie.web.services.FileTransferFactory; import com.netflix.genie.web.services.JobCoordinatorService; import com.netflix.genie.web.services.JobDirectoryServerService; @@ -52,6 +54,7 @@ import com.netflix.genie.web.services.JobResolverService; import com.netflix.genie.web.services.JobStateService; import com.netflix.genie.web.services.JobSubmitterService; +import com.netflix.genie.web.services.LegacyAttachmentService; import com.netflix.genie.web.services.MailService; import com.netflix.genie.web.services.impl.ArchivedJobServiceImpl; import com.netflix.genie.web.services.impl.CacheGenieFileTransferService; @@ -64,8 +67,10 @@ import com.netflix.genie.web.services.impl.JobKillServiceV3; import com.netflix.genie.web.services.impl.JobLaunchServiceImpl; import com.netflix.genie.web.services.impl.JobResolverServiceImpl; +import com.netflix.genie.web.services.impl.LocalFileSystemAttachmentServiceImpl; import com.netflix.genie.web.services.impl.LocalFileTransferImpl; import com.netflix.genie.web.services.impl.LocalJobRunner; +import com.netflix.genie.web.services.impl.S3AttachmentServiceImpl; import com.netflix.genie.web.tasks.job.JobCompletionService; import com.netflix.genie.web.util.ProcessChecker; import io.micrometer.core.instrument.MeterRegistry; @@ -83,7 +88,9 @@ import org.springframework.retry.support.RetryTemplate; import javax.validation.constraints.NotEmpty; +import javax.validation.constraints.NotNull; import java.io.IOException; +import java.net.URI; import java.util.List; /** @@ -104,6 +111,7 @@ JobsUsersProperties.class, ExponentialBackOffTriggerProperties.class, JobsActiveLimitProperties.class, + AttachmentServiceProperties.class } ) @Slf4j @@ -319,6 +327,35 @@ public FileSystemAttachmentService legacyAttachmentService(final JobsProperties return new FileSystemAttachmentService(jobsProperties.getLocations().getAttachments().toString()); } + /** + * The attachment service to use. + * + * @param s3ClientFactory the S3 client factory + * @param attachmentServiceProperties the service properties + * @param meterRegistry the meter registry + * @return The attachment service to use + * @throws IOException if the local filesystem implmentation is used and it fails to initialize + */ + @Bean + @ConditionalOnMissingBean(AttachmentService.class) + public AttachmentService attachmentService( + final S3ClientFactory s3ClientFactory, + final AttachmentServiceProperties attachmentServiceProperties, + final MeterRegistry meterRegistry + ) throws IOException { + final @NotNull URI location = attachmentServiceProperties.getLocationPrefix(); + final String scheme = location.getScheme(); + if ("s3".equals(scheme)) { + return new S3AttachmentServiceImpl(s3ClientFactory, attachmentServiceProperties, meterRegistry); + } else if ("file".equals(scheme)) { + return new LocalFileSystemAttachmentServiceImpl(attachmentServiceProperties); + } else { + throw new IllegalStateException( + "Unknown attachment service implementation to use for location: " + location + ); + } + } + /** * FileTransfer factory. * diff --git a/genie-web/src/main/resources/genie-web-defaults.yml b/genie-web/src/main/resources/genie-web-defaults.yml index e2f08f5fc1e..f5d212a819f 100644 --- a/genie-web/src/main/resources/genie-web-defaults.yml +++ b/genie-web/src/main/resources/genie-web-defaults.yml @@ -48,6 +48,10 @@ genie: health: maxCpuLoadPercent: 80 jobs: + attachments: + location-prefix: file://${java.io.tmpdir:/tmp}/genie/attachments + max-size: 100MB + max-total-size: 150MB cleanup: deleteDependencies: true forwarding: diff --git a/genie-web/src/test/groovy/com/netflix/genie/web/aspects/DataServiceRetryAspectSpec.groovy b/genie-web/src/test/groovy/com/netflix/genie/web/aspects/DataServiceRetryAspectSpec.groovy index cfed638b9b7..c9e7de8bc17 100644 --- a/genie-web/src/test/groovy/com/netflix/genie/web/aspects/DataServiceRetryAspectSpec.groovy +++ b/genie-web/src/test/groovy/com/netflix/genie/web/aspects/DataServiceRetryAspectSpec.groovy @@ -162,12 +162,5 @@ class DataServiceRetryAspectSpec extends Specification { then: thrown(IdAlreadyExistsException.class) 1 * dataService.saveJobSubmission(jobSubmission) >> { throw new IdAlreadyExistsException("conflict") } - - when: - dataServiceProxy.saveJobSubmission(jobSubmission) - - then: - thrown(SaveAttachmentException.class) - 1 * dataService.saveJobSubmission(jobSubmission) >> { throw new SaveAttachmentException("bad") } } } diff --git a/genie-web/src/test/groovy/com/netflix/genie/web/dtos/JobSubmissionSpec.groovy b/genie-web/src/test/groovy/com/netflix/genie/web/dtos/JobSubmissionSpec.groovy index 2b8fe94ef40..8fb9dca5ec5 100644 --- a/genie-web/src/test/groovy/com/netflix/genie/web/dtos/JobSubmissionSpec.groovy +++ b/genie-web/src/test/groovy/com/netflix/genie/web/dtos/JobSubmissionSpec.groovy @@ -32,8 +32,8 @@ class JobSubmissionSpec extends Specification { def "Can build"() { def jobRequest = Mock(JobRequest) def jobRequestMetadata = Mock(JobRequestMetadata) - def attachment1 = Mock(Resource) - def attachment2 = Mock(Resource) + def attachment1 = URI.create("s3://some-bucket/scripts/script1.sql") + def attachment2 = URI.create("s3://some-bucket/scripts/script2.sql") def builder = new JobSubmission.Builder(jobRequest, jobRequestMetadata) @@ -53,10 +53,9 @@ class JobSubmissionSpec extends Specification { submission2.getJobRequestMetadata() == jobRequestMetadata submission2.getAttachments().size() == 2 submission2.getAttachments().containsAll([attachment1, attachment2]) - // note the attachments are ignored - submission1.toString() == submission2.toString() - submission1.hashCode() == submission2.hashCode() - submission1 == submission2 + submission1.toString() != submission2.toString() + submission1.hashCode() != submission2.hashCode() + submission1 != submission2 submission1.getAttachments() != submission2.getAttachments() when: @@ -68,10 +67,9 @@ class JobSubmissionSpec extends Specification { submission3.getAttachments().size() == 2 submission3.getAttachments().containsAll([attachment1, attachment2]) // note the attachments are ignored - submission1.toString() == submission3.toString() - submission1.hashCode() == submission3.hashCode() - submission1 == submission3 - submission1.getAttachments() != submission3.getAttachments() + submission2.toString() == submission3.toString() + submission2.hashCode() == submission3.hashCode() + submission2 == submission3 submission2.getAttachments() == submission3.getAttachments() when: diff --git a/genie-web/src/test/groovy/com/netflix/genie/web/exceptions/checked/GenieWebCheckedExceptionsSpec.groovy b/genie-web/src/test/groovy/com/netflix/genie/web/exceptions/checked/GenieWebCheckedExceptionsSpec.groovy index 5befd6252ea..e103242a9fa 100644 --- a/genie-web/src/test/groovy/com/netflix/genie/web/exceptions/checked/GenieWebCheckedExceptionsSpec.groovy +++ b/genie-web/src/test/groovy/com/netflix/genie/web/exceptions/checked/GenieWebCheckedExceptionsSpec.groovy @@ -65,6 +65,7 @@ class GenieWebCheckedExceptionsSpec extends Specification { where: exceptionClass | _ AgentLaunchException | _ + AttachmentTooLargeException | _ IdAlreadyExistsException | _ JobDirectoryManifestNotFoundException | _ JobNotArchivedException | _ diff --git a/genie-web/src/test/groovy/com/netflix/genie/web/properties/AttachmentServicePropertiesSpec.groovy b/genie-web/src/test/groovy/com/netflix/genie/web/properties/AttachmentServicePropertiesSpec.groovy new file mode 100644 index 00000000000..274bea97b04 --- /dev/null +++ b/genie-web/src/test/groovy/com/netflix/genie/web/properties/AttachmentServicePropertiesSpec.groovy @@ -0,0 +1,44 @@ +/* + * + * Copyright 2020 Netflix, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +package com.netflix.genie.web.properties + +import org.springframework.util.unit.DataSize +import spock.lang.Specification + +class AttachmentServicePropertiesSpec extends Specification { + + def "Defaults, getters, setters"() { + when: + AttachmentServiceProperties props = new AttachmentServiceProperties() + + then: + props.getLocationPrefix().toString() ==~ /file:\/\/\/.+\/genie\/attachments/ + props.getMaxSize() == DataSize.ofMegabytes(100) + props.getMaxTotalSize() == DataSize.ofMegabytes(150) + + when: + props.setLocationPrefix(URI.create("s3://genie-attachments/prod")) + props.setMaxSize(DataSize.ofMegabytes(50)) + props.setMaxTotalSize(DataSize.ofMegabytes(75)) + + then: + props.getLocationPrefix() == URI.create("s3://genie-attachments/prod") + props.getMaxSize() == DataSize.ofMegabytes(50) + props.getMaxTotalSize() == DataSize.ofMegabytes(75) + } +} diff --git a/genie-web/src/test/groovy/com/netflix/genie/web/services/impl/JobLaunchServiceImplSpec.groovy b/genie-web/src/test/groovy/com/netflix/genie/web/services/impl/JobLaunchServiceImplSpec.groovy index ff5d87c7348..637a36dca0e 100644 --- a/genie-web/src/test/groovy/com/netflix/genie/web/services/impl/JobLaunchServiceImplSpec.groovy +++ b/genie-web/src/test/groovy/com/netflix/genie/web/services/impl/JobLaunchServiceImplSpec.groovy @@ -108,19 +108,6 @@ class JobLaunchServiceImplSpec extends Specification { when: service.launchJob(jobSubmission) - then: - 1 * persistenceService.saveJobSubmission(jobSubmission) >> { - throw new SaveAttachmentException("hmm that's not good") - } - 0 * jobResolverService.resolveJob(_ as String) - 0 * persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) - 0 * persistenceService.updateJobArchiveStatus(_, _) - 0 * agentLauncher.launchAgent(_ as ResolvedJob) - thrown(SaveAttachmentException) - - when: - service.launchJob(jobSubmission) - then: 1 * persistenceService.saveJobSubmission(jobSubmission) >> jobId 1 * jobResolverService.resolveJob(jobId) >> { diff --git a/genie-web/src/test/groovy/com/netflix/genie/web/services/impl/LocalFileSystemAttachmentServiceImplSpec.groovy b/genie-web/src/test/groovy/com/netflix/genie/web/services/impl/LocalFileSystemAttachmentServiceImplSpec.groovy new file mode 100644 index 00000000000..7a458719a93 --- /dev/null +++ b/genie-web/src/test/groovy/com/netflix/genie/web/services/impl/LocalFileSystemAttachmentServiceImplSpec.groovy @@ -0,0 +1,153 @@ +/* + * + * Copyright 2020 Netflix, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +package com.netflix.genie.web.services.impl + +import com.google.common.collect.Sets +import com.netflix.genie.web.exceptions.checked.AttachmentTooLargeException +import com.netflix.genie.web.exceptions.checked.SaveAttachmentException +import com.netflix.genie.web.properties.AttachmentServiceProperties +import org.apache.commons.io.FileUtils +import org.apache.commons.lang3.RandomStringUtils +import org.junit.Rule +import org.junit.rules.TemporaryFolder +import org.springframework.core.io.FileSystemResource +import org.springframework.core.io.Resource +import org.springframework.util.unit.DataSize +import spock.lang.Specification +import spock.lang.Unroll + +import java.nio.file.Paths +import java.util.stream.Collectors + +class LocalFileSystemAttachmentServiceImplSpec extends Specification { + + AttachmentServiceProperties serviceProperties + LocalFileSystemAttachmentServiceImpl service + + @Rule + TemporaryFolder temporaryFolder + + + def setup() { + this.serviceProperties = new AttachmentServiceProperties() + this.serviceProperties.setLocationPrefix(temporaryFolder.getRoot().toURI()) + this.serviceProperties.setMaxSize(DataSize.ofBytes(100)) + this.serviceProperties.setMaxTotalSize(DataSize.ofBytes(150)) + this.service = new LocalFileSystemAttachmentServiceImpl(serviceProperties) + } + + def "saveAttachments with no attachments"() { + Set attachments = Sets.newHashSet() + + when: + Set attachmentUris = service.saveAttachments(null, attachments) + + then: + attachmentUris.isEmpty() + } + + @Unroll + def "saveAttachments (job id present: #jobIdPresent)"() { + File input1 = temporaryFolder.newFile("file1.txt") + File input2 = temporaryFolder.newFile("file2.txt") + input1.write(RandomStringUtils.randomAscii(50)) + input2.write(RandomStringUtils.randomAscii(80)) + Resource resource1 = new FileSystemResource(input1) + Resource resource2 = new FileSystemResource(input2) + Set attachments = Sets.newHashSet(resource1, resource2) + + String jobId = null + if (jobIdPresent) { + jobId = UUID.randomUUID().toString() + } + + when: + Set attachmentUris = service.saveAttachments(jobId, attachments) + + then: + attachmentUris.size() == 2 + List attachmentsList = attachmentUris.stream().map({uri -> Paths.get(uri).toAbsolutePath().toString()}).collect(Collectors.toList()) + Collections.sort(attachmentsList) + FileUtils.contentEquals(new File(attachmentsList.get(0)), input1) + FileUtils.contentEquals(new File(attachmentsList.get(1)), input2) + + where: + jobIdPresent << [true, false] + } + + @Unroll + def "reject attachments with sizes: #firstFileSize and #secondFileSize"() { + File input1 = temporaryFolder.newFile("file1.txt") + File input2 = temporaryFolder.newFile("file2.txt") + input1.write(RandomStringUtils.randomAscii(firstFileSize)) + input2.write(RandomStringUtils.randomAscii(secondFileSize)) + Resource resource1 = new FileSystemResource(input1) + Resource resource2 = new FileSystemResource(input2) + Set attachments = Sets.newHashSet(resource1, resource2) + + when: + service.saveAttachments(null, attachments) + + then: + thrown(AttachmentTooLargeException) + + + where: + firstFileSize | secondFileSize + 110 | 10 + 10 | 110 + 100 | 100 + } + + def "saveAttachments copy exception (job id present: #jobIdPresent)"() { + File input = temporaryFolder.newFile("file1.txt") + input.write(RandomStringUtils.randomAscii(50)) + Resource resource = new FileSystemResource(input) + Set attachments = Sets.newHashSet(resource) + input.delete() + + when: + service.saveAttachments(null, attachments) + + then: + thrown(SaveAttachmentException) + } + + def "saveAttachments create base directory exception"() { + this.serviceProperties.setLocationPrefix(Paths.get("/likely-fail-to-create/genie/attachments").toUri()) + + when: + this.service = new LocalFileSystemAttachmentServiceImpl(serviceProperties) + + then: + thrown(IOException) + } + + def "saveAttachments create attachments directory exception"() { + File base = new File(serviceProperties.getLocationPrefix()) + FileUtils.deleteDirectory(base) + base.createNewFile() + Set attachments = Sets.newHashSet(Mock(Resource)) + + when: + service.saveAttachments(null, attachments) + + then: + thrown(SaveAttachmentException) + } +} diff --git a/genie-web/src/test/groovy/com/netflix/genie/web/services/impl/S3AttachmentServiceImplSpec.groovy b/genie-web/src/test/groovy/com/netflix/genie/web/services/impl/S3AttachmentServiceImplSpec.groovy new file mode 100644 index 00000000000..8c47a9e808c --- /dev/null +++ b/genie-web/src/test/groovy/com/netflix/genie/web/services/impl/S3AttachmentServiceImplSpec.groovy @@ -0,0 +1,259 @@ +/* + * + * Copyright 2020 Netflix, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +package com.netflix.genie.web.services.impl + +import com.amazonaws.SdkClientException +import com.amazonaws.services.s3.AmazonS3 +import com.amazonaws.services.s3.AmazonS3URI +import com.amazonaws.services.s3.model.ObjectMetadata +import com.google.common.collect.Sets +import com.netflix.genie.common.internal.aws.s3.S3ClientFactory +import com.netflix.genie.web.exceptions.checked.AttachmentTooLargeException +import com.netflix.genie.web.exceptions.checked.SaveAttachmentException +import com.netflix.genie.web.properties.AttachmentServiceProperties +import io.micrometer.core.instrument.DistributionSummary +import io.micrometer.core.instrument.MeterRegistry +import io.micrometer.core.instrument.Timer +import org.springframework.core.io.Resource +import org.springframework.util.unit.DataSize +import spock.lang.Specification +import spock.lang.Unroll + +import java.util.concurrent.TimeUnit + +class S3AttachmentServiceImplSpec extends Specification { + public static final String BUCKET_NAME = "some-bucket" + public static final String S3_PREFIX = "some/prefix" + S3ClientFactory s3ClientFactory + AttachmentServiceProperties serviceProperties + MeterRegistry registry + S3AttachmentServiceImpl service + DistributionSummary distributionSummary + Timer timer + AmazonS3 s3Client + InputStream inputStream + + void setup() { + this.distributionSummary = Mock(DistributionSummary) + this.timer = Mock(Timer) + this.s3Client = Mock(AmazonS3) + this.inputStream = Mock(InputStream) + + this.s3ClientFactory = Mock(S3ClientFactory) + this.serviceProperties = new AttachmentServiceProperties() + this.registry = Mock(MeterRegistry) + + this.serviceProperties.setLocationPrefix(URI.create("s3://" + BUCKET_NAME + "/" + S3_PREFIX)) + + this.service = new S3AttachmentServiceImpl(s3ClientFactory, serviceProperties, registry) + + } + + @Unroll + def "No attachments (job id present: #jobIdPresent)"() { + setup: + String jobId = jobIdPresent ? UUID.randomUUID().toString() : null + + when: + Set attachmentURIs = this.service.saveAttachments(jobId, Sets.newHashSet()) + + then: + 1 * registry.summary(S3AttachmentServiceImpl.COUNT_DISTRIBUTION) >> distributionSummary + 1 * distributionSummary.record(0) + attachmentURIs.isEmpty() + + where: + jobIdPresent << [true, false] + } + + @Unroll + def "Pre-upload errors (job id present: #jobIdPresent)"() { + setup: + String jobId = jobIdPresent ? UUID.randomUUID().toString() : null + Resource attachment1 = Mock(Resource) + Resource attachment2 = Mock(Resource) + serviceProperties.setMaxSize(DataSize.ofBytes(60)) + serviceProperties.setMaxTotalSize(DataSize.ofBytes(100)) + + when: "Attachment content throws IOException" + this.service.saveAttachments(jobId, Sets.newHashSet(attachment1)) + + then: + 1 * registry.summary(S3AttachmentServiceImpl.COUNT_DISTRIBUTION) >> distributionSummary + 1 * distributionSummary.record(1) + 1 * attachment1.getFilename() >> "script.sql" + 1 * attachment1.contentLength() >> { throw new IOException("...") } + thrown(SaveAttachmentException) + + when: "Attachment size too large" + this.service.saveAttachments(jobId, Sets.newHashSet(attachment1)) + + then: + 1 * registry.summary(S3AttachmentServiceImpl.COUNT_DISTRIBUTION) >> distributionSummary + 1 * distributionSummary.record(1) + 1 * attachment1.getFilename() >> "script.sql" + 1 * attachment1.contentLength() >> 80 + thrown(AttachmentTooLargeException) + + when: "Attachments total size too large" + this.service.saveAttachments(jobId, Sets.newHashSet(attachment1, attachment2)) + + then: + 1 * registry.summary(S3AttachmentServiceImpl.COUNT_DISTRIBUTION) >> distributionSummary + 1 * distributionSummary.record(2) + 1 * attachment1.getFilename() >> "script1.sql" + 1 * attachment1.contentLength() >> 60 + 1 * attachment2.getFilename() >> "script2.sql" + 1 * attachment2.contentLength() >> 60 + thrown(AttachmentTooLargeException) + + where: + jobIdPresent << [true, false] + } + + @Unroll + def "Successful (job id present: #jobIdPresent)"() { + setup: + String jobId = jobIdPresent ? UUID.randomUUID().toString() : null + Resource attachment1 = Mock(Resource) + Resource attachment2 = Mock(Resource) + URL url1 = new URL("https://" + BUCKET_NAME + "/" + S3_PREFIX + "/bundle-uuid/script1.sql") + URL url2 = new URL("https://" + BUCKET_NAME + "/" + S3_PREFIX + "/bundle-uuid/script2.sql") + + when: "Attachments total size too large" + Set attachmentUris = this.service.saveAttachments(jobId, Sets.newHashSet(attachment1, attachment2)) + + then: + 1 * registry.summary(S3AttachmentServiceImpl.COUNT_DISTRIBUTION) >> distributionSummary + 1 * distributionSummary.record(2) + 1 * attachment1.getFilename() >> "script1.sql" + 1 * attachment1.contentLength() >> DataSize.ofMegabytes(3).toBytes() + 1 * attachment2.getFilename() >> "script2.sql" + 1 * attachment2.contentLength() >> DataSize.ofMegabytes(5).toBytes() + 1 * registry.summary(S3AttachmentServiceImpl.LARGEST_SIZE_DISTRIBUTION) >> distributionSummary + 1 * registry.summary(S3AttachmentServiceImpl.TOTAL_SIZE_DISTRIBUTION) >> distributionSummary + 1 * distributionSummary.record(5 * 1024 * 1024) + 1 * distributionSummary.record((5 + 3) * 1024 * 1024) + 1 * s3ClientFactory.getClient(_ as AmazonS3URI) >> { + AmazonS3URI s3Uri -> + assert s3Uri.getBucket() == BUCKET_NAME + assert s3Uri.getKey() == S3_PREFIX + return s3Client + } + 1 * attachment1.getFilename() >> "script1.sql" + 1 * attachment1.contentLength() >> DataSize.ofMegabytes(3).toBytes() + 1 * attachment1.getInputStream() >> inputStream + 1 * attachment2.getFilename() >> "script2.sql" + 1 * attachment2.contentLength() >> DataSize.ofMegabytes(5).toBytes() + 1 * attachment2.getInputStream() >> inputStream + 2 * inputStream.close() + 2 * s3Client.putObject( + BUCKET_NAME, + { it as String ==~ /some\/prefix\/.+\/script[12]\.sql/ }, + inputStream, + !null as ObjectMetadata + ) + 1 * registry.timer(S3AttachmentServiceImpl.SAVE_TIMER, _) >> timer + 1 * timer.record(_ , TimeUnit.NANOSECONDS) + attachmentUris.size() == 2 + attachmentUris.findAll({it.toString() ==~ /s3:\/\/some-bucket\/some\/prefix\/.+\/script[12]\.sql/}).size() == 2 + + where: + jobIdPresent << [true, false] + } + + @Unroll + def "Upload errors (job id present: #jobIdPresent)"() { + setup: + String jobId = jobIdPresent ? UUID.randomUUID().toString() : null + Resource attachment1 = Mock(Resource) + + when: "Attachments total size too large" + this.service.saveAttachments(jobId, Sets.newHashSet(attachment1)) + + then: + 1 * registry.summary(S3AttachmentServiceImpl.COUNT_DISTRIBUTION) >> distributionSummary + 1 * distributionSummary.record(1) + 1 * attachment1.getFilename() >> "script.sql" + 1 * attachment1.contentLength() >> DataSize.ofMegabytes(3).toBytes() + 1 * registry.summary(S3AttachmentServiceImpl.LARGEST_SIZE_DISTRIBUTION) >> distributionSummary + 1 * registry.summary(S3AttachmentServiceImpl.TOTAL_SIZE_DISTRIBUTION) >> distributionSummary + 1 * distributionSummary.record(3 * 1024 * 1024) + 1 * distributionSummary.record(3 * 1024 * 1024) + 1 * s3ClientFactory.getClient(_ as AmazonS3URI) >> { + AmazonS3URI s3Uri -> + assert s3Uri.getBucket() == BUCKET_NAME + assert s3Uri.getKey() == S3_PREFIX + return s3Client + } + 1 * attachment1.getFilename() >> "script.sql" + 1 * attachment1.contentLength() >> DataSize.ofMegabytes(3).toBytes() + 1 * attachment1.getInputStream() >> inputStream + 1 * inputStream.close() + 1 * s3Client.putObject( + BUCKET_NAME, + { it as String ==~ /some\/prefix\/.+\/script\.sql/ }, + inputStream, + !null as ObjectMetadata + ) >> { + throw new SdkClientException("...") + } + 1 * registry.timer(S3AttachmentServiceImpl.SAVE_TIMER, _) >> timer + 1 * timer.record(_ , TimeUnit.NANOSECONDS) + thrown(SaveAttachmentException) + + where: + jobIdPresent << [true, false] + } + + @Unroll + def "Invalid attachment (filename: #attachmentFilename)"() { + setup: + String jobId = UUID.randomUUID().toString() + Resource attachment1 = Mock(Resource) + + when: "Attachments total size too large" + this.service.saveAttachments(jobId, Sets.newHashSet(attachment1)) + + then: + 1 * registry.summary(S3AttachmentServiceImpl.COUNT_DISTRIBUTION) >> distributionSummary + 1 * distributionSummary.record(1) + 1 * attachment1.getFilename() >> attachmentFilename + 1 * attachment1.contentLength() >> DataSize.ofMegabytes(3).toBytes() + 1 * registry.summary(S3AttachmentServiceImpl.LARGEST_SIZE_DISTRIBUTION) >> distributionSummary + 1 * registry.summary(S3AttachmentServiceImpl.TOTAL_SIZE_DISTRIBUTION) >> distributionSummary + 1 * distributionSummary.record(3 * 1024 * 1024) + 1 * distributionSummary.record(3 * 1024 * 1024) + 1 * s3ClientFactory.getClient(_ as AmazonS3URI) >> { + AmazonS3URI s3Uri -> + assert s3Uri.getBucket() == BUCKET_NAME + assert s3Uri.getKey() == S3_PREFIX + return s3Client + } + 1 * attachment1.getFilename() >> attachmentFilename + 0 * attachment1.getInputStream() + 0 * s3Client.putObject(*_) + 0 * s3Client.getUrl(*_) + 1 * registry.timer(S3AttachmentServiceImpl.SAVE_TIMER, _) >> timer + 1 * timer.record(_ , TimeUnit.NANOSECONDS) + thrown(SaveAttachmentException) + + where: + attachmentFilename << [null, "", " "] + } +} diff --git a/genie-web/src/test/java/com/netflix/genie/web/apis/rest/v3/controllers/GenieExceptionMapperTest.java b/genie-web/src/test/java/com/netflix/genie/web/apis/rest/v3/controllers/GenieExceptionMapperTest.java index 89a55178c29..76157d97f66 100644 --- a/genie-web/src/test/java/com/netflix/genie/web/apis/rest/v3/controllers/GenieExceptionMapperTest.java +++ b/genie-web/src/test/java/com/netflix/genie/web/apis/rest/v3/controllers/GenieExceptionMapperTest.java @@ -39,6 +39,7 @@ import com.netflix.genie.common.internal.exceptions.unchecked.GenieJobNotFoundException; import com.netflix.genie.common.internal.exceptions.unchecked.GenieJobSpecificationNotFoundException; import com.netflix.genie.common.internal.exceptions.unchecked.GenieRuntimeException; +import com.netflix.genie.web.exceptions.checked.AttachmentTooLargeException; import com.netflix.genie.web.exceptions.checked.IdAlreadyExistsException; import com.netflix.genie.web.exceptions.checked.JobNotFoundException; import com.netflix.genie.web.exceptions.checked.NotFoundException; @@ -233,6 +234,7 @@ void canHandleGenieCheckedExceptions() { exceptions.put(new JobNotFoundException(), HttpStatus.NOT_FOUND); exceptions.put(new NotFoundException(), HttpStatus.NOT_FOUND); exceptions.put(new PreconditionFailedException(), HttpStatus.BAD_REQUEST); + exceptions.put(new AttachmentTooLargeException(), HttpStatus.PAYLOAD_TOO_LARGE); for (final Map.Entry exception : exceptions.entrySet()) { final ResponseEntity response = diff --git a/genie-web/src/test/java/com/netflix/genie/web/apis/rest/v3/controllers/JobRestControllerTest.java b/genie-web/src/test/java/com/netflix/genie/web/apis/rest/v3/controllers/JobRestControllerTest.java index ec261701fec..4e8907cb555 100644 --- a/genie-web/src/test/java/com/netflix/genie/web/apis/rest/v3/controllers/JobRestControllerTest.java +++ b/genie-web/src/test/java/com/netflix/genie/web/apis/rest/v3/controllers/JobRestControllerTest.java @@ -43,6 +43,7 @@ import com.netflix.genie.web.data.services.PersistenceService; import com.netflix.genie.web.exceptions.checked.NotFoundException; import com.netflix.genie.web.properties.JobsProperties; +import com.netflix.genie.web.services.AttachmentService; import com.netflix.genie.web.services.LegacyAttachmentService; import com.netflix.genie.web.services.JobCoordinatorService; import com.netflix.genie.web.services.JobDirectoryServerService; @@ -158,6 +159,7 @@ void setup() { registry, this.agentRoutingService, this.environment, + Mockito.mock(AttachmentService.class), Mockito.mock(LegacyAttachmentService.class), jobExecutionModeSelector ); @@ -843,6 +845,7 @@ void canHandleForwardJobOutputRequestWithSuccess() throws IOException, GenieExce registry, this.agentRoutingService, this.environment, + Mockito.mock(AttachmentService.class), Mockito.mock(LegacyAttachmentService.class), this.jobExecutionModeSelector ); diff --git a/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplApplicationsTest.java b/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplApplicationsTest.java index 3a201337e1e..172f67581d6 100644 --- a/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplApplicationsTest.java +++ b/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplApplicationsTest.java @@ -31,7 +31,6 @@ import com.netflix.genie.web.exceptions.checked.IdAlreadyExistsException; import com.netflix.genie.web.exceptions.checked.NotFoundException; import com.netflix.genie.web.exceptions.checked.PreconditionFailedException; -import com.netflix.genie.web.services.LegacyAttachmentService; import org.assertj.core.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -65,8 +64,7 @@ void setup() { Mockito.when(jpaRepositories.getApplicationRepository()).thenReturn(this.jpaApplicationRepository); this.persistenceService = new JpaPersistenceServiceImpl( Mockito.mock(EntityManager.class), - jpaRepositories, - Mockito.mock(LegacyAttachmentService.class) + jpaRepositories ); } diff --git a/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplClustersTest.java b/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplClustersTest.java index 5fd20882cdc..b509e7f71be 100644 --- a/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplClustersTest.java +++ b/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplClustersTest.java @@ -31,7 +31,6 @@ import com.netflix.genie.web.exceptions.checked.IdAlreadyExistsException; import com.netflix.genie.web.exceptions.checked.NotFoundException; import com.netflix.genie.web.exceptions.checked.PreconditionFailedException; -import com.netflix.genie.web.services.LegacyAttachmentService; import org.assertj.core.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -70,8 +69,7 @@ void setup() { Mockito.when(jpaRepositories.getFileRepository()).thenReturn(this.jpaFileRepository); this.service = new JpaPersistenceServiceImpl( Mockito.mock(EntityManager.class), - jpaRepositories, - Mockito.mock(LegacyAttachmentService.class) + jpaRepositories ); } diff --git a/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplCommandsTest.java b/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplCommandsTest.java index 143adffc545..27da2a4a3a4 100644 --- a/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplCommandsTest.java +++ b/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplCommandsTest.java @@ -32,7 +32,6 @@ import com.netflix.genie.web.exceptions.checked.IdAlreadyExistsException; import com.netflix.genie.web.exceptions.checked.NotFoundException; import com.netflix.genie.web.exceptions.checked.PreconditionFailedException; -import com.netflix.genie.web.services.LegacyAttachmentService; import org.assertj.core.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -76,8 +75,7 @@ void setup() { Mockito.when(jpaRepositories.getCriterionRepository()).thenReturn(Mockito.mock(JpaCriterionRepository.class)); this.service = new JpaPersistenceServiceImpl( Mockito.mock(EntityManager.class), - jpaRepositories, - Mockito.mock(LegacyAttachmentService.class) + jpaRepositories ); } diff --git a/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsTest.java b/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsTest.java index 76a52741ce6..101451502d0 100644 --- a/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsTest.java +++ b/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsTest.java @@ -60,7 +60,6 @@ import com.netflix.genie.web.data.services.impl.jpa.repositories.JpaTagRepository; import com.netflix.genie.web.dtos.ResolvedJob; import com.netflix.genie.web.exceptions.checked.NotFoundException; -import com.netflix.genie.web.services.LegacyAttachmentService; import org.apache.commons.lang3.StringUtils; import org.assertj.core.api.Assertions; import org.junit.jupiter.api.BeforeEach; @@ -123,8 +122,7 @@ void setup() { this.persistenceService = new JpaPersistenceServiceImpl( Mockito.mock(EntityManager.class), - jpaRepositories, - Mockito.mock(LegacyAttachmentService.class) + jpaRepositories ); } diff --git a/genie-web/src/test/java/com/netflix/genie/web/spring/autoconfigure/services/ServicesAutoConfigurationTest.java b/genie-web/src/test/java/com/netflix/genie/web/spring/autoconfigure/services/ServicesAutoConfigurationTest.java index 812f269cc23..1acc614120b 100644 --- a/genie-web/src/test/java/com/netflix/genie/web/spring/autoconfigure/services/ServicesAutoConfigurationTest.java +++ b/genie-web/src/test/java/com/netflix/genie/web/spring/autoconfigure/services/ServicesAutoConfigurationTest.java @@ -19,6 +19,7 @@ import com.netflix.genie.common.exceptions.GenieException; import com.netflix.genie.common.external.util.GenieObjectMapper; +import com.netflix.genie.common.internal.aws.s3.S3ClientFactory; import com.netflix.genie.common.internal.services.JobDirectoryManifestCreatorService; import com.netflix.genie.common.internal.util.GenieHostInfo; import com.netflix.genie.web.agent.services.AgentFileStreamService; @@ -27,6 +28,7 @@ import com.netflix.genie.web.data.services.PersistenceService; import com.netflix.genie.web.events.GenieEventBus; import com.netflix.genie.web.jobs.workflow.WorkflowTask; +import com.netflix.genie.web.properties.AttachmentServiceProperties; import com.netflix.genie.web.properties.ExponentialBackOffTriggerProperties; import com.netflix.genie.web.properties.FileCacheProperties; import com.netflix.genie.web.properties.JobsActiveLimitProperties; @@ -45,7 +47,9 @@ import com.netflix.genie.web.services.JobResolverService; import com.netflix.genie.web.services.JobStateService; import com.netflix.genie.web.services.impl.JobKillServiceV3; +import com.netflix.genie.web.services.impl.LocalFileSystemAttachmentServiceImpl; import com.netflix.genie.web.services.impl.LocalFileTransferImpl; +import com.netflix.genie.web.services.impl.S3AttachmentServiceImpl; import com.netflix.genie.web.util.ProcessChecker; import io.micrometer.core.instrument.MeterRegistry; import org.apache.commons.exec.Executor; @@ -58,6 +62,8 @@ import org.springframework.core.io.Resource; import org.springframework.core.io.ResourceLoader; +import java.io.IOException; +import java.net.URI; import java.nio.file.Path; import java.util.ArrayList; import java.util.List; @@ -70,6 +76,7 @@ * @since 3.0.0 */ class ServicesAutoConfigurationTest { + //TODO update this test class to use ContextRunner, like the rest of configuration tests private ServicesAutoConfiguration servicesAutoConfiguration; @@ -208,4 +215,32 @@ void canGetJobDirectoryServerServiceBean() { ) .isNotNull(); } + + @Test + void canGetS3AttachmentServiceServiceBean() throws IOException { + + final AttachmentServiceProperties properties = new AttachmentServiceProperties(); + + Assertions + .assertThat( + this.servicesAutoConfiguration.attachmentService( + Mockito.mock(S3ClientFactory.class), + properties, + Mockito.mock(MeterRegistry.class) + ) + ) + .isInstanceOf(LocalFileSystemAttachmentServiceImpl.class); + + properties.setLocationPrefix(URI.create("s3://foo/bar/genie/attachments")); + + Assertions + .assertThat( + this.servicesAutoConfiguration.attachmentService( + Mockito.mock(S3ClientFactory.class), + properties, + Mockito.mock(MeterRegistry.class) + ) + ) + .isInstanceOf(S3AttachmentServiceImpl.class); + } }