Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Development: Cache windfiles on startup #7817

Merged
merged 9 commits into from
Dec 29, 2023
Merged

Development: Cache windfiles on startup #7817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3409e5c
cache windfiles on startup
reschandreas Dec 21, 2023
1c3ec16
disable sequential tests for Kotlin
reschandreas Dec 22, 2023
60479e4
Merge branch 'develop' into bugfix/fix-missing-aeolus-templates
reschandreas Dec 22, 2023
ba7e045
Merge branch 'develop' into bugfix/fix-missing-aeolus-templates
reschandreas Dec 23, 2023
c9f03c4
Merge branch 'develop' into bugfix/fix-missing-aeolus-templates
reschandreas Dec 23, 2023
44cd5cf
Merge branch 'develop' into bugfix/fix-missing-aeolus-templates
reschandreas Dec 23, 2023
ec72db3
Merge branch 'develop' into bugfix/fix-missing-aeolus-templates
reschandreas Dec 26, 2023
ea5a545
Merge branch 'develop' into bugfix/fix-missing-aeolus-templates
reschandreas Dec 29, 2023
fc8b2f7
Merge branch 'develop' into bugfix/fix-missing-aeolus-templates
reschandreas Dec 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 31 additions & 2 deletions src/main/java/de/tum/in/www1/artemis/service/connectors/aeolus/AeolusTemplateService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
@Profile("aeolus | localci")
public class AeolusTemplateService {

private final Logger LOGGER = LoggerFactory.getLogger(AeolusTemplateService.class);
private final Logger logger = LoggerFactory.getLogger(AeolusTemplateService.class);

private final ProgrammingLanguageConfiguration programmingLanguageConfiguration;

Expand All @@ -44,6 +44,35 @@ public class AeolusTemplateService {
public AeolusTemplateService(ProgrammingLanguageConfiguration programmingLanguageConfiguration, ResourceLoaderService resourceLoaderService) {
this.programmingLanguageConfiguration = programmingLanguageConfiguration;
this.resourceLoaderService = resourceLoaderService;
// load all scripts into the cache
cacheOnBoot();
}

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);
}
bassner marked this conversation as resolved.
Show resolved Hide resolved
}
Comment on lines +51 to +75
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
Expand Down Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
return null;
}
Expand Down
101 changes: 12 additions & 89 deletions src/main/java/de/tum/in/www1/artemis/service/connectors/localci/LocalCIContainerService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.github.dockerjava.api.DockerClient;
import com.github.dockerjava.api.async.ResultCallback;
import com.github.dockerjava.api.command.CreateContainerResponse;
import com.github.dockerjava.api.command.ExecCreateCmd;
import com.github.dockerjava.api.command.ExecCreateCmdResponse;
import com.github.dockerjava.api.exception.NotFoundException;
import com.github.dockerjava.api.model.Container;
Expand All @@ -37,7 +38,6 @@
import de.tum.in.www1.artemis.domain.BuildLogEntry;
import de.tum.in.www1.artemis.domain.ProgrammingExercise;
import de.tum.in.www1.artemis.domain.enumeration.ProgrammingLanguage;
import de.tum.in.www1.artemis.domain.enumeration.ProjectType;
import de.tum.in.www1.artemis.domain.participation.ProgrammingExerciseParticipation;
import de.tum.in.www1.artemis.exception.LocalCIException;
import de.tum.in.www1.artemis.service.connectors.aeolus.AeolusTemplateService;
Expand Down Expand Up @@ -131,7 +131,7 @@ public List<BuildLogEntry> runScriptInContainer(String containerId) {
// container's
// main process. The execution command can run concurrently with the main process. This setup with the ExecCreateCmdResponse gives us the ability to wait in code until the
// command has finished before trying to extract the results.
return executeDockerCommand(containerId, true, true, "sh", WORKING_DIRECTORY + "/script.sh");
return executeDockerCommand(containerId, true, true, false, "bash", WORKING_DIRECTORY + "/script.sh");
}

