🐛 Fixes #472 Remove stateless kai resources when set to false#533
🐛 Fixes #472 Remove stateless kai resources when set to false#533jmontleon merged 2 commits intokonveyor:mainfrom
Conversation
📝 WalkthroughWalkthroughCentralizes deployment state in Ansible tasks, converting nested conditionals to state-driven tasks for KAI DB, LLM Proxy, and Solution Server; ensures kai-api deployment only sets KAI_DB_DSN when the solution server is enabled; fixes llm-proxy ConfigMap YAML by providing Changes
Sequence Diagram(s)sequenceDiagram
participant Ansible
participant StateCalc as "State Calculator"
participant K8sAPI as "Kubernetes API"
participant LLMProxy as "LLM Proxy Components"
participant SolutionAPI as "KAI Solution Server"
participant DB as "KAI DB"
rect rgba(200,220,255,0.5)
Ansible->>StateCalc: Evaluate kai_common_state, kai_llm_proxy_state, kai_solution_server_state
StateCalc-->>Ansible: States
end
rect rgba(200,255,200,0.5)
Ansible->>K8sAPI: Create/merge KAI DB (if kai_common_state)
K8sAPI-->>DB: Deploy DB resources
end
rect rgba(255,230,200,0.5)
Ansible->>K8sAPI: Create ConfigMap, Deployment, Service for LLM Proxy (if kai_llm_proxy_state)
K8sAPI-->>LLMProxy: Provision LLM Proxy resources
end
rect rgba(255,200,220,0.5)
Ansible->>K8sAPI: Deploy KAI Solution Server (if kai_solution_server_state)
K8sAPI-->>SolutionAPI: Deploy Solution Server (env KAI_DB_DSN only rendered when enabled)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@roles/tackle/tasks/kai.yml`:
- Around line 83-87: The ConfigMap task "Create LLM Proxy Client Configuration"
currently uses "state: present" with "when: kai_solution_server_enabled | bool"
so it is skipped rather than removed when the server is disabled; change the
task to set state conditionally (e.g. state: present if
kai_solution_server_enabled | bool else absent) or drive state from the
lifecycle variable kai_solution_server_state so the k8s module will create or
delete the template kai/llm-proxy-client-configmap.yaml.j2 appropriately instead
of merely skipping the task.
- Around line 67-71: The ConfigMap task "Create LLM Proxy ConfigMap" currently
uses state: present with a when: kai_llm_proxy_enabled | bool so it is only
skipped when disabled and never removed; change it to always run and set the
state based on kai_llm_proxy_enabled (e.g. state: "{{ 'present' if
kai_llm_proxy_enabled | bool else 'absent' }}") and remove the when clause,
keeping the same template kai/llm-proxy-configmap.yaml.j2 so the ConfigMap is
created when true and deleted when false.
🧹 Nitpick comments (1)
roles/tackle/tasks/kai.yml (1)
73-98: Minor: Inconsistent spacing afterstate:.Some lines have double spaces (
state: "{{ ... }}"), others have single. Consider normalizing for consistency.✨ Proposed fix for spacing consistency
- name: Create LLM Proxy Deployment k8s: - state: "{{ kai_llm_proxy_state }}" + state: "{{ kai_llm_proxy_state }}" template: kai/llm-proxy-deployment.yaml.j2 - name: Create LLM Proxy Service k8s: - state: "{{ kai_llm_proxy_state }}" + state: "{{ kai_llm_proxy_state }}" template: kai/llm-proxy-service.yaml.j2(Same for lines 91 and 97)
c36b3d2 to
b317260
Compare
b317260 to
905c924
Compare
fabianvf
left a comment
There was a problem hiding this comment.
Not sure why the test failed so unhelpfully
| value: /api | ||
| - name: KAI_LLM_PARAMS | ||
| value: '{{ kai_llm_params | to_json }}' | ||
| {% if kai_solution_server_enabled|bool %} |
There was a problem hiding this comment.
Minor nit, this template is for the kai-solution-server, so I don't think we need this extra guard
There was a problem hiding this comment.
When kai-solution-server is false pg_password is undefined causing an undefined variable error if it's not guarded by the conditional.
905c924 to
5cb0837
Compare
5cb0837 to
05188a8
Compare
Signed-off-by: Jason Montleon <jmontleo@redhat.com>
05188a8 to
98a806d
Compare
…t to false Signed-off-by: Jason Montleon <jmontleo@redhat.com>
66d6187 to
05e3bcd
Compare
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Ensured config always includes a proper models mapping and added a terminating newline to avoid parsing issues. * **Refactor** * Simplified deployment logic to use centralized state flags, reducing nested conditions and improving reliability of component provisioning. * **Chores** * Rendered database-related environment settings only when the corresponding service is enabled to prevent unnecessary configuration exposure. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Jason Montleon <jmontleo@redhat.com> Signed-off-by: Cherry Picker <noreply@github.com>
|
PR cherry-picked to branch release-0.9. Backport PR: #539 |
…539) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Ensured config always includes a proper models mapping and added a terminating newline to avoid parsing issues. * **Refactor** * Simplified deployment logic to use centralized state flags, reducing nested conditions and improving reliability of component provisioning. * **Chores** * Rendered database-related environment settings only when the corresponding service is enabled to prevent unnecessary configuration exposure. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Jason Montleon <jmontleo@redhat.com> Signed-off-by: Cherry Picker <noreply@github.com> Signed-off-by: Jason Montleon <jmontleo@redhat.com> Signed-off-by: Cherry Picker <noreply@github.com> Co-authored-by: Jason Montleon <jmontleo@redhat.com>
Summary by CodeRabbit
Bug Fixes
Refactor
Chores