Testbranch (do not merge)#5247
Conversation
Summary of ChangesHello @LAVEEN, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on enhancing the robustness of the CI/CD pipeline, particularly for 'onspot' test configurations. The changes aim to mitigate common issues like resource naming conflicts and false-positive build conflict detections, which can arise during parallel or frequent test deployments. By introducing randomization and unique identifiers for networking resources, and refining the build conflict detection logic, the PR improves the reliability and efficiency of automated testing. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several changes to improve the reliability of integration tests by randomizing network and IP address resources to avoid conflicts between concurrent builds. The changes in check_running_build.sh are good, allowing concurrent 'onspot' builds. The changes in the Ansible playbooks and test configurations effectively introduce this randomization. My review includes a few suggestions to improve maintainability and fix a potential bug in the Ansible playbook. Specifically, I've pointed out a redundant and likely buggy task, a brittle string replacement that could be improved, and an opportunity to reduce repetition in the test configuration file.
| - name: Generate random suffix for networking | ||
| ansible.builtin.set_fact: | ||
| random_id: "{{ query('password', '/dev/null chars=ascii_lowercase,digits length=4') | first }}" | ||
|
|
||
| - name: Generic randomization of networking keys in blueprint | ||
| ansible.builtin.replace: | ||
| path: "{{ workspace }}/{{ blueprint_path }}" | ||
| regexp: '^(\s*)({{ item }}):\s*([^#\n\s]+)(.*)$' | ||
| replace: '\1\2: \3-{{ random_id }}\4' | ||
| loop: | ||
| - network_name | ||
| - subnetwork_name | ||
| - network_name_prefix |
There was a problem hiding this comment.
This block of tasks appears to be incorrect and is also redundant.
- The
replacetask usespath: "{{ workspace }}/{{ blueprint_path }}". The variableblueprint_pathis not defined within the provided context; other tasks correctly useblueprint_yaml. This is likely a bug that would cause the task to fail. - This block's goal of randomizing network names is also handled by the task "Prefix networking resources with Build ID to prevent 409 conflicts" at line 145.
To fix the bug and remove redundancy, I recommend removing this entire block (lines 56-68). If a random suffix is still required, that logic should be merged into the task at line 145.
| - name: Force randomization of networking strings in blueprint | ||
| ansible.builtin.replace: | ||
| path: "{{ blueprint_yaml }}" | ||
| # Matches any occurrence of the conflicting network name string | ||
| regexp: 'a3u-onspot-slurm-net' | ||
| # Replaces it with the prefixed version | ||
| replace: '{{ build }}-a3u-onspot-slurm-net' |
There was a problem hiding this comment.
This task uses a global replacement for the hardcoded string a3u-onspot-slurm-net. This is a brittle approach that can be difficult to maintain, as it might have unintended side effects if the string appears in other contexts (e.g., comments, or as part of a larger name).
For better long-term maintainability, it would be best to identify where this string originates in the blueprint and parameterize it properly. This would make the configuration more explicit and robust.
| deployment_name: "a3u-onspot-slurm-{{ build[0:4] }}" | ||
| slurm_cluster_name: "a3usp{{ build[0:4] }}" | ||
| workspace: /workspace | ||
| blueprint_yaml: "{{ workspace }}/examples/machine-learning/a3-ultragpu-8g/a3ultra-slurm-blueprint.yaml" | ||
| login_node: "{{ slurm_cluster_name }}-slurm-login-*" | ||
| controller_node: "{{ slurm_cluster_name }}-controller" | ||
| network: "{{ test_name }}-net-0" | ||
| network: "a3u-{{ build[0:4] }}-net-0" |
There was a problem hiding this comment.
The string slice [0:4] is used multiple times across this file (here and on line 54) to create a shortened build ID. This use of a "magic number" and repeated slicing makes the code harder to read and maintain.
To improve this, I suggest defining a variable at the top of the file, like short_build_id: "{{ build[0:4] }}", and then reusing {{ short_build_id }} wherever the shortened ID is needed. This would make the code DRY (Don't Repeat Yourself) and make it easier to change the length of the short ID in the future.
1 participant
Submission Checklist
NOTE: Community submissions can take up to 2 weeks to be reviewed.
Please take the following actions before submitting this pull request.