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

feat(nextcloud): add notify_push support #581

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

Conversation

wrenix
Copy link
Collaborator

@wrenix wrenix commented Jun 9, 2024

Pull Request

Description of the change

Not yet tested

Benefits

  • support notify_push
  • improve ServiceMonitor (to collect data from nextcloud-exporter and notify_push)

Possible drawbacks

Applicable issues

Additional information

Checklist

TODO

  • redis password from existing secret (or URL)
  • put redis password from env to secrets
  • solve bootstrap problem (nextcloud and notify_push needs to be online)
  • option to set urlEnv on notifyPush (for external redis)

@wrenix wrenix force-pushed the feat/notify_push branch 2 times, most recently from 4c3cee5 to 46eb0e9 Compare June 9, 2024 22:57
@wrenix wrenix mentioned this pull request Jun 9, 2024
3 tasks
@wrenix wrenix force-pushed the feat/notify_push branch 3 times, most recently from f5f716e to 6ed673a Compare June 9, 2024 23:19
@AndreKoepke
Copy link

Can we ensure that the notify_push-plugin is installed? Maybe something like this?

 lifecycle:
      postStart:
        exec:
          command: ["occ",  "app:install notify_push"]

And we have to active that plugin by running sudo -u www-data ./occ notify_push:setup https://NEXTCLOUD_HOST/push. Maybe we should add a little script like this (pseudocode):

if (!notify_push installed)
  install notify_push
/occ notify_push:setup https://NEXTCLOUD_HOST/push

@provokateurin
Copy link
Member

I think it makes sense to give this option so the installation and setup is completely automatic, but I'd rather put it behind a second config flag:

notify_push:
	enabled: true
	automatic_setup: true

Maybe some people don't want to have this done automatically, so it's nice to give them the option.

@wrenix
Copy link
Collaborator Author

wrenix commented Jun 14, 2024

Therefore we has that hook of the container script (see #525), i write a ConfigMap for it.

PS: the same way, then in #480 (@provokateurin you wanted to take a look there ...)

PSS: Does somebody test this/my code?

@wrenix wrenix force-pushed the feat/notify_push branch from 65046cb to 7bb1bb9 Compare June 14, 2024 21:21
@AndreKoepke
Copy link

AndreKoepke commented Jun 17, 2024

andre@server:~/k8s/nextcloud$ helm upgrade -n nextcloud akops-nextcloud -f values.yml ./helm/charts/nextcloud/
Error: UPGRADE FAILED: template: nextcloud/templates/notify_push/deployment.yaml:41:31: executing "nextcloud/templates/notify_push/deployment.yaml" at <$.Values.global.image.registry>: nil pointer evaluating interface {}.image

Copy link

@AndreKoepke AndreKoepke left a comment

Choose a reason for hiding this comment

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

When I remove the global-references, then I got this error:

andre@server:~/k8s/nextcloud$ helm upgrade -n nextcloud akops-nextcloud -f values.yml ./helm/charts/nextcloud/
Error: UPGRADE FAILED: YAML parse error on nextcloud/templates/notify_push/deployment.yaml: error converting YAML to JSON: yaml: line 41: did not find expected key

charts/nextcloud/templates/notify_push/deployment.yaml Outdated Show resolved Hide resolved
charts/nextcloud/templates/notify_push/deployment.yaml Outdated Show resolved Hide resolved
@wrenix
Copy link
Collaborator Author

wrenix commented Jun 18, 2024

Oh sorry, that was a copy-paste error.
normally i has on my helm-charts a global.image part to overwrite for registry and so on.

@wrenix wrenix force-pushed the feat/notify_push branch from 6acfe33 to 84009ac Compare June 18, 2024 13:43
@AndreKoepke
Copy link

AndreKoepke commented Jun 18, 2024

I was able to try an install. The result was this:

Configuring Redis as session handler
=> Searching for scripts (*.sh) to run, located in the folder: /docker-entrypoint-hooks.d/before-starting
==> Running the script (cwd: /var/www/html): "/docker-entrypoint-hooks.d/before-starting/notify_push.sh"
notify_push already installed
✓ redis is configured
🗴 push server is not receiving redis messages (received 272721789, got 0)
==> Failed at executing "/docker-entrypoint-hooks.d/before-starting/notify_push.sh". Exit code: 1

Edit

I have a password for redis and it was not set. Can add this like this?

            - name: REDIS_URL
              value: "redis://:<PASSWORD>@{{ template "nextcloud.redis.fullname" . }}-master:{{ .Values.redis.master.service.ports.redis }}"

When I did this locally, then we have the chicken-egg-problem ...
The notify_push application needs a running nextcloud-instance to fully start. And the nextcloud-instance need a running notify_push.

Maybe there is a better hook after nextcloud has started?

@wrenix wrenix force-pushed the feat/notify_push branch from 7b358ca to cc7051d Compare June 18, 2024 18:37
@AndreKoepke
Copy link

With the fixed port, I still unable to run it.

Logs from notify_push pod:

[2024-06-19 06:44:36.746890 +00:00] ERROR [notify_push] src/main.rs:78: Self test failed: Error while communicating with nextcloud instance: error sending request for url (http://akops-nextcloud.nextcloud.svc.cluster.local:8080/index.php/apps/notify_push/test/version): error trying to connect: tcp connect error: Connection refused (os error 111)

@jessebot
Copy link
Collaborator

@wrenix I'm updated your push file to be .tpl instead of .gotmpl to match the rest of the tpl files. I may also pop in here and write a quick test so this gets tested in CI?

@wrenix wrenix force-pushed the feat/notify_push branch 6 times, most recently from 43ba133 to fc97749 Compare November 27, 2024 21:01
@wrenix
Copy link
Collaborator Author

wrenix commented Nov 27, 2024

Okay, i fix the startup problem.
It is inside the nextcloud "broken", blocking.

So i use:

occ config:app:set notify_push base_endpoint --value="URL"

instatt of occ notify_push:setup URL (which runs a self-test and needs nextcloud running (but we make the hook, before nextcloud starts)

@AndreKoepke
Copy link

AndreKoepke commented Nov 27, 2024

@wrenix cool changes. I tried your branch and it worked directly without any issue in my environment. :)

image

@wrenix wrenix force-pushed the feat/notify_push branch 3 times, most recently from b73428e to fa34027 Compare December 18, 2024 13:37
@wrenix wrenix marked this pull request as ready for review December 18, 2024 13:37
@wrenix
Copy link
Collaborator Author

wrenix commented Dec 18, 2024

I move the env handling back to the helper (and split it).
So we have nearly the same logic for database(_url) and redis_url for nextcloud and notify-push.

So it should be review ready (and merge ready).

I create an CHANGELOG.md for the Breaking Changes.

@wrenix wrenix force-pushed the feat/notify_push branch 3 times, most recently from d879976 to 3e0e0ca Compare December 18, 2024 15:45
@wrenix wrenix changed the base branch from main to develop December 19, 2024 22:26
@wrenix wrenix force-pushed the develop branch 3 times, most recently from d5360ea to da98433 Compare December 19, 2024 22:36
@wrenix wrenix changed the base branch from develop to main December 19, 2024 22:49
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.

[Feature] High Performance File Backend
5 participants