Integrated code lifecycle: Allow instructor to set Docker Performance flags for programming exercises
#10356
Conversation
…ze-docker-flags # Conflicts: # src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseService.java
github-actions
bot
added
tests
server
WalkthroughThis pull request expands Docker container configuration by adding resource parameters for CPU and memory management. It introduces new fields ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
⏰ Context from checks skipped due to timeout of 90000ms (9)
🔇 Additional comments (3)
✨ Finishing Touches
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (2)
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIInfoContributorTest.java (1)
14-32: 🛠️ Refactor suggestionAdd test coverage for Docker performance flags.
The test only verifies timeout settings but doesn't cover the newly added Docker performance flags (CPU count, memory, memory swap). Consider adding test cases to verify these configurations are correctly contributed to the Info object.
@Test void testContribute() { Info.Builder builder = new Info.Builder(); LocalCIInfoContributor localCIInfoContributor = new LocalCIInfoContributor(new ProgrammingLanguageConfiguration()); ReflectionTestUtils.setField(localCIInfoContributor, "minInstructorBuildTimeoutOption", 10); ReflectionTestUtils.setField(localCIInfoContributor, "maxInstructorBuildTimeoutOption", 240); ReflectionTestUtils.setField(localCIInfoContributor, "defaultInstructorBuildTimeoutOption", 120); try { localCIInfoContributor.contribute(builder); } catch (NullPointerException e) { } Info info = builder.build(); assertThat(info.getDetails().get("buildTimeoutMin")).isEqualTo(10); assertThat(info.getDetails().get("buildTimeoutMax")).isEqualTo(240); assertThat(info.getDetails().get("buildTimeoutDefault")).isEqualTo(120); + // Verify Docker performance flags + assertThat(info.getDetails().get("defaultContainerCpuCount")).isNotNull(); + assertThat(info.getDetails().get("defaultContainerMemoryLimitInMB")).isNotNull(); + assertThat(info.getDetails().get("defaultContainerMemorySwapLimitInMB")).isNotNull(); }src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)
92-128: 🛠️ Refactor suggestionAdd validation for resource limits.
The method accepts resource values without validation, which could lead to container creation failures if values exceed system limits.
Add validation for CPU and memory values:
public CreateContainerResponse configureContainer(String containerName, String image, String buildScript, List<String> exerciseEnvVars, int cpuCount, int memory, int memorySwap) { + if (cpuCount < 0) { + throw new IllegalArgumentException("CPU count cannot be negative"); + } + if (memory < 0) { + throw new IllegalArgumentException("Memory limit cannot be negative"); + } + if (memorySwap < memory && memorySwap != 0) { + throw new IllegalArgumentException("Memory swap must be greater than or equal to memory limit"); + }
🧹 Nitpick comments (7)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIInfoContributor.java (1)
60-73: Parsing logic may yield unexpected results for fractional or uppercase values.
- Suffix recognition is limited to
g",m",k". Users might provide memory strings without trailing quotes or with uppercase letters (e.g., "1G").- Converting from kilobytes to MB via integer division could round down to 0 for small values.
- No fallback for unexpected or malformed strings beyond a
NumberFormatException.Consider extending detection to handle uppercase suffixes, fractional values, or re-check the usage context to ensure the input format is strictly validated.
private static long parseMemoryString(String memoryString) { - if (memoryString.endsWith("g\"")) { - return Long.parseLong(memoryString.replaceAll("[^0-9]", "")) * 1024L; - } - else if (memoryString.endsWith("m\"")) { - return Long.parseLong(memoryString.replaceAll("[^0-9]", "")); - } - else if (memoryString.endsWith("k\"")) { - return Long.parseLong(memoryString.replaceAll("[^0-9]", "")) / 1024L; - } - else { - return Long.parseLong(memoryString); - } + String lower = memoryString.toLowerCase().replace("\"", ""); + // Example approach: parse with pattern "(\d+)(g|m|k)" + if (lower.endsWith("g")) { + return Long.parseLong(lower.replaceAll("[^0-9]", "")) * 1024L; + } else if (lower.endsWith("m")) { + return Long.parseLong(lower.replaceAll("[^0-9]", "")); + } else if (lower.endsWith("k")) { + return Long.parseLong(lower.replaceAll("[^0-9]", "")) / 1024L; + } else { + // Potential fallback or error + return Long.parseLong(lower); + } }src/main/java/de/tum/cit/aet/artemis/buildagent/dto/DockerFlagsDTO.java (1)
5-5: Consider using long or double for memory fields.
Storing memory in MB asintcan cause overflow if values exceed ~2GB, and fractional CPU count is not possible withint. Although it may be sufficient for your use case, consider a more flexible numeric type (e.g.,longfor memory,doublefor CPU).src/main/java/de/tum/cit/aet/artemis/buildagent/dto/DockerRunConfig.java (1)
6-6: Check potential overflow for memory fields.
Same reasoning: if an instructor sets memory over 2GB, usingintmight be limiting. Ensure that your usage scenario aligns with these constraints; otherwise, considerlong.src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.ts (1)
172-172: Consider using nullish coalescing for Docker flags.When building the Docker flags object, consider using nullish coalescing to avoid including undefined values.
- this.dockerFlags = { network: this.isNetworkDisabled ? 'none' : undefined, env: newEnv, cpuCount: this.cpuCount, memory: this.memory, memorySwap: this.memorySwap }; + this.dockerFlags = { + ...(this.isNetworkDisabled && { network: 'none' }), + ...(Object.keys(newEnv ?? {}).length > 0 && { env: newEnv }), + ...(this.cpuCount && { cpuCount: this.cpuCount }), + ...(this.memory && { memory: this.memory }), + ...(this.memorySwap && { memorySwap: this.memorySwap }) + };src/test/javascript/spec/component/programming-exercise/programming-exercise-build-configuration.component.spec.ts (1)
52-102: Enhance test coverage for profile values.The test verifies the initialization of profile values but doesn't test edge cases for CPU and memory settings.
Consider adding test cases for:
- Zero or negative values for CPU and memory
- Maximum allowed values
- Invalid values (e.g., non-numeric)
it('should set profile values', () => { + // Test invalid values + profileServiceMock.getProfileInfo.mockReturnValue( + of({ + defaultContainerCpuCount: -1, + defaultContainerMemoryLimitInMB: 'invalid', + defaultContainerMemorySwapLimitInMB: Number.MAX_SAFE_INTEGER + 1, + }), + ); + comp.ngOnInit(); + expect(comp.cpuCount).toBeUndefined(); + expect(comp.memory).toBeUndefined(); + expect(comp.memorySwap).toBeUndefined();src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)
130-134: Consider adding memory reservation.The host config could benefit from memory reservation settings to prevent container OOM issues.
Add memory reservation to the host config:
private HostConfig copyAndAdjustHostConfig(long cpuCount, long memory, long memorySwap) { long cpuPeriod = hostConfig.getCpuPeriod(); - return HostConfig.newHostConfig().withCpuQuota(cpuCount * cpuPeriod).withCpuPeriod(cpuPeriod).withMemory(memory).withMemorySwap(memorySwap) + return HostConfig.newHostConfig() + .withCpuQuota(cpuCount * cpuPeriod) + .withCpuPeriod(cpuPeriod) + .withMemory(memory) + .withMemorySwap(memorySwap) + .withMemoryReservation(memory / 2) // Set soft limit to half of hard limit .withPidsLimit(hostConfig.getPidsLimit()) .withAutoRemove(true); }src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.html (1)
95-117: Consider using numeric input types for better user experience.While the UI structure is good, consider these improvements:
- Use
type="number"for numeric inputs- Add
minandmaxattributes to prevent invalid values- Add placeholders to indicate expected units (e.g., "MB" for memory)
Apply this diff to improve the input fields:
-<input type="text" class="form-control" name="cpuCount" id="field_dockerCpuCount" [value]="cpuCount" (input)="onCpuCountChange($event)" /> +<input type="number" class="form-control" name="cpuCount" id="field_dockerCpuCount" [value]="cpuCount" (input)="onCpuCountChange($event)" min="1" placeholder="Number of CPUs" /> -<input type="text" class="form-control" name="memory" id="field_memory" [value]="memory" (input)="onMemoryChange($event)" /> +<input type="number" class="form-control" name="memory" id="field_memory" [value]="memory" (input)="onMemoryChange($event)" min="6" placeholder="Memory in MB" /> -<input type="text" class="form-control" name="memorySwap" id="field_memorySwap" [value]="memorySwap" (input)="onMemorySwapChange($event)" /> +<input type="number" class="form-control" name="memorySwap" id="field_memorySwap" [value]="memorySwap" (input)="onMemorySwapChange($event)" min="0" placeholder="Memory swap in MB" />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/DockerFlagsDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/dto/DockerRunConfig.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseBuildConfigService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIInfoContributor.java(3 hunks)src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.html(1 hunks)src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.ts(6 hunks)src/main/webapp/app/shared/layouts/profiles/profile-info.model.ts(1 hunks)src/main/webapp/app/shared/layouts/profiles/profile.service.ts(1 hunks)src/main/webapp/i18n/de/error.json(1 hunks)src/main/webapp/i18n/de/programmingExercise.json(2 hunks)src/main/webapp/i18n/en/error.json(1 hunks)src/main/webapp/i18n/en/programmingExercise.json(2 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIInfoContributorTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.java(1 hunks)src/test/javascript/spec/component/programming-exercise/programming-exercise-build-configuration.component.spec.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/buildagent/dto/DockerFlagsDTO.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/dto/DockerRunConfig.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIInfoContributor.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseBuildConfigService.javasrc/main/java/de/tum/cit/aet/artemis/core/config/Constants.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseService.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIInfoContributorTest.javasrc/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/shared/layouts/profiles/profile-info.model.tssrc/main/webapp/app/shared/layouts/profiles/profile.service.tssrc/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.ts
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.html
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/programming-exercise/programming-exercise-build-configuration.component.spec.ts
`src/main/webapp/i18n/de/**/*.json`: German language transla...
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/programmingExercise.jsonsrc/main/webapp/i18n/de/error.json
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: client-tests-selected
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: client-tests
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: client-style
- GitHub Check: Analyse
🔇 Additional comments (19)
src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.java (1)
226-226: LGTM! Good improvement to test flexibility.Making
dockerClientMockprotected allows subclasses to customize Docker client behavior for specific test scenarios while maintaining the base mocking functionality. This change enhances the reusability of the test infrastructure.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIInfoContributor.java (5)
5-6: Imports look good.
No issues here. Everything follows the Java conventions, and there's no star import.
14-14: Good choice of direct import.
ImportingProgrammingLanguageConfigurationdirectly maintains clarity and avoids wildcard imports.
29-30: Field addition aligns with constructor injection.
Using a final field with constructor injection promotes immutability and complies with best practices in dependency injection.
31-33: Constructor injection is properly implemented.
This approach adheres to the recommended DI patterns for Java. No changes needed.
45-46: Validate possible null or empty lists.
AlthoughgetDefaultDockerFlags()may reliably return a non-null list, consider defensive checks or at least handle the possibility of an empty list gracefully to avoid unexpected exceptions.src/main/webapp/app/shared/layouts/profiles/profile-info.model.ts (1)
38-40: LGTM!The new Docker performance flag properties are well-typed as optional numbers and follow consistent naming conventions.
src/test/javascript/spec/component/programming-exercise/programming-exercise-build-configuration.component.spec.ts (2)
127-134: LGTM! Docker flag tests are comprehensive.The test cases for Docker flags cover the new performance settings appropriately, including network flags and environment variables.
152-160: LGTM! Language support tests are accurate.The test cases for language support are well-structured and verify both unsupported and supported languages.
src/main/webapp/app/shared/layouts/profiles/profile.service.ts (1)
91-94: LGTM! Profile properties are correctly added.The new container configuration properties are added following the established pattern and maintain consistency with the existing code.
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
290-295: LGTM! Docker flag constants are well-defined.The new constants follow the established naming convention and are appropriately documented through their descriptive names.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java (1)
240-242: LGTM! Docker resource configuration is properly implemented.The changes correctly initialize and handle Docker container resource limits (CPU, memory, memory swap) from the configuration and pass them to the container setup.
Also applies to: 246-248, 251-252
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java (1)
631-647: LGTM! Comprehensive test coverage for Docker performance flags.The test thoroughly verifies:
- Correct setting of Docker performance flags
- Proper conversion of values (e.g., MB to bytes for memory limits)
- Container creation with expected resource configurations
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseService.java (1)
135-136: LGTM! Robust validation for Docker resource limits.The implementation includes comprehensive validation checks with clear error messages:
- Memory limit validation against Docker's minimum requirement (6MB)
- CPU count validation (must be positive)
- Memory swap validation (must be non-negative)
Also applies to: 1119-1129
src/main/webapp/i18n/en/error.json (1)
101-103: New Error Messages for Docker Resources Added
The three new error messages for validating memory limits, CPU count, and memory swap are clearly and consistently defined. They provide clear guidance on the expected numerical constraints.src/main/webapp/i18n/de/error.json (1)
101-103: Neue Fehlermeldungen zur Validierung von Docker-Ressourcen
Die neuen Fehlertexte für „memoryLimitInvalid“, „cpuCountInvalid“ und „memorySwapLimitInvalid“ sind passend formuliert und im informellen Stil gehalten. Sie passen gut zu den restlichen Übersetzungen im File.src/main/webapp/i18n/en/programmingExercise.json (2)
123-124: Terminology Update: Rename Build Plan Customization
The keys “customizeBuildPlan” and “customizeBuildPlanWithAeolus” have been updated to “Customize Build Configuration” to reflect a broader scope than just scripting. This enhances clarity for the user.
746-757: New Docker Resource Configuration Options Added
The new entries for “cpuCount”, “memory”, and “memorySwap” under the dockerFlags section are well defined and provide clear labels and help texts for users. They align nicely with the backend changes for Docker resource management.src/main/webapp/i18n/de/programmingExercise.json (1)
113-114: Terminology Update: Anpassung der Build-Konfiguration
Die Übersetzung der Keys „customizeBuildPlan“ und „customizeBuildPlanWithAeolus“ wurde in „Build Konfiguration anpassen“ geändert. Dies entspricht der neuen, erweiterte Bedeutung und sorgt für mehr Klarheit in der Benutzerführung.
| switch (flag) { | ||
| case "--cpus" -> builder.withDetail(Constants.DOCKER_FLAG_CPUS, Long.parseLong(value.replaceAll("[^0-9]", ""))); | ||
| case "--memory" -> builder.withDetail(Constants.DOCKER_FLAG_MEMORY, parseMemoryString(value)); | ||
| case "--memory-swap" -> builder.withDetail(Constants.DOCKER_FLAG_MEMORY_SWAP, parseMemoryString(value)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential silent failures for unexpected flags.
This switch handles three known flags but silently ignores others. Consider adding a default case (e.g., logging unrecognized flags) or validating the entire set to avoid missing new flags unintentionally.
switch (flag) {
case "--cpus" -> builder.withDetail(Constants.DOCKER_FLAG_CPUS, Long.parseLong(value.replaceAll("[^0-9]", "")));
case "--memory" -> builder.withDetail(Constants.DOCKER_FLAG_MEMORY, parseMemoryString(value));
case "--memory-swap" -> builder.withDetail(Constants.DOCKER_FLAG_MEMORY_SWAP, parseMemoryString(value));
+ default -> {
+ // Potentially log or handle additional flags, e.g.:
+ builder.withDetail("unrecognized-docker-flag-" + flag, value);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (flag) { | |
| case "--cpus" -> builder.withDetail(Constants.DOCKER_FLAG_CPUS, Long.parseLong(value.replaceAll("[^0-9]", ""))); | |
| case "--memory" -> builder.withDetail(Constants.DOCKER_FLAG_MEMORY, parseMemoryString(value)); | |
| case "--memory-swap" -> builder.withDetail(Constants.DOCKER_FLAG_MEMORY_SWAP, parseMemoryString(value)); | |
| } | |
| } | |
| switch (flag) { | |
| case "--cpus" -> builder.withDetail(Constants.DOCKER_FLAG_CPUS, Long.parseLong(value.replaceAll("[^0-9]", ""))); | |
| case "--memory" -> builder.withDetail(Constants.DOCKER_FLAG_MEMORY, parseMemoryString(value)); | |
| case "--memory-swap" -> builder.withDetail(Constants.DOCKER_FLAG_MEMORY_SWAP, parseMemoryString(value)); | |
| default -> { | |
| // Potentially log or handle additional flags, e.g.: | |
| builder.withDetail("unrecognized-docker-flag-" + flag, value); | |
| } | |
| } | |
| } |
| for (int i = 0; i < defaultDockerFlags.size(); i += 2) { | ||
| String flag = defaultDockerFlags.get(i); | ||
| String value = defaultDockerFlags.get(i + 1); | ||
|
|
There was a problem hiding this comment.
Potential out-of-bounds if the number of flags is odd.
The loop increments by 2, assuming pairs. If defaultDockerFlags.size() is not an even number, calling get(i + 1) could throw an IndexOutOfBoundsException. Ensure the upstream logic guarantees pairs or add a guard check.
for (int i = 0; i < defaultDockerFlags.size(); i += 2) {
+ if (i + 1 >= defaultDockerFlags.size()) {
+ // Handle error or break gracefully
+ break;
+ }
String flag = defaultDockerFlags.get(i);
String value = defaultDockerFlags.get(i + 1);
...
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (int i = 0; i < defaultDockerFlags.size(); i += 2) { | |
| String flag = defaultDockerFlags.get(i); | |
| String value = defaultDockerFlags.get(i + 1); | |
| for (int i = 0; i < defaultDockerFlags.size(); i += 2) { | |
| if (i + 1 >= defaultDockerFlags.size()) { | |
| // Handle error or break gracefully | |
| break; | |
| } | |
| String flag = defaultDockerFlags.get(i); | |
| String value = defaultDockerFlags.get(i + 1); | |
| ... | |
| } |
| int cpuCount = 0; | ||
| int memory = 0; | ||
| int memorySwap = 0; |
There was a problem hiding this comment.
Add validation for Docker performance flags.
The CPU count and memory values should be validated to ensure they are within reasonable bounds. Consider adding checks for:
- CPU count > 0
- Memory limit > 0 and within system constraints
- Memory swap ≥ memory limit
- int cpuCount = 0;
- int memory = 0;
- int memorySwap = 0;
+ int cpuCount = 0;
+ int memory = 0;
+ int memorySwap = 0;
+ final int MIN_MEMORY_MB = 128;
+ final int MAX_MEMORY_MB = 4096;
+ final int MAX_CPU_COUNT = 4;And in the createDockerRunConfig method:
DockerRunConfig createDockerRunConfig(String network, Map<String, String> environmentMap, int cpuCount, int memory, int memorySwap) {
+ // Validate CPU count
+ if (cpuCount < 0 || cpuCount > MAX_CPU_COUNT) {
+ throw new IllegalArgumentException("CPU count must be between 0 and " + MAX_CPU_COUNT);
+ }
+ // Validate memory limits
+ if (memory > 0 && (memory < MIN_MEMORY_MB || memory > MAX_MEMORY_MB)) {
+ throw new IllegalArgumentException("Memory must be between " + MIN_MEMORY_MB + "MB and " + MAX_MEMORY_MB + "MB");
+ }
+ if (memorySwap > 0 && memorySwap < memory) {
+ throw new IllegalArgumentException("Memory swap must be greater than or equal to memory limit");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int cpuCount = 0; | |
| int memory = 0; | |
| int memorySwap = 0; | |
| int cpuCount = 0; | |
| int memory = 0; | |
| int memorySwap = 0; | |
| final int MIN_MEMORY_MB = 128; | |
| final int MAX_MEMORY_MB = 4096; | |
| final int MAX_CPU_COUNT = 4; | |
| // ... other class code ... | |
| DockerRunConfig createDockerRunConfig(String network, Map<String, String> environmentMap, int cpuCount, int memory, int memorySwap) { | |
| // Validate CPU count | |
| if (cpuCount < 0 || cpuCount > MAX_CPU_COUNT) { | |
| throw new IllegalArgumentException("CPU count must be between 0 and " + MAX_CPU_COUNT); | |
| } | |
| // Validate memory limits | |
| if (memory > 0 && (memory < MIN_MEMORY_MB || memory > MAX_MEMORY_MB)) { | |
| throw new IllegalArgumentException("Memory must be between " + MIN_MEMORY_MB + "MB and " + MAX_MEMORY_MB + "MB"); | |
| } | |
| if (memorySwap > 0 && memorySwap < memory) { | |
| throw new IllegalArgumentException("Memory swap must be greater than or equal to memory limit"); | |
| } | |
| // ... rest of the implementation ... | |
| } |
| onMemoryChange(event: any) { | ||
| this.memory = event.target.value; | ||
| this.parseDockerFlagsToString(); | ||
| } |
There was a problem hiding this comment.
Add validation and type safety to memory limit changes.
The event handler directly assigns the value without validation or type conversion. Consider adding validation and proper type handling.
- onMemoryChange(event: any) {
- this.memory = event.target.value;
+ onMemoryChange(event: any) {
+ const value = parseInt(event.target.value, 10);
+ if (isNaN(value) || value < 128 || value > 4096) {
+ return;
+ }
+ this.memory = value;
this.parseDockerFlagsToString();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onMemoryChange(event: any) { | |
| this.memory = event.target.value; | |
| this.parseDockerFlagsToString(); | |
| } | |
| onMemoryChange(event: any) { | |
| const value = parseInt(event.target.value, 10); | |
| if (isNaN(value) || value < 128 || value > 4096) { | |
| return; | |
| } | |
| this.memory = value; | |
| this.parseDockerFlagsToString(); | |
| } |
| onMemorySwapChange(event: any) { | ||
| this.memorySwap = event.target.value; | ||
| this.parseDockerFlagsToString(); | ||
| } |
There was a problem hiding this comment.
Add validation and type safety to memory swap changes.
The event handler directly assigns the value without validation or type conversion. Consider adding validation and proper type handling.
- onMemorySwapChange(event: any) {
- this.memorySwap = event.target.value;
+ onMemorySwapChange(event: any) {
+ const value = parseInt(event.target.value, 10);
+ if (isNaN(value) || value < this.memory!) {
+ return;
+ }
+ this.memorySwap = value;
this.parseDockerFlagsToString();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onMemorySwapChange(event: any) { | |
| this.memorySwap = event.target.value; | |
| this.parseDockerFlagsToString(); | |
| } | |
| onMemorySwapChange(event: any) { | |
| const value = parseInt(event.target.value, 10); | |
| if (isNaN(value) || value < this.memory!) { | |
| return; | |
| } | |
| this.memorySwap = value; | |
| this.parseDockerFlagsToString(); | |
| } |
| onCpuCountChange(event: any) { | ||
| this.cpuCount = event.target.value; | ||
| this.parseDockerFlagsToString(); | ||
| } |
There was a problem hiding this comment.
Add validation and type safety to CPU count changes.
The event handler directly assigns the value without validation or type conversion. Consider adding validation and proper type handling.
- onCpuCountChange(event: any) {
- this.cpuCount = event.target.value;
+ onCpuCountChange(event: any) {
+ const value = parseInt(event.target.value, 10);
+ if (isNaN(value) || value < 0 || value > 4) {
+ return;
+ }
+ this.cpuCount = value;
this.parseDockerFlagsToString();
}| private long convertMemorFromMBToBytes(long memory) { | ||
| return memory * 1024L * 1024L; | ||
| } |
There was a problem hiding this comment.
Add overflow check for memory conversion.
The memory conversion could overflow for large values.
Add overflow check:
private long convertMemorFromMBToBytes(long memory) {
+ if (memory > Long.MAX_VALUE / (1024L * 1024L)) {
+ throw new IllegalArgumentException("Memory value too large: would cause overflow");
+ }
return memory * 1024L * 1024L;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private long convertMemorFromMBToBytes(long memory) { | |
| return memory * 1024L * 1024L; | |
| } | |
| private long convertMemorFromMBToBytes(long memory) { | |
| if (memory > Long.MAX_VALUE / (1024L * 1024L)) { | |
| throw new IllegalArgumentException("Memory value too large: would cause overflow"); | |
| } | |
| return memory * 1024L * 1024L; | |
| } |
| "cpuCount": { | ||
| "label": "Anzahl der CPUs bearbeiten", | ||
| "help": "Die Anzahl der CPUs, die der Docker-Container verwenden kann. Weitere Informationen finden Sie in der Docker-Dokumentation." | ||
| }, | ||
| "memory": { | ||
| "label": "Maximale Arbeitsspeichergröße bearbeiten (in MB)", | ||
| "help": "Die maximale Größe des Arbeitsspeichers, die der Docker-Container verwenden kann. Weitere Informationen finden Sie in der Docker-Dokumentation." | ||
| }, | ||
| "memorySwap": { | ||
| "label": "Maximale Swap-Speichergröße bearbeiten (in MB)", | ||
| "help": "Die maximale Größe des Swap-Speichers, die der Docker-Container verwenden kann. Weitere Informationen finden Sie in der Docker-Dokumentation." |
There was a problem hiding this comment.
🛠️ Refactor suggestion
New Docker Resource Configuration Options – Language Consistency
The new entries for “cpuCount”, “memory”, and “memorySwap” are correctly added; however, note that the help texts use formal language (“Weitere Informationen finden Sie …”). Per our guidelines, the German translations should be informal. Consider changing “Sie” to “du”. For example, you could update as follows:
- "cpuCount": {
- "label": "Anzahl der CPUs bearbeiten",
- "help": "Die Anzahl der CPUs, die der Docker-Container verwenden kann. Weitere Informationen finden Sie in der Docker-Dokumentation."
- },
+ "cpuCount": {
+ "label": "Anzahl der CPUs bearbeiten",
+ "help": "Die Anzahl der CPUs, die der Docker-Container verwenden kann. Weitere Informationen findest du in der Docker-Dokumentation."
+ },Apply similar changes for “memory” and “memorySwap”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "cpuCount": { | |
| "label": "Anzahl der CPUs bearbeiten", | |
| "help": "Die Anzahl der CPUs, die der Docker-Container verwenden kann. Weitere Informationen finden Sie in der Docker-Dokumentation." | |
| }, | |
| "memory": { | |
| "label": "Maximale Arbeitsspeichergröße bearbeiten (in MB)", | |
| "help": "Die maximale Größe des Arbeitsspeichers, die der Docker-Container verwenden kann. Weitere Informationen finden Sie in der Docker-Dokumentation." | |
| }, | |
| "memorySwap": { | |
| "label": "Maximale Swap-Speichergröße bearbeiten (in MB)", | |
| "help": "Die maximale Größe des Swap-Speichers, die der Docker-Container verwenden kann. Weitere Informationen finden Sie in der Docker-Dokumentation." | |
| "cpuCount": { | |
| "label": "Anzahl der CPUs bearbeiten", | |
| "help": "Die Anzahl der CPUs, die der Docker-Container verwenden kann. Weitere Informationen findest du in der Docker-Dokumentation." | |
| }, | |
| "memory": { | |
| "label": "Maximale Arbeitsspeichergröße bearbeiten (in MB)", | |
| "help": "Die maximale Größe des Arbeitsspeichers, die der Docker-Container verwenden kann. Weitere Informationen findest du in der Docker-Dokumentation." | |
| }, | |
| "memorySwap": { | |
| "label": "Maximale Swap-Speichergröße bearbeiten (in MB)", | |
| "help": "Die maximale Größe des Swap-Speichers, die der Docker-Container verwenden kann. Weitere Informationen findest du in der Docker-Dokumentation." | |
| } |
| if (cpuCount > 0 || memory > 0 || memorySwap > 0) { | ||
| long adjustedCpuCount = (cpuCount > 0) ? cpuCount : hostConfig.getCpuCount(); | ||
| long adjustedMemory = (memory > 0) ? convertMemorFromMBToBytes(memory) : hostConfig.getMemory(); | ||
| long adjustedMemorySwap = (memorySwap > 0) ? convertMemorFromMBToBytes(memorySwap) : hostConfig.getMemorySwap(); |
There was a problem hiding this comment.
| long adjustedMemorySwap = (memorySwap > 0) ? convertMemorFromMBToBytes(memorySwap) : hostConfig.getMemorySwap(); | |
| long adjustedMemorySwap = (memorySwap > 0) ? convertMemoryFromMBToBytes(memorySwap) : hostConfig.getMemorySwap(); |
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Motivation and Context
We want instructors to be able to change CPU count, memory, and memory swap.
Description
We expanded the corresponding DTOs and added the necessary UI. We also changed the info contributor to include the default values so that they are displayed in the UI.
Steps for Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit