Skip to content
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

Time function #119

Closed
wants to merge 7 commits into from
Closed

Time function #119

wants to merge 7 commits into from

Conversation

aecay
Copy link
Collaborator

@aecay aecay commented Mar 2, 2020

Add a concept of "time function" to mite. This is a function that is run in the controller alongside the main controller logic. It receives two asyncio.Event objects: one which it can use to gate the start of the test, and one it can use to listen for the end of the test. We are able to port the previously added "logging webhook" feature to be a generic time function. We have other use cases in our private testing code as well. One is at https://github.com/sky-uk/id-mite-nft/blob/8e8f8e10672d0903aceb253d81dd600fe8a65390/mite_id/ims/journeys.py#L200 (where we purge rabbit queues at the start of a test; currently we just use the beginning of the scenario function for this) and for our upcoming chaos testing deliverables (where we want to run not only at the beginning of the test, but during it as well).

Main changes:

  • implementation logic for the above
  • add a decorator time_function to the mite top-level module. When applied to a scenario function, it marks the scenario to be used with the time function.
  • port the logging webhook to be a time function instead (now in lib/logging_webhook.py)
  • add some unit tests for this functionality (principally, the logic of asyncio.Event sequencing)

Consequent changes:

  • mite/__init__.py: add underscore prefixes to private names
  • move the controller logic from __main__.py to a new file cli/controller.py
  • refactor ScenarioManager to know the spec of the scenario it represents. Controller objects no longer need to know this; they can read it from their ScenarioManager.
  • refactor the mite controller and mite (journey|scenario) test CLI commands to use the same code path. This allows changes like this one introducing time functions to be made in only one place, rather than two.

Outstanding tasks:

  • documentation
  • consider whether, for the sake of modularity/composability, we should allow multiple time functions to be specified.
  • consider whether to allow specifying a time function on the command line and/or via environment variables, in addition to as a decorator
  • consider whether there's a better name than "time function" for this feature

@jb098
Copy link
Collaborator

jb098 commented Mar 2, 2020

One of the problems I see with this are around seperation of concerns. The controller's job should be to control tasks and not run them in my opinion. While it may seem like a bit of a cludge to add something to the beginning of a scenario to do something like cleanup data from an environment, that is still a 'task' which is run according to some sort of schedule.

There is also the concern that the controller is one of the only bottlenecking areas in mite, the more we put into it, the more we risk bringing that bottleneck closer to the inevitable having to deal with it stage.

I'd maybe like to chat about the problem we're trying to solve, perhaps if we're looking to trigger behaviour based on some event listener, we could explore if a change to the the logic in the runners might make a better place for it by swapping the volume model out for something else? I'm not sure whether that's appropriate as it seems you care very much about when a 'test' starts and ends which the controller does understand best.

Let's just think about it a little is all I'm saying :)

@jb098
Copy link
Collaborator

jb098 commented Mar 2, 2020

I also agree with the sentiments around naming 😄

@aecay
Copy link
Collaborator Author

aecay commented Mar 3, 2020

Closing this PR for now. As we discussed, we're going to do this a different way, via changes to the volume model abstraction. That will allow us to keep tasks on the runners (not the controller) and also bring a fix for #114 into the scope of the resolution as well.

@aecay aecay closed this Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants