-
-
Notifications
You must be signed in to change notification settings - Fork 747
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
Add single sensor mode to the sensor container service / process #4179
Conversation
…ion) to the sensor container. When running in this mode, it's assumed that sensor process life cycle and failover is handled by an external service so the sensor process container exits immediately when a server crashes. This is useful in scenarios where sensor process life cycle and failover is handled by external service such as kubernetes.
when running in single sensor mode.
Checked, that looks like what we need from the description 👍 Also sounds like sensor_partitioner is irrelevant in this mode? Lines 262 to 263 in 28129d4
Since in both cases sensor ref is already specified explicitly. I will test/play more closely with the package produced from this build later next week. |
help='Only run sensor with the provided reference. Value is of the form pack.sensor-name.') | ||
other_opts = [ | ||
cfg.BoolOpt( | ||
'single_sensor_mode', default=False, |
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.
I'm also open to a better name for this option.
Correct, right now when I can modify the code a bit to at least log a message in this case since right now we just ignore partitioner setting in such scenarios without logging anything. |
partitioner is used.
--single-user-mode flag to the process.
@@ -41,16 +41,19 @@ | |||
|
|||
def get_sensors_partitioner(): | |||
if cfg.CONF.sensor_ref: | |||
LOG.info('Running in single sensor mode, using a single sensor partitioner...') |
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.
👍
…kStorm/st2 into sensor_container_single_sensor_mode
integration tests and correctly set up the environment on Travis.
…ner_single_sensor_mode
@armab Tests are now in place and this PR is ready to be reviewed although I'm still battling with getting integration tests to work on Travis under Python 3. |
…kStorm/st2 into sensor_container_single_sensor_mode
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.
Thanks for adding this!
I tested the feature in my local K8s env and it worked well 👍
👍 |
Just a future note for myself, when I'll need to productionize that in Docker & K8s: exec /opt/stackstorm/st2/bin/st2sensorcontainer \
--config-file /etc/st2/st2.conf \
--single-sensor-mode \
--sensor-ref=githubwebhook.GitHubWebhookSensor |
I don't think we should merge this for 2.8.0 but instead bundle all HA changes for 2.9.0. |
@lakshmi-kannan It doesn't negatively affect things (or change the behavior) in any way, so I see no reason to not merge it. At the very least, if we decide to postpone the merge we should pick out and merge Python 3 fixes and changes. But again, as mentioned above, I see no reason for waiting with merging this (if we want to postpone including this from product / announcing perspective we can simply skip the changelog entry / add it behind the feature flag, but I think that's really an over kill for such a simple change). |
@armab Yeah, you can also set single sensor mode via config option, but I personally prefer the command line flag since it's more explicit and visible compared to the config file option. |
under Python 3. Input body is actually bytes under Python 3, but we except and work with text type (str / unicode) so we need to convert from bytes to unicode.
…kStorm/st2 into sensor_container_single_sensor_mode
@Kami If you feel strongly and you are sure it won't break anything with sensor behavior today, I am +1 to shipping this. I looked at the code and I don't expect it to break existing behavior. Please validate this and you have my +1. However, splitting timers into own process though isn't functionally different, I will wait until after 2.8 to ship it. |
@lakshmi-kannan Yeah, I agree about the timer change - it's more involved since it includes a new service and packaging + installation changes. |
This pull request adds single sensor mode to the sensor container service (enabled via
sensorcontainer.single_sensor_mode
config option).When this flag is set, sensor container process doesn't manage lifecycle of a sensor processes and respawn them if a sensor exits / crashes, but it exits itself and propagates sensor exit code to the parent sensor container process.
This comes handy in scenarios when a sensor life cycle and failover is handled by an external service such as Kubernetes / Mesos / monitoring + bash scripts / ...
Note: This flag needs to be used in combination with
--sensor-ref
CLI option since without it it doesn't make sense.TODO