Skip to content
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

Adding an optional deployment to increase AI pickup speed #688

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

dhendricken
Copy link

@dhendricken dhendricken commented Jan 23, 2025

Description of the change

The aim of this PR is to add the process described in increasing AI pickup speed in a simple but functional way to the helm-chart.

It functions similarly to the cronjob sidecar by mounting the volumes under nextcloud-main.

Benefits

The deployment is completely optional as it only has relevance to those who want to use the nextcloud-assistant or other AI integrations. Additionally this method allows for scaling by increasing the number of replicas of the deployment. (This is why I decided against using a container withing the main pod).

It makes use of the same image and volumes as the main nextcloud pod, so I don't think it will require additional maintenance unless something about how AI Tasks are handled changes in the future. Maybe we won't even need this anymore who knows :)

Possible drawbacks

None that I can think of.

Applicable issues

I didn't find any open issues concerning this functionality.

Additional information

I created a modified version of the nextcloud.volumeMounts helper template to reduce the required volumes that need to be mounted to the pods as much as possible.

Checklist

dependabot bot and others added 4 commits January 23, 2025 21:20
Bumps [helm/chart-testing-action](https://github.com/helm/chart-testing-action) from 2.6.1 to 2.7.0.
- [Release notes](https://github.com/helm/chart-testing-action/releases)
- [Commits](helm/chart-testing-action@v2.6.1...v2.7.0)

---
updated-dependencies:
- dependency-name: helm/chart-testing-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Daniel Hendricken <daniel.hendricken@bedag.ch>
Signed-off-by: Daniel Hendricken <daniel.hendricken@bedag.ch>
Signed-off-by: Daniel Hendricken <daniel.hendricken@bedag.ch>
Signed-off-by: Daniel Hendricken <daniel.hendricken@bedag.ch>
…rker deployment

Signed-off-by: Daniel Hendricken <daniel.hendricken@bedag.ch>
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle this is a good idea, but the entire system is shifting away from being cron based, so I don't know how much sense it makes to add this now.

charts/nextcloud/README.md Outdated Show resolved Hide resolved
Co-authored-by: Kate <26026535+provokateurin@users.noreply.github.com>
Signed-off-by: Daniel Hendricken <162698556+dhendricken@users.noreply.github.com>
@dhendricken
Copy link
Author

dhendricken commented Jan 23, 2025

Alright no worries, I add these workers per kustomization myself at the moment. Works like a charm. I made the PR because I thought it might increase the ease of use for others that decide to use the assistant.

Ah and I forgot to mention that I tested it with both the apache and fpm-alpine images.

Copy link
Collaborator

@wrenix wrenix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good first try.

Comment on lines +23 to +25
app.kubernetes.io/name: {{ include "nextcloud.name" . }}-ai-worker
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/component: app
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You add a new component.
Please adjust the labels like in other deployments here:
e.g.:

Suggested change
app.kubernetes.io/name: {{ include "nextcloud.name" . }}-ai-worker
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/component: app
app.kubernetes.io/name: {{ include "nextcloud.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/component: ai-worker

volumeMounts:
{{- include "aiWorker.volumeMounts" . | trim | nindent 12 }}
volumes:
- name: nextcloud-main
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should we move that into _helpers.tpl and reuse it with deployment of nextcloud.

{{- toYaml .Values.aiWorker.podSecurityContext | nindent 8 }}
initContainers:
- name: wait-for-service
image: alpine/curl
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image should be in values.yaml too.



{{/*
Create volume mounts for the nextcloud-aiworker container.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it needed without the defaultConfigs and configs like in nextcloud.volumeMounts ?

I believe we should use the same setup then in the main deployment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants