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

Improved maintenance page handling of 503 errors [release_2.0] #2202

Merged
merged 3 commits into from
Aug 4, 2022

Conversation

treydock
Copy link
Contributor

@treydock treydock commented Aug 3, 2022

Fixes #2196

<%- @maintenance_ip_whitelist.each do |ip| -%>
RewriteCond %{REMOTE_ADDR} !^<%= escape_ip(ip) %>
<%- end -%>
RewriteRule ^.*$ <%= @public_uri %>/maintenance/index.html [R=503,L]
ErrorDocument 503 <%= @public_uri %>/maintenance/index.html
Header Set Cache-Control "max-age=0, no-store"
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to see how this behaves in 2.0. 2.1 already had this removed - but 2.0 didn't.

I keep deferring this test, but basically we want to be sure that the maintenance page isn't being cached forever in 2.1. By removing the Header Set Cache-Control are we running into a similar situation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested on webdev02. I removed the whitelist for VPN so I didn't have to disconnect. But I loaded OnDemand, touched maintenance.enable, reloaded (not full reload, just click refresh button) and got maintenance page then removed the enable and reloaded again and got dashboard.

So one thing I think this change might break is the URL for maintenance page goes from staying like /pun/sys/dashboard to being forced to /public/maintenance. So if a user refreshes, they won't go back to dashboard, they must modify URL. I think one way we could ease that issue is update maintenance page with very clear link to get back to OnDemand. We'd also need to update OSC's specific page as we override OOD default page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might have a solution in mind to fix the URL issue. I am building new nightlies for dev and will test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So tested on dev with nightly I just built for Gitlab, enable maintenance and get this:

https://ondemand-dev.osc.edu/public/maintenance/index.html

Added a few lines to maintenance directory

    RewriteCond /etc/ood/maintenance.enable !-f
    ReWriteRule ^.*$ /

Removed maintenance file and reloaded, back to OnDemand. I'll open 2.1 pull request and update this pull request.

@treydock treydock requested a review from johrstrom August 4, 2022 15:08
Copy link
Contributor

@johrstrom johrstrom left a comment

Choose a reason for hiding this comment

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

Thanks for testing manually.

@treydock treydock merged commit 090cadc into release_2.0 Aug 4, 2022
@treydock treydock deleted the better-maint-2.0 branch August 4, 2022 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants