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

view caching issue in shipit-engine 0.27.1 #911

Open
npaufler opened this issue Jun 19, 2019 · 3 comments
Open

view caching issue in shipit-engine 0.27.1 #911

npaufler opened this issue Jun 19, 2019 · 3 comments

Comments

@npaufler
Copy link

We noticed some odd (and breaking) behavior when we upgraded from shipit-engine 0.26.0 (Rails 5.1) to shipit-engine 0.27.1 (Rails 5.2). @kevinhughes27 and I spent some time this morning debugging exactly what was going on.

When viewing the undeployed commits for a stack, the status icon (...) would not update, showing all the CI tasks for that commit as scheduled and they would never update as the steps completed. What was especially weird was that after the CI completed, the deploy button would change from the greyed out 'pending ci' to a blue 'deploy' button.

If we viewed the statuses table in the backing database, all of the stauses were correct - we saw the scheduled, started, and passed CI steps, with the latter having a success state.

Force refreshing the page, manually refreshing github statuses, etc. didn't have any impact.

We next tried editing _commit.html.erb within the gem (and restarted the server process) by adding <%= commit.status.inspect %> and we saw a few interesting things. First, after the process restarted after changing that view, commits that were previously broken now had the correct results. However, new commits created after the restart still behaved as above.

The contents of the .inspect also looked odd - the status was showing as missing for them, even for ones that showed correctly in the database.

Next, we brought up a rails console and selected one of the commits and called .status on it directly. From the rails console, we saw the correct state - which did not match what the .status.inspect call did from within the web interface.

We were pretty confident it was a caching problem at this point, so we backed out the change to _commit.html.erb and restarted, and all the currently broken commits started working but again, new commits were still broken.

We checked config/environments/production.rb and looked for lines relating to caching, and we saw config.action_controller.perform_caching which was set to true (as it was with the previous version) and tried setting it to false and restarting things. As soon as we did that, everything started working properly, and all commit statuses were rendered properly.

Any thoughts on what is going on? It's clearly some rails caching mechanism, though we are not sure what changed between Rails 5.1 and 5.2, or shipit 0.26 vs 0.27.1. Why does it only impact the commit status display, and not the deploy button?

It's possibly related to this change in rails 5.2 (https://www.engineyard.com/blog/rails-5-2-redis-cache-store) but I'm unsure how.

Thanks!

@ohsabry
Copy link

ohsabry commented Jun 21, 2019

We started off with 0.27.1 (Rails 5.2), and had very similar issues (status icon would not update, statuses table was correct in the database). Everything started working properly when we set config.action_controller.perform_caching to false

wenzowski added a commit to bcgov/cas-shipit that referenced this issue Jul 3, 2019
@davefp
Copy link

davefp commented Aug 19, 2019

The problem is that the contents of _commit.html.erb is wrapped in a fragment cache that only takes into account the updated_at value on the commit itself. This means that changes to the Status and CheckRun objects under the commit won't bust the cache.

I think the simplest way to fix this (which admittedly I haven't tested yet) would be to add touch: true to the belongs_to associations on the commit model.

@davefp
Copy link

davefp commented Aug 19, 2019

Looking more closely at the code I see that the Status and CheckRun classes already implement the custom deferred touch DSL to do what I'm proposing. Back to square one :)

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

No branches or pull requests

3 participants