Skip to content
This repository has been archived by the owner on Apr 21, 2023. It is now read-only.

standby: add standby mode #1365

Merged
merged 6 commits into from Jan 25, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 14 additions & 7 deletions src/ngx_pagespeed.cc
Expand Up @@ -1152,13 +1152,17 @@ char* ps_merge_srv_conf(ngx_conf_t* cf, void* parent, void* child) {
delete cfg_s->options;
cfg_s->options = NULL;

if (cfg_s->server_context->global_options()->enabled()) {
if (!cfg_s->server_context->global_options()->unplugged()) {
// Validate FileCachePath
GoogleMessageHandler handler;
const char* file_cache_path =
cfg_s->server_context->config()->file_cache_path().c_str();
if (file_cache_path[0] == '\0') {
return const_cast<char*>("FileCachePath must be set");
if (!cfg_s->server_context->global_options()->standby()) {
return const_cast<char*>("FileCachePath must be set, even for standby");
} else {
return const_cast<char*>("FileCachePath must be set");
}
} else if (!cfg_m->driver_factory->file_system()->IsDir(
file_cache_path, &handler).is_true()) {
return const_cast<char*>(
Expand Down Expand Up @@ -1844,11 +1848,6 @@ ngx_int_t ps_resource_handler(ngx_http_request_t* r,
options = cfg_s->server_context->global_options();
}

if (!options->enabled()) {
// Disabled via query params or request headers.
return NGX_DECLINED;
}

request_context->set_options(options->ComputeHttpOptions());

// ps_determine_options modified url, removing any ModPagespeedFoo=Bar query
Expand All @@ -1875,6 +1874,14 @@ ngx_int_t ps_resource_handler(ngx_http_request_t* r,
response_category == RequestRouting::kGlobalAdmin ||
response_category == RequestRouting::kCachePurge;

// Normally if we're disabled we won't handle any requests, but if we're in
// standby mode we do want to handle requests for .pagespeed. resources.
if (options->unplugged() ||
(!options->enabled() && !pagespeed_resource)) {
// Disabled via query params or request headers.
return NGX_DECLINED;
}

if (!html_rewrite) {
// create request ctx
CHECK(ctx == NULL);
Expand Down
17 changes: 10 additions & 7 deletions src/ngx_rewrite_options.cc
Expand Up @@ -192,15 +192,18 @@ RewriteOptions::OptionScope NgxRewriteOptions::GetOptionScope(

RewriteOptions::OptionSettingResult NgxRewriteOptions::ParseAndSetOptions0(
StringPiece directive, GoogleString* msg, MessageHandler* handler) {
if (IsDirective(directive, "on")) {
set_enabled(RewriteOptions::kEnabledOn);
} else if (IsDirective(directive, "off")) {
set_enabled(RewriteOptions::kEnabledOff);
} else if (IsDirective(directive, "unplugged")) {
set_enabled(RewriteOptions::kEnabledUnplugged);
} else {
EnabledEnum enabled;
if (!ParseFromString(directive, &enabled)) {
return RewriteOptions::kOptionNameUnknown;
}
if (enabled == RewriteOptions::kEnabledOff) {
// In ngx_pagespeed, for historical reasons, we treat "off" as "unplugged".
// Also, "off" is deprecated and people should be using "standby" or
// "unplugged" now depending on which sense they want. See comment on
// RewriteOptions::EnabledEnum.
enabled = RewriteOptions::kEnabledUnplugged;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am kinda wondering if the mod_pagespeed parser shouldn't do the same and we shouldn't kill the Off enum... Seems a little easier to think of this way, even if it would risk making some of the parsing redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea! Let me try that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Getting rid of EnabledOff turns out to be pretty hard. We have a nice option parsing abstraction that isn't compatible with the idea that "off" should map to one value in one server and another value in another server. It would be possible to extend this, but I don't think it's worth it seeing as once we're done parsing the options everything just asks about enabled() and unplugged() and doesn't see the enums.

}
set_enabled(enabled);
return RewriteOptions::kOptionOk;
}

Expand Down
36 changes: 36 additions & 0 deletions test/pagespeed_test.conf.template
Expand Up @@ -132,6 +132,42 @@ http {
"@@SERVER_ROOT@@/mod_pagespeed_example/styles/";
}

server {
listen @@SECONDARY_PORT@@;
listen [::]:@@SECONDARY_PORT@@;
server_name pagespeed-off.example.com;
pagespeed FileCachePath "@@FILE_CACHE@@";
pagespeed off;
pagespeed EnableFilters collapse_whitespace;
}

server {
listen @@SECONDARY_PORT@@;
listen [::]:@@SECONDARY_PORT@@;
server_name pagespeed-on.example.com;
pagespeed FileCachePath "@@FILE_CACHE@@";
pagespeed on;
pagespeed EnableFilters collapse_whitespace;
}

server {
listen @@SECONDARY_PORT@@;
listen [::]:@@SECONDARY_PORT@@;
server_name pagespeed-standby.example.com;
pagespeed FileCachePath "@@FILE_CACHE@@";
pagespeed standby;
pagespeed EnableFilters collapse_whitespace;
}

server {
listen @@SECONDARY_PORT@@;
listen [::]:@@SECONDARY_PORT@@;
server_name pagespeed-unplugged.example.com;
pagespeed FileCachePath "@@FILE_CACHE@@";
pagespeed unplugged;
pagespeed EnableFilters collapse_whitespace;
}

pagespeed UseNativeFetcher "@@NATIVE_FETCHER@@";
@@RESOLVER@@

Expand Down
2 changes: 1 addition & 1 deletion testing-dependencies/mod_pagespeed