Allow setting cache loader = boot on staging and never reload the config [THREESCALE-756] #651

davidor · 2018-03-12T15:44:37Z

APIcast crashes when it starts with:

THREESCALE_DEPLOYMENT_ENV: staging
APICAST_CONFIGURATION_LOADER: boot
APICAST_CONFIGURATION_CACHE: ''

Error msg:

[lua] configuration_loader.lua:139: init(): cache is off, cannot store configuration, exiting

This is because by default, the staging deployment env sets configuration_cache to 0, which is incompatible with the 'boot' mode. This PR solves the issue by avoiding setting that default.

Ref: https://issues.jboss.org/browse/THREESCALE-756

mikz · 2018-03-12T16:31:42Z

gateway/src/apicast/cli/command/start.lua

+local function default_config_cache(options, context, configuration_loader)
+    local configuration_cache = options.cache or context.configuration_cache
+
+    if not configuration_cache and configuration_loader == 'lazy' then


Can't we delegate this to the environment configurations ?

Yes, that's better 👍

mikz · 2018-03-12T18:21:11Z

gateway/src/apicast/cli/command/start.lua

@@ -110,10 +110,15 @@ local function openresty_binary(candidates)
 end

 local function build_env(options, config, context)
+    local configuration_cache = options.cache or context.configuration_cache
+    if configuration_cache then -- Avoid calling tostring() on nil, would return "nil".
+        configuration_cache = tostring(configuration_cache)


Maybe it is not necessary to call tostring here because it is done in the update_env function?

I think it's needed. The result of this method is not only used in update_env, it's also used in the call to exec at the end of mt:__call.

https://github.com/3scale/apicast/blob/30984b947c3abaef6335d5dbb591e0136c07f247/gateway/src/apicast/cli/command/start.lua#L174

But nil keys are being removed from the table, so it won't even get there.

update_env does not mutate the table that it receives in the params. so if we do not apply tostring here, it will never be applied before it reaches that exec.

exec does not call tostring on the value: https://github.com/3scale/lua-resty-execvp/blob/bfa07e3f82fdd8bfb41be5c152438f03ccd60b9f/src/resty/execvp.lua#L48
and update_env does: https://github.com/3scale/apicast/blob/30984b947c3abaef6335d5dbb591e0136c07f247/gateway/src/apicast/cli/command/start.lua#L47

that means the values in this table should be either a string or nil.
lua tables don't have nil values, so assigning nil will make the key disappear so it is never iterated by pairs

Not sure I follow. I probably didn't explain this well. Let me try:

build_env constructs a table t and returns it. We're discussing whether we should call tostring in some values of t here or not. I think we should.

The table t returned in the previous step is sent to update_env. But that function does not mutate t.

t is sent as a param of the call to exec. As 2) didn't modify it, it will be sent as we left it in 1). That's why we need to call tostring() in build_env. Otherwise, exec might receive a table that contains things that are not strings.

Looks like I was really confused 😄

We are saying the same. This table now needs to contain nil or string and that is what you are ensuring.

I think it would be good if we could clean it up by changing the exec to serialize the env:
https://github.com/3scale/lua-resty-execvp/blob/bfa07e3f82fdd8bfb41be5c152438f03ccd60b9f/src/resty/execvp.lua#L48

Then we don't have to really think about the serialization anymore.

Right. I think that fixes the root problem: 3scale/lua-resty-execvp#1

mikz · 2018-03-12T18:23:27Z

@davidor Can we try to test this? Would blackbox test with env do the trick?

davidor · 2018-03-12T20:41:00Z

@mikz : I tried and it didn't work. I've been investigating, and I found that THREESCALE_DEPLOYMENT_ENV is not respected in the integration tests. config/production.lua is used instead of config/staging.lua even with TREESCALE_DEPLOYMENT_ENV=staging.

It's read here when it hasn't been set yet:
https://github.com/3scale/apicast/blob/69ff05c03e934daf1fa3a9b2cfb4231773b5a22a/gateway/src/apicast/cli/command/start.lua#L184

I'll try to fix it.

mikz · 2018-03-13T06:22:46Z

@davidor it should be possible to set the environment_file block: 3scale/Test-APIcast@f5451e4

davidor · 2018-03-13T16:02:25Z

@mikz It turns out that it was easier than I thought, using $ENV instead of env_to_apicast was enough, as you said. Check the new regression test. I verified that it fails in master.

mikz · 2018-03-13T16:38:02Z

