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

Add job template to deploy search #95

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

abbottmg
Copy link
Contributor

@abbottmg abbottmg commented Oct 9, 2023

This creates a simple job template to fire off when deploying elasticsearch, e.g. as part of upgrading to 4.2.0 and beyond.

@abbottmg
Copy link
Contributor Author

abbottmg commented Oct 9, 2023

This addresses #87.

@hardillb
Copy link
Contributor

I ran the command this job runs manually after enabling elastic search and it all worked as expected, I vote to get this merged.

@renchap
Copy link
Sponsor Member

renchap commented Jan 28, 2024

If I understand correctly, this will launch an ES re-index after each chart install/upgrade.

This is a bad idea as reindexing is an heavy task and has impact to the cluster. It starts by dropping the indexes, then re-create them, so search results are non-existing and/or wrong until it finished.

Re-indexes should be launched only when needed (a new index is created, or an existing index has an updated schema), and this is nothing in Mastodon to know if this is the case after an update.

I see how this could be useful and will see if we can add a command that launches a re-index only if needed, but for now this should not be merged.

@abbottmg
Copy link
Contributor Author

The intended use case is to deploy once with this enabled and then disable it before future chart deployments. This is a pattern we already use for other one-time jobs.

@renchap
Copy link
Sponsor Member

renchap commented Jan 28, 2024

The intended use case is to deploy once with this enabled and then disable it before future chart deployments. This is a pattern we already use for other one-time jobs.

I am not sure I understand how this works. How do you disable for future deployments?

@abbottmg
Copy link
Contributor Author

abbottmg commented Jan 29, 2024

When you want to deploy search, you pass --set 'matsodon.deploySearch.enabled=true' at the command line, which overrides the setting in values.yaml for that single run.

You can also change that setting to true within your values file, run helm upgrade once, then change it back to false before your next deploy.

@grrywlsn
Copy link

Is the intention that it runs once, on initial setup, but then not again on future helm upgrades?

In which case, wouldn't the post-install helm hook be a better fit for what you're trying to do?

Having to change values (especially ones going against the default) to achieve the behaviour feels like an anti-pattern imo.

@hardillb
Copy link
Contributor

The intention was to allow the simple enabling of Elastic Search at the point of an upgrade after an initial install without ES enabled (as discussed in #87)

So the question becomes:

  • Should this be something the user needs to do as part of the upgrade (e.g. include the --set option mentioned)
  • Or a manual task after that is well documented and easy to find (since the docs linked to in the values.yml currently 404)

@abbottmg
Copy link
Contributor Author

abbottmg commented Jan 29, 2024

The intention is [edit: as hardillb noted while I wrote this!] for pre-4.2 installations to run this on upgrade. IIRC a server installed post 4.2 with elasticsearch already working wouldn't need to run this command, but my memory's rusty about that.

In either case, this is technically a post-install hook, as well as a post-upgrade hook. Users may leave this feature enabled across multiple upgrades; they will simply have their databases re-indexed which, as @renchap pointed out yesterday, is heavy work to repeat needlessly.

We can compare and contrast this with job-db-migrate, which also runs in an upgrade hook and so runs every time the chart is upgraded--and can slow down or block deployment, because it requires the database containers to be accessible, even if we're not actually making any changes to the db schema.

We decide to run it every time because the best way to detect whether the work is necessary is to run the rake command anyway. I would definitely love to know if there's an easy way to detect whether ES needs indexing, but I did not come across one as I worked on this implementation. I believe the one method I tried was happy to declare my initial, empty index was correct and up-to-date 😅

@abbottmg
Copy link
Contributor Author

I've rebased this work onto main. A hooks stanza has since been added so I plan to move the new keys under that heading as soon as able

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.

None yet

4 participants