MGMT-21352: Update system prompt regarding static networking support #173
Conversation
- **Adds a message to inform the user when the .env file already exists**, preventing unnecessary interactive configuration. - **Adds print statements to clarify script actions**, specifically when generating the `lightspeed-stack.yaml` and `systemprompt.txt` files. Signed-off-by: Eran Cohen <[email protected]>
Adds `dnsPolicy: "None"` and sets `nameservers` to Google's public DNS (`8.8.8.8` and `8.8.4.4`) in the `assisted-chat-pod.yaml`. This change ensures reliable DNS resolution for the container, preventing "Name or service not known" errors when connecting to external services like `generativelanguage.googleapis.com`. Signed-off-by: Eran Cohen <[email protected]>
|
@eranco74: This pull request references MGMT-21352 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
WalkthroughAdds Pod DNS settings, makes the generate script skip interactive prompts when Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Gen as scripts/generate.sh
participant FS as Filesystem
User->>Gen: run ./scripts/generate.sh
Gen->>FS: test -f .env ?
alt .env exists
Gen-->>User: "The .env file already exists. Skipping interactive configuration."
Gen-->>User: "Generating $PROJECT_ROOT/config/lightspeed-stack.yaml"
Gen->>FS: write config/lightspeed-stack.yaml
Gen-->>User: "Generating $PROJECT_ROOT/config/systemprompt.txt"
Gen->>FS: write config/systemprompt.txt
else .env missing
Gen-->>User: prompt interactively and generate files
end
sequenceDiagram
autonumber
actor Client
participant Template as template.yaml logic
participant User as Requestor
User->>Template: Request cluster creation (with static networking)
Template->>Template: Pre‑flight checks: detect static/manual networking
alt static networking detected
Template-->>User: Refuse request — "I do not support creating clusters with static networking... Please use the assisted-installer web-based wizard"
else no static networking
Template-->>User: Continue installation flow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@eranco74: This pull request references MGMT-21352 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
template.yaml (1)
245-245: Clarify “static networking” guidance to avoid ambiguity.Recommend explicitly saying “assisted-chat does not support configuring static IPs; use the Assisted Installer UI wizard to create the cluster” and keep the VIPs clarification. This keeps the assistant’s guidance crisp and user-visible, without implying the platform overall lacks static IP support.
- * **Static Networking (unsupported)**: If the user asks about **static networking** (manually assigning IP addresses to hosts), inform them that you currently don't support this. **Instruct them to use the wizard to create the cluster instead**. Do not confuse this with setting API and Ingress VIPs, which are a different concept and are supported. + * **Static networking (unsupported in assisted-chat):** If asked about manually assigning host IPs, state that assisted-chat cannot configure static IPs. **Instruct the user to use the Assisted Installer UI wizard to create the cluster instead.** This is distinct from setting API and Ingress VIPs, which remain supported.scripts/generate.sh (1)
88-98: Helpful progress logs. Consider adding lightweight prereq checks.The new echoes improve visibility. Optionally add a fast-fail check for
ocandyqbefore generation to fail early with a clear message.Example snippet to place above the generation steps:
command -v oc >/dev/null || { echo "oc not found in PATH" >&2; exit 1; } command -v yq >/dev/null || { echo "yq not found in PATH" >&2; exit 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
assisted-chat-pod.yaml(1 hunks)scripts/generate.sh(2 hunks)template.yaml(1 hunks)
🔇 Additional comments (1)
scripts/generate.sh (1)
76-78: Good UX: skip interactive flow when .env exists.Message is clear and prevents accidental overwrite. LGTM.
| dnsPolicy: "None" | ||
| dnsConfig: | ||
| nameservers: | ||
| - "8.8.8.8" | ||
| - "8.8.4.4" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoding external DNS breaks in-cluster name resolution.
Setting dnsPolicy: "None" with Google DNS disables cluster DNS, so Pod code cannot resolve in-cluster names (Services, pods). This can break calls like http://lightspeed-stack:8090/ and other cluster lookups, and may violate restricted-network policies.
Prefer default cluster DNS, or make external resolvers optional/parametrized. Minimal safe fix:
- dnsPolicy: "None"
- dnsConfig:
- nameservers:
- - "8.8.8.8"
- - "8.8.4.4"
+ dnsPolicy: ClusterFirstIf you truly need public resolvers, consider parameterizing them and appending via dnsConfig.options while keeping cluster DNS (and document the trade-offs).
📝 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.
| dnsPolicy: "None" | |
| dnsConfig: | |
| nameservers: | |
| - "8.8.8.8" | |
| - "8.8.4.4" | |
| dnsPolicy: ClusterFirst |
🤖 Prompt for AI Agents
In assisted-chat-pod.yaml around lines 6 to 10, the pod is forcing dnsPolicy:
"None" and hardcoding public nameservers which disables cluster DNS and breaks
in-cluster name resolution; change to use the cluster DNS (remove dnsPolicy or
set dnsPolicy: ClusterFirst) and make external resolvers optional/parameterized
instead of hardcoding them, or if public resolvers are required, append them via
dnsConfig.options while keeping ClusterFirst so cluster service/pod names still
resolve and document the trade-offs.
There was a problem hiding this comment.
This is a good point. Will this break dns resolution to the mcp server?
There was a problem hiding this comment.
Same thing for the UI which has:
- name: AIUI_CHAT_API_URL
value: http://lightspeed-stack:8090/
There was a problem hiding this comment.
It doesn't seem to break anything:
Enter your query (or type 'exit' to quit): list my clusters
Our conversation ID: 1e5decc5-ad48-4448-87eb-a41f7c153099
conversation_id: 1e5decc5-ad48-4448-87eb-a41f7c153099
response: 'Here are your clusters:
* `test1` (ID: `7707ed0e-8a28-43a0-b0c6-a0e812cb055e`, OpenShift version: `4.20.0-ec.6`,
status: `pending-for-input`)
* `eran` (ID: `3ec3d3f0-66e2-4277-8c8f-099fb5370d53`, OpenShift version: `4.19.9`,
status: `pending-for-input`)
* `demo` (ID: `6e294cd4-ef3c-472a-8326-64c69b53e06d`, OpenShift version: `4.19.9`,
status: `pending-for-input`)
* `itzik` (ID: `118be539-8302-466f-87ab-dd8731d6e9e1`, OpenShift version: `4.19.7`,
status: `pending-for-input`)
What would you like to do next? I can provide more information about a specific
cluster, or we can proceed with the installation of one of these clusters.
There was a problem hiding this comment.
It is a good point though. I asked Gemini how it works:
How It Works
Podman's Internal DNS: When you run podman play kube, it creates a network namespace for the entire Pod. Within this namespace, Podman sets up a small DNS resolver. This resolver is specifically designed to handle the hostnames of containers within the same Pod. So, when the ui container tries to resolve lightspeed-stack, Podman's internal DNS immediately provides the correct internal IP address (typically 127.0.0.1 or the Pod's loopback address, as all containers in a Pod share the same network stack).
DNS Hierarchy: The dnsConfig you added doesn't completely replace Podman's internal DNS functionality; instead, it is appended to the list of resolvers that a container will try to use. The resolution process typically works like this:
A container tries to resolve a name, like lightspeed-stack.
The request first goes to the Pod's internal DNS resolver.
Since lightspeed-stack is a known hostname within the Pod, the internal resolver immediately returns the correct IP address (127.0.0.1), and the connection is successful.
Fallback to External DNS: If a container tries to resolve an external hostname, such as generativelanguage.googleapis.com:
The request goes to the Pod's internal DNS resolver first.
The internal resolver doesn't know about this external domain.
The request then falls back to the external DNS servers you specified in dnsConfig (8.8.8.8 and 8.8.4.4).
These external servers can successfully resolve generativelanguage.googleapis.com to its public IP address, allowing the container to make the external connection.
|
@eranco74: This pull request references MGMT-21352 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
The chat behavior with the updated system prompt: |
|
@eranco74: This pull request references MGMT-21352 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
This commit updates the assisted-chat responses to correctly address user queries about static networking. Previously, the bot would confuse static networking for hosts with the manual configuration of API and Ingress VIPs. assisted-chat new states that it doesn't support configuring static networking and instruct the user to use the UI wizard to create the cluster instead * add eval test Signed-off-by: Eran Cohen <[email protected]>
|
@eranco74: This pull request references MGMT-21352 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/evals/eval_data.yaml (1)
36-42: Harden eval for wizard phrasing variations.• Our search found only the lowercase, hyphenated “assisted-installer web-based wizard” in template.yaml and eval_data.yaml; no capitalized “Assisted Installer web-based wizard” exists. Consider adding an additional eval entry that expects the capitalized variant to avoid flakes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
template.yaml(2 hunks)test/evals/eval_data.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- template.yaml
| - eval_id: static_networking_support | ||
| eval_query: Create a cluster with static networking | ||
| eval_types: [response_eval:sub-string] | ||
| expected_keywords: ["I do not support creating clusters with static networking", "Please use the assisted-installer web-based wizard"] |
There was a problem hiding this comment.
How does this work? Does it require that one of the entire quoted strings is present? That seems fragile.
There was a problem hiding this comment.
It does, and it's not fragile because it's instructed to reply with these strings in the system prompt
There was a problem hiding this comment.
still it might be better to use accuracy test here, it's more flexible. sub-string eval looks for exact matches and is not really forgiving.
There was a problem hiding this comment.
TBH, I started with an accuracy test, and switched to sub-string because:
- it's simple and doesn't require LLM.
- I want to know that we get this exact output.
For instance, I don't want the chat response to say:
"Would you like me to help you create a cluster using the assisted-installer wizard" which might pass the accuracy test.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin, eranco74 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit f85f588
into
rh-ecosystem-edge:main
4 participants
Summary by CodeRabbit
New Features
Chores
Documentation
Tests