-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
azure - small container host bug fixes #6416
azure - small container host bug fixes #6416
Conversation
@@ -554,7 +583,8 @@ def test_unload_policy_file_with_bad_schema(self, | |||
with open(file_path, 'w') as f: | |||
f.write(policy_string) | |||
|
|||
host.unload_policy_file(file_path, {}) | |||
host.load_policy(file_path, host.policies) |
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.
Was this test just not doing anything before since it didn't load the bad policy?
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.
The code path it was testing might have been the same, but it was just luck with the implementation (loading a policy with a bad schema is almost the same as not loading one at all). I fixed it to explicitly do what it says it is doing.
resource: azure.resourcegroup | ||
""") | ||
|
||
host.load_policy(file_path, host.policies) |
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.
Why is there no error when loading an incorrect policy schema? Does the user know a policy was never loaded?
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.
We do log errors for the various failure types when we load the policy but we don't fail on them since this is headless and needs to just keep working for all the other policies.
The tests for these edge cases were all from scenarios where you have a single yaml
file that contains both valid and non-valid policies. When we update or remove that file we go unregister jobs for all the policies that were in it, and there were various ways the lookups could fail if a subset of policies never got loaded in the first place.
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.
Silly question (it's been awhile) - but is there an easy way to look up what policies are running in a container? Other than looking at the YAML file?
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.
Yeah today you'd basically just look at the storage container to see what YAML is in there, or look at the logs or app insights as it traces out the list and their schedules periodically. Would be nice if it had a little endpoint you could interrogate.
Four fixes here:
unload_policy_file
was attempting to unload policies which were never loaded (usually due to the policy YAML being invalid). This could make it impossible to unload other policies which were contained in the same file as an invalid policy. Remove the exception handler and instead update the list comprehension with the appropriate check.run_policies_for_event
was case sensitive on identifying the event, and the actual casing of events fired in azure is often unpredictable....ProcessPoolExecutor
can be problematic on Windows. UseThreadPoolExecutor
on Windows (primarily for Windows developers as we don't ship this in a Windows container anyway).