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

Varnish cache BAN on update #536

Closed
wants to merge 33 commits into from
Closed

Varnish cache BAN on update #536

wants to merge 33 commits into from

Conversation

expede
Copy link
Member

@expede expede commented Jul 2, 2021

  • Purge on app modify
  • Purge on app destroy
  • Purge on user modify
  • Purge on user destroy
  • Confirm working on staging

Server Logs

Screen Shot 2021-10-01 at 22 48 51

Screen Shot 2021-10-01 at 22 49 33

I'd show more, but it's kind of hard to capture in screenshots ("Here it an app before, and here it is after a push!").

Setting Expectations

Because of the way Varnish bans work, it can often take about a minute to actually cache bust. The end result is a refreshed page, you just sometimes have to be patient.

If the cache purge fails, we log and continue. It doesn't seem worth reporting a 502 over, since our worst case is that a page will at worst appear to update slowly and we can always manually cache bust. We'll see these in Sentry if they start to pile up.

@expede expede requested a review from walkah July 2, 2021 23:52

NGINX.purgeMany urls >>= \case
Left err -> return $ openLeft err
Right _ -> return $ Right appId
Copy link
Member Author

Choose a reason for hiding this comment

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

This entire section will feel much cleaner when we switch to Rescue

@expede expede marked this pull request as ready for review July 3, 2021 00:00
@expede expede added 💗 enhancement New feature or request 🚠 infrastructure What we use to run our services 🛑 blocked labels Jul 3, 2021
@expede
Copy link
Member Author

expede commented Sep 3, 2021

Now that https://github.com/fission-suite/infrastructure/pull/73 is deployed, we can test that this works!

@expede expede changed the base branch from main to generics September 10, 2021 03:57
@expede
Copy link
Member Author

expede commented Sep 10, 2021

Rebased on top of #547

Base automatically changed from generics to main September 11, 2021 04:26
@expede expede changed the title Purge NGINX cache BLOCKED Purge NGINX cache Sep 15, 2021
@expede expede changed the title BLOCKED Purge NGINX cache Purge Varnish cache Oct 1, 2021
@expede
Copy link
Member Author

expede commented Oct 2, 2021

You can really tell when I'm debugging stupid typos on staging by number of close commits 😛

@expede
Copy link
Member Author

expede commented Oct 9, 2021

@walkah this is updated with BAN. It looks like that change hasn't been deployed to the Varnish on staging yet (BAN is 405ing via curl). Any reason we shouldn't flip that over?

@expede expede changed the title Purge Varnish cache Varnish cache BAN on update Oct 9, 2021
@expede expede marked this pull request as draft October 9, 2021 17:57
@walkah
Copy link
Member

walkah commented Oct 15, 2021

@walkah this is updated with BAN. It looks like that change hasn't been deployed to the Varnish on staging yet (BAN is 405ing via curl). Any reason we shouldn't flip that over?

Oops! I had swapped it out to test something else... put it back now!

@expede expede added this to the 🦾 Infrastructure Reliabilty milestone Oct 27, 2021
@expede expede self-assigned this Oct 27, 2021
@expede
Copy link
Member Author

expede commented Oct 28, 2021

Confirmed that BAN is accepted over curl 👍 I haven't been able to test it on our domains yet because...

I'm currently bumping into a problem with the message we get from the remote ipfs stat for the file size calculation. I suppose that it's possible that the format changed in 0.9.1, but then why is it not broken on main/prod?

I thought I could just push this over the line quickly, but no such luck. It's late here, so I'm going to look at it in more depth tomorrow 🌅

@expede
Copy link
Member Author

expede commented Oct 29, 2021

Okay I have the code working. It was a mix of that changed response from IPFS a while back, but then I was treating a numeric field as a string for... reasons? To make iterating on that kind of thing easier, I've moved the ipfs-haskell package into this repo.

Possibly worth noting that the BAN can be pretty slow. My testing here today has quite a few over a minute, often hovering around 90 seconds after the upload completes on a barebones HTML file. I'm not sure if we can make that more aggressive?

@expede
Copy link
Member Author

expede commented Oct 29, 2021

@walkah both ☝️ and...

Screen Shot 2021-10-28 at 23 36 34

Also just merged another PR. Rebasing... 👇

@expede expede marked this pull request as ready for review October 29, 2021 06:42
@expede
Copy link
Member Author

expede commented Oct 29, 2021

Talked it through with @walkah

The performance hit on write with the Varnish cache makes us nervous. We're going to wait and see what comes out of improving our IPFS setup directly first. We still have this code around if we want to add our own caches for larger users with immutable data (which we're currently throwing paid AWS caches in front of)

@expede
Copy link
Member Author

expede commented Nov 2, 2021

Backported the important parts

@expede expede deleted the purge-nginx branch October 30, 2022 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💗 enhancement New feature or request 🚠 infrastructure What we use to run our services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants