-
Notifications
You must be signed in to change notification settings - Fork 353
KAgent Install Profile Feature #796
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
KAgent Install Profile Feature #796
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
08ca93c to
6292660
Compare
|
something's broken with my signing, i use |
b1cf0e5 to
eeed096
Compare
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.
Pull Request Overview
Introduces a profile system for KAgent installations that allows users to select predefined configurations for which agents to install. The change sets the default installation to minimal (no agents) and provides demo and minimal profiles via YAML files.
- Changes default installation behavior from enabling all agents to enabling none
- Adds profile support through values files for demo and minimal configurations
- Updates CLI to support profile selection both interactively and via command-line flags
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| helm/kagent/values.yaml | Changes default agent configurations from enabled to disabled |
| helm/kagent/files/profiles/minimal.yaml | Adds minimal profile with all agents disabled |
| helm/kagent/files/profiles/demo.yaml | Adds demo profile with all agents enabled |
| helm/kagent/files/profiles/README.md | Documents the profile system and available profiles |
| helm/README.md | Updates documentation with profile usage examples |
| go/cli/internal/cli/install.go | Implements profile support in CLI installation logic |
| go/cli/internal/cli/const.go | Adds profile URL configuration constants |
| go/cli/cmd/kagent/main.go | Adds profile flag to install command and interactive selection |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
a5268bf to
917566d
Compare
go/cli/internal/cli/install.go
Outdated
| // If a profile is provided, write the embedded YAML to a temp file and pass its path | ||
| var tmpProfilePath string | ||
| if profile != "" { | ||
| profileYAML := profiles.GetProfile(profile) |
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.
converted back to a file, because the values are defined in Helm, and in order to properly unmarshal (to a non-generic interface{}) we'd need to redefine the values in a go struct, which could lead to drift + needing to keep both in sync.
| ### Using Helm | ||
|
|
||
| ```bash | ||
| helm install kmcp-crds oci://ghcr.io/kagent-dev/kmcp/helm/kmcp-crds --version 0.1.2 --namespace kagent |
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.
we no longer need to install kmcp-crds separately
go/cli/internal/cli/install.go
Outdated
| _ = tmpFile.Close() | ||
| _ = os.Remove(tmpFile.Name()) | ||
| return "", fmt.Errorf("failed writing temp profile file: %w", err) | ||
| } | ||
| if err := tmpFile.Close(); err != nil { | ||
| _ = os.Remove(tmpFile.Name()) | ||
| return "", fmt.Errorf("failed closing temp profile file: %w", err) |
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.
Can you just do a defer close/remove?
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.
Also, I believe helm supports passing the values yaml as a string, so that may be easier than saving a file.
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 couldn't find any docs on doing it inline through a parameter, but since it's a shell command we can pipe/input it as below (tested locally):
<yaml content> | helm ... -f -so the logic becomes:
cmd := exec.CommandContext(ctx, "helm", args...)
// If a profile is provided, pass the embedded YAML to the stdin of the helm command.
// This must be the last set of arguments.
if profile != "" {
profileYAML := profiles.GetProfile(profile)
cmd.Stdin = strings.NewReader(profileYAML)
cmd.Args = append(cmd.Args, "-f", "-")
}
// execute, etc.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 don't think we can do this yet :(.
I don't want such a big change/break mid-release. I would like to potentially wait until 0.7.0, or at least bring it up in the community meeting first
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.
Darn 💔
Although, if we're alright with this approach (after resolving review comments), we could keep this non-breaking by
- Revert agent defaults back to
true. - Remove
demoprofile. - Keep the
minimalprofile for users who want to easily opt-out of the additional agents.
Then in the future (i.e. 0.7.0) we could update agent defaults to false and re-add demo profile. (or we keep both right now, and just revert agents back to true).
I'm not against not merging this though 👍🏼
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 think keep them both for now and just revert the actual values file back to true. Having both profiles creates a good starting point for us to move forward with, even if the demo one is a tad redundant right this minute
go/cli/internal/cli/install.go
Outdated
| type helmConfig struct { | ||
| registry string | ||
| version string | ||
| values []string |
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 put profile in here as well
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.
Did it. I named it inlineValues to keep the naming generic, even though at the moment it's for holding the profile values (if one is chosen).
Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Fabian Gonzalez <fabiangonz98@gmail.com> Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
…ne in CLI installation, set up profile in helmConfig Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io> Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
- go dir structure: move controller folder under pkg and internal - remove webhook: we don't have it anyway Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
upgrading all libs from PRs - kagent-dev#787, kagent-dev#788, kagent-dev#777, kagent-dev#776 Signed-off-by: Peter Jausovec <peter.jausovec@solo.io> Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
- grab user from x-user-id header instead. - added debug ability to run kagent + agent locally. - added observed generation to conditions, fix agent text from mcp sever reconciler --------- Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com> Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
`kagent install` would construct an invalid `helm upgrade` command due
to a missing `--set` parameter.
When invoking `kagent install` (with no specified config params or other
`kagent` env vars) & running a `ps aux` in parallel I can see that
`kagent` runs the following command
```go
helm upgrade --install kagent oci://ghcr.io/kagent-dev/kagent/helm/kagent --version 0.6.3 --namespace kagent --create-namespace --wait --history-max 2 --timeout 5m --set providers.default=openAI --set providers.openAI.apiKey=<REDACTED> --set `
```
This command is invalid and errors with `Error: flag needs an argument:
--set`.
The reason that we pass `[]{""}` to `installChart()` is because we
obtain the values from
[here](https://github.com/kagent-dev/kagent/blob/main/go/cli/internal/cli/install.go#L78):
```go
helmExtraArgs := GetEnvVarWithDefault(KAGENT_HELM_EXTRA_ARGS, "")
```
where if `KAGENT_HELM_EXTRA_ARGS` is not present we return `""`.
This PR simply checks if the value is `!= ""` before appending to `helm
upgrade`
Signed-off-by: Tom Morelly <tom.morelly@clearroute.io>
Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
76a2546 to
d0e8162
Compare
|
|
merge conflicts, rebasing, and signing got this PR in a broken state. |
Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
original pr (which got in a terrible state due to merge conflicts + rebasing, so better to start fresh): #796 --- Originally, when listing the agents, if a single agent failed (i.e. `modelConfig` is invalid), no Agents were shown. # Changes With these changes, we: 1. Add a new AgentResponse field `Accepted` (i.e. `Reconciled`, but front-end friendly name) - This is based on the `Accepted` state of the Agent. 2. **Don't** return an error response _to the frontend_ when an Agent (or multiple) had failures. - We now continue listing agents, making the UI usable for valid agents, and adding a `Not Accepted` (could also be `Not Reconciled`, or `Error` for simplest terms) status on broken agents -- similar to how we do `Not Ready` while they are being deployed. 3. No longer display unready/un-accepted Agents in the agent sidebar from chats. ## Alternative Approaches Another approach would be to avoid returning the not-reconciled agent altogether. This would avoid needing to set up the new response field and UI changes. I was originally leaning towards that one, but figured it may be useful/quicker for users to notice any broken agents if they appear in the UI with their failed status. Another idea I had, but would probably be overkill and could constitute as a new feature, is to have a "notification system", where we would _not_ display failed agents in the agents list, but instead have the reconciliation as a notification. # Testing ## Dashboard Build & run ```sh make create-kind-cluster helm-install build-cli-local KAGENT_HELM_REPO=./helm/ ./go/bin/kagent-local dashboard ``` Update an Agent to have an invalid `modelConfig` ```sh kubectl patch agents.kagent.dev/k8s-agent -n kagent --type='json' -p='[{"op": "replace", "path": "/spec/declarative/modelConfig", "value": "default-model-config-2"}]' ``` Reload page. View listing UI to see the failed state (Not Accepted). Fix the agent. ```sh kubectl patch agents.kagent.dev/k8s-agent -n kagent --type='json' -p='[{"op": "replace", "path": "/spec/declarative/modelConfig", "value": "default-model-config"}]' ``` Reload page. It’s all good again! ## CLI This would ideally also get tested through the CLI to test the behavior of `KAGENT_HELM_REPO=./helm/ ./go/bin/kagent-local get agent`, _but_ I’m hitting “userID is required” issues. My `$HOME/.kagent/config.yaml` file has it defined, so I'm not sure of the exact issue yet. --- Resolves: #797 --------- Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
original pr (which got in a terrible state due to merge conflicts + rebasing, so better to start fresh): kagent-dev#796 --- Originally, when listing the agents, if a single agent failed (i.e. `modelConfig` is invalid), no Agents were shown. # Changes With these changes, we: 1. Add a new AgentResponse field `Accepted` (i.e. `Reconciled`, but front-end friendly name) - This is based on the `Accepted` state of the Agent. 2. **Don't** return an error response _to the frontend_ when an Agent (or multiple) had failures. - We now continue listing agents, making the UI usable for valid agents, and adding a `Not Accepted` (could also be `Not Reconciled`, or `Error` for simplest terms) status on broken agents -- similar to how we do `Not Ready` while they are being deployed. 3. No longer display unready/un-accepted Agents in the agent sidebar from chats. ## Alternative Approaches Another approach would be to avoid returning the not-reconciled agent altogether. This would avoid needing to set up the new response field and UI changes. I was originally leaning towards that one, but figured it may be useful/quicker for users to notice any broken agents if they appear in the UI with their failed status. Another idea I had, but would probably be overkill and could constitute as a new feature, is to have a "notification system", where we would _not_ display failed agents in the agents list, but instead have the reconciliation as a notification. # Testing ## Dashboard Build & run ```sh make create-kind-cluster helm-install build-cli-local KAGENT_HELM_REPO=./helm/ ./go/bin/kagent-local dashboard ``` Update an Agent to have an invalid `modelConfig` ```sh kubectl patch agents.kagent.dev/k8s-agent -n kagent --type='json' -p='[{"op": "replace", "path": "/spec/declarative/modelConfig", "value": "default-model-config-2"}]' ``` Reload page. View listing UI to see the failed state (Not Accepted). Fix the agent. ```sh kubectl patch agents.kagent.dev/k8s-agent -n kagent --type='json' -p='[{"op": "replace", "path": "/spec/declarative/modelConfig", "value": "default-model-config"}]' ``` Reload page. It’s all good again! ## CLI This would ideally also get tested through the CLI to test the behavior of `KAGENT_HELM_REPO=./helm/ ./go/bin/kagent-local get agent`, _but_ I’m hitting “userID is required” issues. My `$HOME/.kagent/config.yaml` file has it defined, so I'm not sure of the exact issue yet. --- Resolves: kagent-dev#797 --------- Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
Changes
Support "profiles" through values files with default settings. At the moment, these are just for setting up which agents to install into cluster.
demo: Installs all agents, as done now.minimal(default): Installs no agent.I created both explicitly in case we change defaults are anything happens, where these profiles should still act the same.
Profiles are not bundled with helm and are stored alongside the CLI code in go to be embedded. They can still be referenced through
helminstallations by providing the yaml filepath.Breaking/Changed Defaults
IMPORTANT: With these changes, the additional agents are not installed by default. Useers would either need to
helm [...] -f <raw-gh-path-to-demo-file>orkagent install --profile demoto install.Testing
Helm Installation
CLI Installation
Build
Uninstall current kagent installation
Install, non-interactive
Uninstall current kagent installation
Install, interactive
Then choose a profile from the profile selection and see installed pods.
Notes
I was looking into Istio's profile implementation, but this may not be possible on our end because of us importing the agents as subchart dependencies in Chart.yaml. Afaik, Chart.yaml is processed before templates, so
--set profile=xwould not work. Doing a-f <path>/<profile>.yamldoes work as it's not a template, and the values would process before the Chart.Resolves: #766
(not an implementation of the exact request flag, but the concluded method to handle this issue)