update ansible prepare server script #6
Conversation
Signed-off-by: Sebastian Sch <[email protected]>
Summary of ChangesHello @SchSeba, 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 updates the Ansible server preparation script to improve Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 updates an Ansible playbook and adds several Kubernetes configuration files. My review has identified a few issues that need attention:
In prepare-node-virtual-cluster/prepare-server.yaml, a new task to create a symlink for kubectl uses overly permissive 0777 permissions, which is a security concern.
In sriov-configs/test17.yaml, there is a critical YAML syntax error in the ipam configuration that will prevent it from being parsed. Additionally, the file contains hardcoded node hostnames, which is not ideal for maintainability.
In pods/tool-box-pdb.yaml, the new PodDisruptionBudget has a label selector that likely doesn't match the intended toolbox deployment's pods.
Please see my detailed comments for suggestions on how to address these points.
| ipam: '{"type": "host-local","ranges": [[{"subnet": "10.0.1.0/24"}]],"dataDir": | ||
| "/run/my-orchestrator/container-ipam-state-1"}' |
There was a problem hiding this comment.
The ipam configuration is specified as a multi-line single-quoted string, which is invalid YAML syntax and will cause parsing errors. The string should be on a single line, or a YAML block scalar should be used for better readability.
ipam: '{"type": "host-local","ranges": [[{"subnet": "10.0.1.0/24"}]],"dataDir": "/run/my-orchestrator/container-ipam-state-1"}'| minAvailable: 1 | ||
| selector: | ||
| matchLabels: | ||
| app: dpdk |
There was a problem hiding this comment.
The selector app: dpdk in this PodDisruptionBudget does not match the labels of the pods created by the toolbox deployment (defined in pods/deployment-toolbox.yaml), which are labeled with app: toolbox. Given the PDB's name is toolbox-pdb, it seems intended for that deployment. To fix this, the selector should be updated to match the deployment's pod labels.
app: toolbox| - name: Create symbolic link for kubectl binary | ||
| file: | ||
| src: /usr/local/bin/kubectl | ||
| dest: /bin/kubectl | ||
| state: link | ||
| mode: '0777' |
There was a problem hiding this comment.
This task creates a symbolic link for kubectl with overly permissive 0777 permissions, which is a security risk as it makes the link world-writable. Additionally, creating symlinks in /bin is less common; /usr/bin is a more standard location and is consistent with how the Go binary is handled earlier in this playbook.
I suggest changing the destination to /usr/bin/kubectl and removing the mode parameter to use secure default permissions.
- name: Create symbolic link for kubectl binary
file:
src: /usr/local/bin/kubectl
dest: /usr/bin/kubectl
state: link| nodeSelector: | ||
| kubernetes.io/hostname: ocp-virt-worker-0.lab | ||
| numVfs: 5 | ||
| priority: 99 | ||
| resourceName: sriov_nic_1 | ||
| mtu: 1500 | ||
| --- | ||
| apiVersion: sriovnetwork.openshift.io/v1 | ||
| kind: SriovNetworkNodePolicy | ||
| metadata: | ||
| name: sriov-nic-2 | ||
| namespace: openshift-sriov-network-operator | ||
| spec: | ||
| deviceType: netdevice | ||
| nicSelector: | ||
| pfNames: ["enp22s0"] | ||
| nodeSelector: | ||
| kubernetes.io/hostname: ocp-virt-worker-0.lab |
There was a problem hiding this comment.
The nodeSelector in both SriovNetworkNodePolicy resources (lines 10-11 and 26-27) uses a hardcoded hostname (ocp-virt-worker-0.lab). This ties the configuration to a specific node in a specific environment, which hurts reusability and makes it brittle. Consider using a more generic node label to identify nodes with the required capabilities. For example, you could label the node with something like sriov-nic-type: intel-XX and use that in the selectors.
No description provided.