/**
Expand Down Expand Up @@ -235,7 +235,7 @@ public void populateBuildJobContainer(String buildJobContainerId, Path assignmen

if (!Objects.equals(testCheckoutPath, "")) {
addDirectory(buildJobContainerId, WORKING_DIRECTORY + "/testing-dir", true);
executeDockerCommand(buildJobContainerId, false, false, "chmod", "-R", "777", WORKING_DIRECTORY + "/testing-dir");
executeDockerCommand(buildJobContainerId, false, false, true, "chmod", "-R", "777", WORKING_DIRECTORY + "/testing-dir");
}
addAndPrepareDirectory(buildJobContainerId, testRepositoryPath, WORKING_DIRECTORY + "/testing-dir/" + testCheckoutPath);
addAndPrepareDirectory(buildJobContainerId, assignmentRepositoryPath, WORKING_DIRECTORY + "/testing-dir/" + assignmentCheckoutPath);
Expand All @@ -258,16 +258,16 @@ private void addAndPrepareDirectory(String containerId, Path repositoryPath, Str
}

private void renameDirectoryOrFile(String containerId, String oldName, String newName) {
executeDockerCommand(containerId, false, false, "mv", oldName, newName);
executeDockerCommand(containerId, false, false, true, "mv", oldName, newName);
}

private void addDirectory(String containerId, String directoryName, boolean createParentsIfNecessary) {
String[] command = createParentsIfNecessary ? new String[] { "mkdir", "-p", directoryName } : new String[] { "mkdir", directoryName };
executeDockerCommand(containerId, false, false, command);
executeDockerCommand(containerId, false, false, true, command);
}

private void convertDosFilesToUnix(String path, String containerId) {
executeDockerCommand(containerId, false, false, "sh", "-c", "find " + path + " -type f ! -path '*/.git/*' -exec sed -i 's/\\r$//' {} \\;");
executeDockerCommand(containerId, false, false, true, "sh", "-c", "find " + path + " -type f ! -path '*/.git/*' -exec sed -i 's/\\r$//' {} \\;");
}

private void copyToContainer(String sourcePath, String containerId) {
Expand Down Expand Up @@ -328,11 +328,14 @@ private void executeDockerCommandWithoutAwaitingResponse(String containerId, Str
dockerClient.execStartCmd(createCmdResponse.getId()).withDetach(true).exec(new ResultCallback.Adapter<>());
}

private List<BuildLogEntry> executeDockerCommand(String containerId, boolean attachStdout, boolean attachStderr, String... command) {
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");
}
Comment on lines +331 to +337
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ExecCreateCmdResponse execCreateCmdResponse = execCreateCmd.exec();
List<BuildLogEntry> buildLogEntries = new ArrayList<>();
final CountDownLatch latch = new CountDownLatch(1);
dockerClient.execStartCmd(execCreateCmdResponse.getId()).withDetach(detach).exec(new ResultCallback.Adapter<>() {
Expand Down Expand Up @@ -369,8 +372,6 @@ public void onComplete() {
*/
public Path createBuildScript(ProgrammingExerciseParticipation participation, String containerName) {
ProgrammingExercise programmingExercise = participation.getProgrammingExercise();
boolean hasSequentialTestRuns = programmingExercise.hasSequentialTestRuns();
boolean hasStaticCodeAnalysis = programmingExercise.isStaticCodeAnalysisEnabled();

List<ScriptAction> actions = List.of();

Expand Down Expand Up @@ -412,19 +413,6 @@ public Path createBuildScript(ProgrammingExerciseParticipation participation, St
}
});

// 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);
}
Comment on lines 413 to 418
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand All @@ -435,71 +423,6 @@ public Path createBuildScript(ProgrammingExerciseParticipation participation, St
return buildScriptPath;
}

private void scriptForJavaKotlin(ProgrammingExercise programmingExercise, StringBuilder buildScript, boolean hasSequentialTestRuns, boolean hasStaticCodeAnalysis) {
boolean isMaven = ProjectType.isMavenProject(programmingExercise.getProjectType());

if (isMaven) {
if (hasSequentialTestRuns) {
buildScript.append("""
cd structural
mvn clean test
if [ $? -eq 0 ]; then
cd ..
cd behavior
mvn clean test
fi
cd ..
""");
}
else {
buildScript.append("""
mvn clean test
""");
if (hasStaticCodeAnalysis) {
buildScript.append("""
mvn checkstyle:checkstyle
mvn pmd:pmd
mvn spotbugs:spotbugs
""");
}
}
}
else {
if (hasSequentialTestRuns) {
buildScript.append("""
chmod +x gradlew
./gradlew clean structuralTests
if [ $? -eq 0 ]; then
./gradlew behaviorTests
fi
""");
}
else {
buildScript.append("""
chmod +x gradlew
./gradlew clean test
""");
if (hasStaticCodeAnalysis) {
buildScript.append("""
./gradlew check -x test
""");
}
}
}
}

private void scriptForPython(StringBuilder buildScript) {
buildScript.append("""
python3 -m compileall . -q || error=true
if [ ! $error ]
then
pytest --junitxml=test-reports/results.xml
else
exit 1
fi
""");
}

/**
* Deletes the build script for a given programming exercise.
* The build script is stored in a file in the local-ci-scripts directory.
Expand Down
2 changes: 1 addition & 1 deletion .../in/www1/artemis/service/connectors/localci/LocalCIProgrammingLanguageFeatureService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

programmingLanguageFeatures.put(VHDL, new ProgrammingLanguageFeature(VHDL, false, false, false, false, false, List.of(), false, true, true));
programmingLanguageFeatures.put(HASKELL, new ProgrammingLanguageFeature(HASKELL, true, false, false, false, true, List.of(), false, true, true));
programmingLanguageFeatures.put(OCAML, new ProgrammingLanguageFeature(OCAML, false, false, false, false, true, List.of(), false, true, true));
Expand Down
Loading