Development: Cache windfiles on startup
#7817
Conversation
github-actions
bot
added
the
server
reschandreas
force-pushed
the
bugfix/fix-missing-aeolus-templates
branch
from
94a5643 to
6201add
Compare
reschandreas
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
reschandreas
force-pushed
the
bugfix/fix-missing-aeolus-templates
branch
2 times, most recently
from
91ddac8 to
55869e1
Compare
reschandreas
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
reschandreas
force-pushed
the
bugfix/fix-missing-aeolus-templates
branch
2 times, most recently
from
3c3f03c to
6713447
Compare
reschandreas
force-pushed
the
bugfix/fix-missing-aeolus-templates
branch
from
6713447 to
3409e5c
Compare
reschandreas
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
reschandreas
added
deploy:artemis-test3
deploy:artemis-test6
and removed
lock:artemis-test3
labels
reschandreas
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
reschandreas
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
reschandreas
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
egekurt123
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
src/main/java/de/tum/in/www1/artemis/service/connectors/aeolus/AeolusTemplateService.java
Show resolved
Hide resolved
WalkthroughThe recent updates to the Artemis service involve a series of refinements across various components. The logging mechanism has been standardized with a renamed logger variable. Initialization now includes a preloading step for scripts, enhancing efficiency. Docker command execution has been fine-tuned, and the handling of programming language features, particularly for Kotlin, has been altered, potentially impacting how Kotlin is supported or processed within the service. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (3)
- src/main/java/de/tum/in/www1/artemis/service/connectors/aeolus/AeolusTemplateService.java (3 hunks)
- src/main/java/de/tum/in/www1/artemis/service/connectors/localci/LocalCIContainerService.java (9 hunks)
- src/main/java/de/tum/in/www1/artemis/service/connectors/localci/LocalCIProgrammingLanguageFeatureService.java (1 hunks)
Additional comments: 3
src/main/java/de/tum/in/www1/artemis/service/connectors/aeolus/AeolusTemplateService.java (1)
- 36-36: The logger variable name has been changed to follow the Java naming conventions for non-final variables. This is a good practice for consistency and readability.
src/main/java/de/tum/in/www1/artemis/service/connectors/localci/LocalCIContainerService.java (2)
31-31: The addition of the
ExecCreateCmdimport suggests that there are new usages of this class in the file. This is a standard change when new functionality is introduced.331-331: The removal of
scriptForJavaKotlinandscriptForPythonmethods indicates a simplification of the script creation process. Ensure that the functionality provided by these methods is either no longer needed or has been appropriately integrated into the new implementation.
| @@ -26,7 +26,7 @@ public LocalCIProgrammingLanguageFeatureService() { | |||
| programmingLanguageFeatures.put(PYTHON, new ProgrammingLanguageFeature(PYTHON, false, false, true, false, false, List.of(), false, false, true)); | |||
| programmingLanguageFeatures.put(C, new ProgrammingLanguageFeature(C, false, true, true, false, false, List.of(FACT, GCC), false, true, true)); | |||
| programmingLanguageFeatures.put(ASSEMBLER, new ProgrammingLanguageFeature(ASSEMBLER, false, false, false, false, false, List.of(), false, true, true)); | |||
| programmingLanguageFeatures.put(KOTLIN, new ProgrammingLanguageFeature(KOTLIN, true, false, true, true, false, List.of(), false, true, true)); | |||
| programmingLanguageFeatures.put(KOTLIN, new ProgrammingLanguageFeature(KOTLIN, false, false, true, true, false, List.of(), false, true, true)); | |||
There was a problem hiding this comment.
The change from true to false for the first parameter in the KOTLIN feature declaration disables the sequential build support for Kotlin. Ensure that this change aligns with the intended functionality and that any dependent features or documentation are updated accordingly.
| private void cacheOnBoot() { | ||
| var resources = this.resourceLoaderService.getResources(Path.of("templates", "aeolus")); | ||
| for (var resource : resources) { | ||
| try { | ||
| String filename = resource.getFilename(); | ||
| if (filename == null) { | ||
| continue; | ||
| } | ||
| String directory = resource.getURL().getPath().split("templates/aeolus/")[1].split("/")[0]; | ||
| String projectType = filename.split("_")[0].replace(".yaml", ""); | ||
| Optional<ProjectType> optionalProjectType = Optional.empty(); | ||
| if (!projectType.equals("default")) { | ||
| optionalProjectType = Optional.of(ProjectType.valueOf(projectType.toUpperCase())); | ||
| } | ||
| String uniqueKey = directory + "_" + filename; | ||
| byte[] fileContent = IOUtils.toByteArray(resource.getInputStream()); | ||
| String script = new String(fileContent, StandardCharsets.UTF_8); | ||
| Windfile windfile = readWindfile(script); | ||
| this.addInstanceVariablesToWindfile(windfile, ProgrammingLanguage.valueOf(directory.toUpperCase()), optionalProjectType); | ||
| templateCache.put(uniqueKey, windfile); | ||
| } | ||
| catch (IOException | IllegalArgumentException e) { | ||
| logger.error("Failed to load windfile {}", resource.getFilename(), e); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new cacheOnBoot method preloads windfiles into the cache during service initialization. This approach can improve performance by avoiding runtime file loading. However, ensure that the error handling is robust and that the application can still function correctly if any windfile fails to load.
| @@ -118,7 +147,7 @@ public Windfile getDefaultWindfileFor(ProgrammingExercise exercise) { | |||
| exercise.hasSequentialTestRuns(), exercise.isTestwiseCoverageEnabled()); | |||
| } | |||
| catch (IOException e) { | |||
| LOGGER.info("No windfile for the settings of exercise {}", exercise.getId(), e); | |||
| logger.info("No windfile for the settings of exercise {}", exercise.getId(), e); | |||
There was a problem hiding this comment.
The logging level in getDefaultWindfileFor has been changed to info when a windfile is not found. This change should be carefully considered; if the absence of a windfile is an expected scenario that does not require immediate attention, info is appropriate. Otherwise, if it is an exceptional situation, a higher severity level might be warranted.
| private List<BuildLogEntry> executeDockerCommand(String containerId, boolean attachStdout, boolean attachStderr, boolean forceRoot, String... command) { | ||
| boolean detach = !attachStdout && !attachStderr; | ||
|
|
||
| ExecCreateCmdResponse execCreateCmdResponse = dockerClient.execCreateCmd(containerId).withAttachStdout(attachStdout).withAttachStderr(attachStderr).withUser("root") | ||
| .withCmd(command).exec(); | ||
| ExecCreateCmd execCreateCmd = dockerClient.execCreateCmd(containerId).withAttachStdout(attachStdout).withAttachStderr(attachStderr).withCmd(command); | ||
| if (forceRoot) { | ||
| execCreateCmd = execCreateCmd.withUser("root"); | ||
| } |
There was a problem hiding this comment.
The executeDockerCommand method has been refactored to include a forceRoot parameter, which allows commands to be executed as the root user. This change should be carefully reviewed to ensure that it does not introduce security risks, especially considering the implications of running commands as root inside Docker containers.
| } | ||
| }); | ||
|
|
||
| // Fall back to hardcoded scripts for old exercises without windfile | ||
| // ***************** | ||
| // TODO: Delete once windfile templates can be used as fallbacks | ||
| if (actions.isEmpty()) { | ||
| // Windfile actions are not defined, use default build script | ||
| switch (programmingExercise.getProgrammingLanguage()) { | ||
| case JAVA, KOTLIN -> scriptForJavaKotlin(programmingExercise, buildScript, hasSequentialTestRuns, hasStaticCodeAnalysis); | ||
| case PYTHON -> scriptForPython(buildScript); | ||
| default -> throw new IllegalArgumentException("No build stage setup for programming language " + programmingExercise.getProgrammingLanguage()); | ||
| } | ||
| } | ||
| // ***************** | ||
|
|
||
| try { | ||
| FileUtils.writeStringToFile(buildScriptPath.toFile(), buildScript.toString(), StandardCharsets.UTF_8); | ||
| } |
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [373-418]
The createBuildScript method has been refactored to generate a build script for a programming exercise. It is important to ensure that the script generation logic is secure and that the scripts are stored and executed in a way that does not expose the system to injection attacks or other security vulnerabilities.
Checklist
General
Server
Changes affecting Programming Exercises
Motivation, Description, and Context
The fetching of the windfiles was pretty flaky on TS3 with multiple nodes, this fixes it by moving the retrieval to the startup of Artemis and throwing errors if the resources could not be found. This PR also removes Kotlin sequential for LocalCI as it is not yet fully supported. It also improves LocalCI in that it does not use root for the execution of the script.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Server
Summary by CodeRabbit
Refactor
Chores