Unit: invalid configurations accepted when using `processes/spare/0`

[i4ki]

When using spare processes field set to 0, the router doesn't actually restart the app but just validate the config. This makes router accept invalid configurations and hang the application in unexpected ways.

Eg.:

$ curl --unix-socket $UNITSOCK 127.0/config/applications/hello
{
    "type": "python",
    "path": "/vagrant/src/unit-configs/ok/python/hello",
    "module": "wsgi",
    "processes": {
        "spare": 0
    }
}

$ curl --unix-socket $UNITSOCK -XPUT 127.0/config/applications/hello/working_directory --data-binary '"/non-existant-dir"'
{
    "success": "Reconfiguration done."
}

$ curl localhost:6666
.
hangs here...
^C
$

I got this problem in tests (that set spare to zero by default), tests were passing when actually they should not... @andrey-zelenkov

[mar0x]

I think, we have to fix _hanging_ issue, not the _validation_.

[i4ki]

Should the API accept a properly validated but still invalid configuration? In the case of namespaces, there are validations that could only be done in main during the start worker event handler, because of privileges. That's how I discovered this, the tests were passing by sending configurations that should not be accepted (if spare is > 0, the API correctly fail).

[i4ki]

also, I think for consistency, the value of processes.spare should not change the controller API behavior. If spare is 0, we can do an application start and return the reconfiguration error in the API, and then quit it later at the idle timer. This way user will not have a surprise of a reconfiguration been success, but failing later when trying to use the application.

[VBart]

I agree that such application startup check makes behaviour more consistent. But I should note that it doesn't guarantee that just a moment later the app won't fail.

Also this check can be unwanted by a user. The main reason why someone wants to have "spare: 0" is saving resources for an application that should work quite rare. Adding many such applications can be a problem if each one will start at once. On the other hand, this use case probably is too far-fetched and we should add the check.

[i4ki]

But I should note that it doesn't guarantee that just a moment later the app won't fail.

Could you elaborate on that?

Also this check can be unwanted by a user. The main reason why someone wants to have "spare: 0" is saving resources for an application that should work quite rare. Adding many such applications can be a problem if each one will start at once. On the other hand, this use case probably is too far-fetched and we should add the check.

I understand, but even for this case, I think the user should be aware that his new configuration will not be able to spawn the processes when needed. For example, working_directory should not be checked in the controller validation because even if the controller can test directory existence, the application's user could not have permission to read it (and chdir into it).

[i4ki]

ok, I just discovered this config is accepted:

{
    "listeners": {
        "*:6666": {
            "pass": "applications/hello"
        }
    },
    "applications": {
        "hello": {
            "working_directory": "invalid",
            "type": "python",
            "path": "invalid",
            "module": "invalid",
            "user": "invalid",
            "group": "invalid",
            "home": "invalid",
            "processes": {
                "spare": 0
            }
        }
    }
}

$ curl $UNITSOCK -XPUT http://localhost/config --data-binary @../unit-configs/ok/python/hello/config.json
{
    "success": "Reconfiguration done."
}
$ curl localhost:6666
.
hangs
^C
$

I thought it happened just when trying to reconfigure a previously working application that has spare 0, but actually even if it's configured for the first time, we don't check the values.
I didn't expect that.

Is this by design? If that's the case, we should avoid spare: 0 in tests.

[mar0x]

We don't check the 'working_directory' presence during validation.

[i4ki]

I know, I meant: we should not solve this by improving the controller validations (for these cases).

[VBart]

But I should note that it doesn't guarantee that just a moment later the app won't fail.

Could you elaborate on that?

How it works: when a request arrives, the app process is started. The new process handles this request and then it exits after an idle timeout if there are no more requests. So, the app starts and quits, starts and quits, multiple times during a long period of time. At any moment something may be changed with the app (or in the environment, in the system) and what was previously "valid" configuration may now turn invalid and the app fail to start. Among all of these many iterations of starts and quits, we choose the first one and make it more important than others, as our decision about whenever this particular configuration valid or not is purely based on this particular start.

[i4ki]

@VBart Thank you very much for the detailed explanation.

I think makes sense to start the application whenever its config changes in order to give users the assurance that his change at least works now. This is important because the config could have been changed _because_ of an environment change (eg.: running the application as a new user, the app's code or executable could have been moved on disk, changing a database environment variable, etc) and the user wants to validate if it works. If we say success, that's it. If user needs to test an endpoint after applying the config, this will spawn a process anyway.
Yes, as you said, this could not be valid in the future, but only if the environment changes again somehow and I think this is acceptable because users are aware that changing the app's code or the system could break things.

Now talking about tests, checks like this doesn't make sense when the application has _processes.spare_ set to zero.
https://github.com/nginx/unit/blob/master/test/test_go_isolation.py#L25
https://github.com/nginx/unit/blob/65ca2d7b198bf573fe3caf5e75bf6bd3a172820b/test/test_java_application.py#L39

[andrey-zelenkov]

Now talking about tests, checks like this doesn't make sense when the application has processes.spare set to zero.

To check that applications is actually working a GET request is sent after the configuration is loaded. In case if you are unhappy about checking configuration before GET - that's just additional step where any error can occur (but this is not a sufficient condition to pass the test).

[i4ki]

Ok. This was very unexpected to me.

I think this behavior should be documented, otherwise, other users could have troubles like me. As by default Unit do start the application and only return success when it is ready, I was confident to rely on it.

[VBart]

@tiago4orion I believe Andrey's comment was related only for our tests. He found it handy to write them this way. We may change the behaviour and always check the app process on applying configuration to make it more consistent.

[VBart]

Current behaviour is not something specially designed. It's just a straightforward implementation: we have N processes to start. If we fail to start them then we return error. If N is zero, then no processes start, nothing fails.

[andrey-zelenkov]

Yes, I talked about test part only.