-
Notifications
You must be signed in to change notification settings - Fork 32
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
admin.py: optionally check for system manager environment variable #246
Conversation
test_admin.py requires OMERODIR to be set for all tests due to an autouse fixture https://github.com/ome/omero-py/blob/b1a627eba9a7dce7880645fd0a42d85949851ff7/test/unit/clitest/test_admin.py#L51-L52
src/omero/plugins/admin.py
Outdated
@@ -721,6 +721,20 @@ def _descript(self, args): | |||
% os.path.sep.join(["etc", "grid", __d__])) | |||
return descript | |||
|
|||
def _requires_service_manager(self, config): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this should also apply to omero-web, I could see this in a mixing superclass like the other superclasses for the AdminControl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you thinking something in https://github.com/ome/omero-py/blob/master/src/omero/cli.py since that's where DiagnosticsControl
is, e.g. ServiceManagerControl
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the property name would be different so it would have to be a parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class AdminControl(ServiceManagerControl, ...):
SERVICE_MANAGER_KEY = "server"
or whatever, yeah.
Barring the introduction of the new class name suffix ( |
I went for a mixin because it only provides a utility method for internal plugin use, it didn't seem right to construct a full Control object. Need to decide between consistency vs gradual improvement. Once this is released I'd start by adding this to our OMERO playbooks through these role vars:
And similarly for OMERO.web If there are no unforeseen issues we can consider making this mandatory in a future release of the roles. |
Does Does
If you did set |
The latter. There's no easy way to tell whether omero was started by systemd (or any other service manager), so the idea of this workaround is to set a variable in the systemd config that wouldn't otherwise be set in a normal environment. You're correct that if you manually set that variable you'd bypass the check in |
How about:
|
Good, thanks. |
If everyones happy with this it'd be nice to include it in #248 but it's not essential. |
a1175d6
to
21dc745
Compare
WFM. 👍 |
Adds a config property that can be set to the name of an environment variable that is only set when running under systemd or some other service manager.
omero admin start|stop
will check this variable and abort if it's not set.Example using systemd
If you have systemd v232+ the
INVOCATION_ID
environment variable should be set by systemd so you can check for this https://serverfault.com/a/927481If you have an older version of systemd you can define your own non-empty variable in your systemd service file:
If you then attempt to start/stop omero outside systemd:
Note since the omero configuration is updated from
/opt/omero/server/config/*.omero
you must've started omero at least once with systemd after updating the configuration.I'll document this property in https://github.com/ome/openmicroscopy/blob/develop/etc/omero.properties if everyone is happy with this.
Includes a test, though I had to put it in a new file because
test_admin.py
contains a global autouser fixture that requiresOMERODIR
, which means it's not tested on Travis.A similar property can be added to
omero web
. Since these are decoupled I don't think they should share the same property, instead it would beomero.web.servicemanager.checkenv
.@stick
Note this was updated so the property is now
omero.server.servicemanager.checkenv
, notomero.admin.servicemanager.checkenv