[bitnami/discourse] Plugins should be installed on sideqik as well

Giiltham commented 2 months ago

Name and Version

bitnami/discourse 14.1.0

What architecture are you using?

None

What steps will reproduce the bug?

install discourse-ai :

discourse: 
    plugins: 
        - https://github.com/discourse/discourse-ai
    persistPlugins: false

What is the expected behavior?

I should be able to use the discourse-ai plugin on my instance

What do you see instead?

plugins actions behind a Job don't work

In the logs, I see:

Job exception: uninitialized constant Jobs::GenerateEmbeddings
Job exception: uninitialized constant Jobs::CreateAiChatReply

Sideqik can't find the plugin Jobs, it doesn't have the plugin at all

Additional information

Installing manually the plugin by exec in the sidekiq container works.

persistPlugins : false should be the culprit here as it removes the link with the discourse-data volume, meaning it will prevent all plugins with jobs from working.

juan131 commented 1 month ago

Hi @Giiltham

Correct me if I'm wrong but you're able to reproduce the issue regardless the discourse.persistPlugins parameter is enabled or not, am I right?

Giiltham commented 1 month ago

I don't think i have this issue while using persistPlugins: true, as the plugins are also present on the Sideqik sidecar. Using discourse-topic-voting with persistPlugins: false, I had over 1000:

Job exception: uninitialized constant Jobs::DiscourseTopicVoting

I don't have any since switching to persistPlugins: true (doesn't seems to be a valid option for us as it does not allow to add new plugins after initial setup)

juan131 commented 1 month ago

Hi @Giiltham

I see.. I can think of two different solutions:

Replicate logic to install plugins on Sidekiq sidecar when plugins aren't persisted.
Move plugins installation to a init-container that shares the modified filesystem with both Discourse and Sidekiq containers.

AMontagu commented 1 month ago

Hello @juan131

It look like the second recommandations: "Move plugins installation to a init-container that shares the modified filesystem with both Discourse and Sidekiq containers." is what it should be done when using persistPlugins. It create a PVC named discourse-data that should be shared between all instances.

The fact is that in the discource container the plugin are installed in /opt/bitnami/plugins but the volumes in mount on /bitnami/plugins

Is a specific script that copy the plugins to the discource-data ? Why not installing plugins directly on the pvc ?

juan131 commented 1 month ago

Hi @AMontagu

Let me provide you some context.

Discourse is installed under the /opt/bitnami/discourse folder. In order to provide a way to upgrade Discourse by simply replacing the container with a new container image (this process is explained in the link below), the container logic moves during initialization certain sub-directories to the /bitnami/discourse folder (where we mount Persistent Volumes to persist data) and create symbolic links following the format "/opt/bitnami/discourse/SUB_DIR_A points to /bitnami/discourse/SUB_DIR_A".

https://github.com/bitnami/containers/tree/main/bitnami/discourse#upgrade-this-image

As explained in the container's README.md (check the link below), the data to be persisted is based on the env. variable DISCOURSE_DATA_TO_PERSIST which, by default, includes the "plugins" directory.

https://github.com/bitnami/containers/tree/main/bitnami/discourse#customizable-environment-variables

You can find the container logic that performs these steps in the links below:

AMontagu commented 1 month ago

Hello @juan131

Thank you for the time you took to make this explanation. This is more clear about how it works. The fact that plugin installed in the PVC was disappearing after being installed make sense now.

In the case where persistPlugins: true why not removing plugins from DISCOURSE_DATA_TO_PERSIST and use the PVC directly to /opt/bitnami/discourse ?

If we go further why not having a persistDiscourseData options that replace the persist_app bash function by a PVC that is directly mounted to the correct path ?

By the way with your explanation we was able to juste copy back the pvc populated in our init container to /bitnami/discourse/plugins after discourse initialisation. It just seem a little bit hacky because it initialize everything and try to persist data only to be overrided at the en by a copy from the PVC. To notice that this copy also add initialisation time.

juan131 commented 1 month ago

In the case where persistPlugins: true why not removing plugins from DISCOURSE_DATA_TO_PERSIST and use the PVC directly to /opt/bitnami/discourse ? If we go further why not having a persistDiscourseData options that replace the persist_app bash function by a PVC that is directly mounted to the correct path ?

Persisting the whole application would break upgradability by replacing the container image (e.g. I'm running Discourse with container image docker.io/bitnami/discourse:3.3.0-debian-12-r0 and I replace it with docker.io/bitnami/discourse:3.3.1-debian-12-r4) given the app code will be persisted.

AMontagu commented 1 month ago

I see for the whole application. But only for the plugins folder should work no ?

juan131 commented 1 month ago

Yes, the idea of using DISCOURSE_DATA_TO_PERSIST and symlinks is to have everything to be persisted (not only plugins) under the same directory so it's easier to mount a data volume there or create backups.

AMontagu commented 1 month ago

So why not reusing the same logic into the sidekiq ? Is there is any blocking point ?

As this logic of persist data happen after the volume are mount it is stopping anyone to have it's own volume easily (If persistPlugins is not true -> no symlink / If it is true then the persistData method override the volume)

The easiest way will be to have the same logic into the sidekiq so the volume already existing is shared between both containers.

juan131 commented 1 month ago

@AMontagu I just created a PR that should fix the issues by moving plugins installation to a init-container.

AMontagu commented 1 month ago

Great thank you ! @Giiltham can you test it and close this issue ?

Giiltham commented 1 month ago

Hi, thank for the PR @juan131, I will test it later this week

juan131 commented 1 month ago

Thanks!

netdata-be commented 3 weeks ago

I tried upgrading to the latest helm chart and this commit seems to break the plugin handling. Whatever we try the plugin is not extracted from the init container and thus not injected in the discours / sidekiq containers.

What I'm I doing wrong here? Tried to play with persistPlugins ( default, true, false) but none of them seem to get this sorted.

It looks like this is a bug

notz commented 3 weeks ago

@netdata-be Experienced the same troubles.

For me, the mount path in the init container doesn't match with the mounted volume in the other containers, if I'm right.

Init container:

empty-dir volume is mounted as /empty-dir/app-plugins-dir and plugins are copied to /empty-dir/app-plugins-dir/plugins so in the root of empty-dir volume

On the other containers:

empty-dir volume with subpath "app-plugins-dir" is mounted to /opt/bitnami/discourse/plugins

So the subpath doesn't exist in empty-dir volume.

juan131 commented 3 weeks ago

Hi @notz @netdata-be

Please share the values you're using and I'd try to reproduce the upgrading issue.

notz commented 3 weeks ago

Hi @notz @netdata-be

Please share the values you're using and I'd try to reproduce the upgrading issue.

@juan131 We are using flux and the helm chart with standard values like this:

    updateStrategy:
      type: Recreate
      rollingUpdate: null
    discourse:
      extraEnvVarsSecret: discourse-env
      plugins:
        - https://github.com/discourse/discourse-oauth2-basic
        - https://github.com/discourse/discourse-reactions
        - https://github.com/discourse/discourse-calendar
        - https://github.com/discourse/discourse-post-voting
        - https://github.com/discourse/discourse-solved
        - https://github.com/discourse/discourse-docs
      persistPlugins: false
    sidekiq:
      extraEnvVarsSecret: discourse-env

juan131 commented 3 weeks ago

Thanks guys! I just created a follow-up PR addressing the error you found out. Sorry for the incoveniences.

bitnami / charts