gateway/src/apicast/configuration_loader.lua

@@ -60,7 +60,7 @@ end
 _M.mock = mock_loader.save

 local function ttl()
-  return tonumber(env.get('APICAST_CONFIGURATION_CACHE'), 10)
+  return tonumber(env.get('APICAST_CONFIGURATION_CACHE') or 0, 10)


env.get can return empty string, so it should do the or after tonumber or use env.value ?

Right. Changed 👍

mikz · 2018-03-13T16:38:26Z

t/configuration-loading-boot-staging.t

+$ENV{APICAST_CONFIGURATION_LOADER} = 'boot';
+$ENV{APICAST_CONFIGURATION_CACHE} = '';
+
+filters { configuration => 'fixture=echo.json' };


The new version of lua-resty-execvp makes sure that tostring() is called in the envs that it receives via params, so we no longer need to worry about it.

mikz

Awesome 👍

octobot assigned davidor Mar 12, 2018

davidor changed the title ~~Allow setting cache loader = boot on staging and never reload the config~~ [WIP] Allow setting cache loader = boot on staging and never reload the config Mar 12, 2018

davidor force-pushed the allow-config-on-boot-and-never-reload-staging branch 2 times, most recently from deb6ca7 to 75dd839 Compare March 12, 2018 16:02

mikz reviewed Mar 12, 2018

View reviewed changes

davidor force-pushed the allow-config-on-boot-and-never-reload-staging branch 2 times, most recently from 388eabd to fa4c034 Compare March 12, 2018 17:36

davidor changed the title ~~[WIP] Allow setting cache loader = boot on staging and never reload the config~~ Allow setting cache loader = boot on staging and never reload the config Mar 12, 2018

davidor requested a review from mikz March 12, 2018 17:52

mikz reviewed Mar 12, 2018

View reviewed changes

mikz changed the title ~~Allow setting cache loader = boot on staging and never reload the config~~ Allow setting cache loader = boot on staging and never reload the config [THREESCALE-756] Mar 13, 2018

davidor force-pushed the allow-config-on-boot-and-never-reload-staging branch 2 times, most recently from 9fdf001 to 30984b9 Compare March 13, 2018 16:29

mikz reviewed Mar 13, 2018

View reviewed changes

davidor added 2 commits March 13, 2018 18:10

Allow getting config just once when loader is boot and env is staging

17fbd01

t: add regression test for boot mode in staging env without caching ttl

72b814a

davidor force-pushed the allow-config-on-boot-and-never-reload-staging branch from 30984b9 to 72b814a Compare March 13, 2018 17:10

davidor added 2 commits March 14, 2018 14:54

Roverfile.lock: bump lua-resty-execvp to v0.1.1-1

8f39a1f

cli/command/start: delete non-needed tostring() calls

19e4d16

The new version of lua-resty-execvp makes sure that tostring() is called in the envs that it receives via params, so we no longer need to worry about it.

davidor requested a review from mikz March 14, 2018 15:07

mikz approved these changes Mar 14, 2018

View reviewed changes

davidor merged commit 24922ef into master Mar 14, 2018

davidor deleted the allow-config-on-boot-and-never-reload-staging branch March 14, 2018 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting cache loader = boot on staging and never reload the config [THREESCALE-756] #651

Allow setting cache loader = boot on staging and never reload the config [THREESCALE-756] #651

davidor commented Mar 12, 2018

mikz Mar 12, 2018

davidor Mar 12, 2018

mikz Mar 12, 2018

davidor Mar 13, 2018

davidor Mar 13, 2018

mikz Mar 13, 2018

davidor Mar 13, 2018

mikz Mar 13, 2018

davidor Mar 13, 2018

mikz Mar 13, 2018

davidor Mar 14, 2018

mikz commented Mar 12, 2018

davidor commented Mar 12, 2018

mikz commented Mar 13, 2018

davidor commented Mar 13, 2018 •

edited

mikz Mar 13, 2018

davidor Mar 13, 2018

mikz Mar 13, 2018

mikz left a comment

Allow setting cache loader = boot on staging and never reload the config [THREESCALE-756] #651

Allow setting cache loader = boot on staging and never reload the config [THREESCALE-756] #651

Conversation

davidor commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz commented Mar 12, 2018

davidor commented Mar 12, 2018

mikz commented Mar 13, 2018

davidor commented Mar 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz left a comment

Choose a reason for hiding this comment

davidor commented Mar 13, 2018 •

edited