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

(Updated) Upgrade app to 4.3 #161

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

Conversation

abbottmg
Copy link
Contributor

@abbottmg abbottmg commented Feb 2, 2025

This is additional commits to #157 to address some suggested changes from this autumn. It also addresses issue #158 by removing the assets precompilation job.

It is related to #95 as the upgrade instructions from 4.2.x to 4.3.x requires running a rebuild on elasticsearch (it recommends --only=accounts which that PR does not yet support. I will update that soon.

Caveat that I have not yet deployed this to my production instance. Additional changes may come in as I attempt to do so over the next few days.

@timetinytim
Copy link
Contributor

@abbottmg This looks good to me, thank you!

I checked with Renaud about the failing chewy upgrade job. Technically since we have the tootctl search deploy for this, we can get rid of this job altogether, which should allow this PR to start passing the tests. How the chart handles elasticsearch rebuilds is still something that's a bit up in the air in any case 🙂

@abbottmg
Copy link
Contributor Author

abbottmg commented Feb 6, 2025

I checked with Renaud about the failing chewy upgrade job. Technically since we have the tootctl search deploy for this, we can get rid of this job altogether, which should allow this PR to start passing the tests. How the chart handles elasticsearch rebuilds is still something that's a bit up in the air in any case 🙂

I know it's my own PR and all but I do think #95 is the right answer, perhaps with some very visible documentation about how it is a heavy enough job to only enable when you absolutely need it. Its goal was precisely to handle situations like this where we need to re/build search as part of a large upgrade.

@timetinytim
Copy link
Contributor

@abbottmg I've been thinking about that PR ever since I saw this one, and honestly, I think you're right. Considering what's possible with how the indexes are rebuilt, I don't see a better option. As long as we make sure it's disabled by default, and there's plenty of documentation explaining how expensive it is / how it should be used (i.e. as a one-time flag for certain deploys) then I think it should be just fine.

Once this is merged I'll move on to that one 🙂

@abbottmg
Copy link
Contributor Author

abbottmg commented Feb 6, 2025

@timetinytim sounds like a reasonable plan to me!

On that note, I just did a 3-version deployment using this branch. My results:

  • From 4.2.10 -> 4.2.12: web and sidekiq deployments rolled out with no discernible downtime BUT the streaming image could not update because the main project only began tagging with application versions with 4.3.0.
  • -> 4.3.0: went off without a hitch. For my small instance (subscribed to a couple fire-hose relays) it took about 15 minutes to rebuild the accounts index.
  • -> 4.3.3: zero downtime

The rolling update's a big success, with the note that I do run 2 replicas of both web and streaming. I bring up that 15 minute duration simply as an example of how expensive that job would be, and so docs also include recommendation to increase helm's timeout from 5 minutes.

Regarding the missing streaming image, I recommend asking the upstream project to back-tag images for 4.2.x and 4.1.x until both minor versions are EOS. If that's not feasible, I think I'd prefer complicating the deployment-streaming template over dropping support for < 4.3 or writing documentation asking users to blank the streaming.image and .tag fields.

@timetinytim
Copy link
Contributor

@abbottmg As far as I'm aware, the separate streaming image is only 4.3+, as I think there are additional changes that wouldn't be compatible in 4.2 and 4.1 (I believe, I might not be 100% accurate, but that's my understanding), so we unfortunately can't make a pre-4.3 streaming image.

I would tend to agree that adding additional logic or documentation is the preferred way to do this for now, especially since a fair amount are still pre-4.3. In this situation, I personally would only add documentation mentioning that the separate streaming image is 4.3 and up, but I'm not against additional logic to make it nicer than that.

@timetinytim timetinytim self-assigned this Feb 10, 2025
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