Fix deployment configuration update & trait update issues #123
Conversation
📝 WalkthroughWalkthroughRefactors two client methods to perform get-validate-mutate-update inside a retry loop (now treating Kubernetes Conflict as retryable) and adds a conditional pod template Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Retry as Retry Loop
participant K8s as Kubernetes API
Client->>Retry: Request update (Attach/Deploy)
rect rgb(235,245,235)
Note over Retry,K8s: Retry attempt begins
Retry->>K8s: GET resource (latest)
K8s-->>Retry: Resource (resourceVersion)
Retry->>Retry: Validate & mutate resource
Retry->>K8s: UPDATE resource (with version)
end
alt 409 Conflict
K8s-->>Retry: 409 Conflict
Retry->>Retry: Backoff & retry (fetch latest)
Retry->>K8s: GET resource (latest)
K8s-->>Retry: Resource (new version)
Retry->>Retry: Re-validate & mutate
Retry->>K8s: UPDATE resource
end
rect rgb(230,240,255)
Note over K8s,Client: Success
K8s-->>Retry: 200 OK
Retry-->>Client: Success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-12-18T16:41:56.601ZApplied to files:
🧬 Code graph analysis (1)agent-manager-service/clients/openchoreosvc/client.go (2)
🔇 Additional comments (3)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
agent-manager-service/clients/openchoreosvc/client.goagent-manager-service/clients/openchoreosvc/retry.godeployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-types/agent-api.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T16:41:56.601Z
Learnt from: hanzjk
Repo: wso2/ai-agent-management-platform PR: 70
File: agent-manager-service/services/agent_manager.go:978-980
Timestamp: 2025-12-18T16:41:56.601Z
Learning: In the agent-manager-service, enforce that external agents cannot have a SubType; only internal/managed agents may have SubType (e.g., chat-api, custom-api). When converting external agents to AgentResponse, the Type payload should include only the Type field and should not expose SubType. Implement this in the conversion/serialization path (likely in services/agent_manager.go) and update related DTO mappings or serializers. Add unit tests to verify internal vs external agent handling and ensure SubType is omitted for external agents.
Applied to files:
agent-manager-service/clients/openchoreosvc/retry.goagent-manager-service/clients/openchoreosvc/client.go
🔇 Additional comments (2)
agent-manager-service/clients/openchoreosvc/retry.go (1)
142-145: LGTM - Correct handling of optimistic concurrency conflicts.Adding
IsConflictto retryable errors enables the standard Kubernetes pattern for handling concurrent updates. When a resource is modified between fetch and update operations, the API returns a Conflict error (HTTP 409). Retrying with a fresh fetch ensures the update is applied to the latest resource version.agent-manager-service/clients/openchoreosvc/client.go (1)
528-542: The retry logic is safe:updateWorkloadSpecdirectly assigns (rather than appends to) the Containers map and builds environment variables from scratch on each call using only request parameters, making it idempotent.
| annotations: | ||
| openchoreo.dev/config-hash: | | ||
| ${has(configurations[parameters.containerName].configs.envs) && configurations[parameters.containerName].configs.envs.size() > 0 ? oc_hash(configurations[parameters.containerName].configs.envs.map(e, e.name + "=" + e.value).join(",")) : oc_hash("no-env")} |
There was a problem hiding this comment.
Config hash should include all configuration sources.
The hash currently only includes environment variables from configs.envs. Changes to the following will NOT trigger a deployment rollout:
secrets.envs(line 87-92)configs.files(line 94-107)secrets.files(line 101-106)
This means users updating secrets or file-based configurations won't see their changes reflected in the deployment without a manual restart, which doesn't fully address issue #121.
🔎 Suggested fix to include all configuration sources
annotations:
- openchoreo.dev/config-hash: |
- ${has(configurations[parameters.containerName].configs.envs) && configurations[parameters.containerName].configs.envs.size() > 0 ? oc_hash(configurations[parameters.containerName].configs.envs.map(e, e.name + "=" + e.value).join(",")) : oc_hash("no-env")}
+ openchoreo.dev/config-hash: |
+ ${oc_hash(
+ (has(configurations[parameters.containerName].configs.envs) && configurations[parameters.containerName].configs.envs.size() > 0 ?
+ configurations[parameters.containerName].configs.envs.map(e, e.name + "=" + e.value).join(",") : "") + "|" +
+ (has(configurations[parameters.containerName].secrets.envs) && configurations[parameters.containerName].secrets.envs.size() > 0 ?
+ configurations[parameters.containerName].secrets.envs.map(e, e.name).join(",") : "") + "|" +
+ (has(configurations[parameters.containerName].configs.files) && configurations[parameters.containerName].configs.files.size() > 0 ?
+ configurations[parameters.containerName].configs.files.map(f, f.name + "@" + f.mountPath).join(",") : "") + "|" +
+ (has(configurations[parameters.containerName].secrets.files) && configurations[parameters.containerName].secrets.files.size() > 0 ?
+ configurations[parameters.containerName].secrets.files.map(f, f.name + "@" + f.mountPath).join(",") : "")
+ )}Note: For secrets, include only the names/keys in the hash, not the values, as the values are sensitive. The goal is to detect when the set of secrets changes, which will trigger a rollout.
🤖 Prompt for AI Agents
In
deployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-types/agent-api.yaml
around lines 57-59, the config-hash only includes configs.envs so changes to
secrets.envs, configs.files and secrets.files won't trigger rollouts; update the
expression to combine and hash all four sources (configs.envs values,
secrets.envs names/keys only, configs.files filenames/keys, and secrets.files
filenames/keys) by mapping each collection to a stable "key=value" or "name"
string and joining them (falling back to a fixed sentinel like "no-config" when
a collection is empty) before calling oc_hash so any change to those sources
updates the annotation and triggers a rollout.
1 participant
Purpose
This PR is to fix #121 & #120
To fix this, updated that whole read-write-modify operation to execute in a retry block, retrying on such conflicts.
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.