-
Notifications
You must be signed in to change notification settings - Fork 179
feat: add agent-operator & kb-operator #2580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| # AkamaiKnowledgeBase custom resources | ||
| - apiGroups: ["akamai.io"] | ||
| resources: ["akamaiknowledgebases"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the group is already akamai.io, why does the resource name need to have the "akamai" prefix?
|
|
||
| # AkamaiAgent custom resources | ||
| - apiGroups: ["akamai.io"] | ||
| resources: ["akamaiagents"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the group is already akamai.io, why does the resource name need to have the "akamai" prefix?
| resources: ["akamaiknowledgebases"] | ||
| verbs: ["get", "list", "watch"] | ||
|
|
||
| # TODO strict this down further for only some namespaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it right now, so later we do not need to worry about side effects of narrowing down the permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you seen this: https://kopf.readthedocs.io/en/stable/deployment/#rbac?
| # Service discovery for foundation models and agent services | ||
| - apiGroups: [""] | ||
| resources: ["services"] | ||
| verbs: ["get", "list", "create", "update", "patch", "delete"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the service discover requires more than "get" and "list"?
| @@ -0,0 +1,92 @@ | |||
| {{- if .Values.agentOperator.enabled }} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do the agent-operator and the kb-operator are part of the same chart?
charts/ai-operators/values.yaml
Outdated
| # Knowledge Base Operator configuration | ||
| kbOperator: | ||
| enabled: true | ||
| replicaCount: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This operator does not support running with more than one replica thus we should not expose the replica parameter.
charts/ai-operators/values.yaml
Outdated
| # Agent Operator configuration | ||
| agentOperator: | ||
| enabled: true | ||
| replicaCount: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This operator does not support running with more than one replica thus we should not expose the replica parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you revise the following:
- split this chart into two helm charts (separation of concerns)
- narrow down RBAC (least privileged rule)
| pkg: kserve | ||
| <<: *default | ||
| - name: ai-operators | ||
| installed: {{ $v.otomi.aiEnabled }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about his flag. Are we going to treat those chart as special ones and build the logic around that flag in UI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flag is already used in the frontend to hide the agents and knowledgebase tabs in the sidebar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which I think is valid because otherwise those tabs might be confusing and just be in the way for people who are not interested in AI parts. But as a counter argument we are hiding features in the platform that people might be interested in.
| # Knowledge Base Operator configuration | ||
| kbOperator: | ||
| enabled: true | ||
| replicaCount: {{ $ag.replicaCount }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| replicaCount: {{ $ag.replicaCount }} | |
| replicaCount: {{ $kb.replicaCount }} |
?
helmfile.d/snippets/defaults.yaml
Outdated
| replicaCount: 1 | ||
| image: | ||
| repository: linode/ai-operators | ||
| tag: APL-1189 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a reminder, it should be a semantic version or not a feature branch.
| @@ -1,5 +1,5 @@ | |||
| api: main | |||
| console: main | |||
| console: APL-1142 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder: please revert it before merging.
📌 Summary
🔍 Reviewer Notes
🧹 Checklist