)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"317ff6626be33a98effef173801cd9f87c8b02e4","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"policy: Don\u0027t persist default rule changes in tests"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"\u0027oslo.policy\u0027 expects a list of default rules be provided when"},{"line_number":10,"context_line":"initialising the enforcer. Unfortunately, it demonstrates a nasty habit"},{"line_number":11,"context_line":"of modifying these defaults once it has them [1]. This can result in"},{"line_number":12,"context_line":"configuration from one test bleeding through to the next."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Resolve this by initialising the enforcer with a copy of the default"},{"line_number":15,"context_line":"rules, rather than the originals, allowing \u0027oslo.policy\u0027 to do whatever"},{"line_number":16,"context_line":"it likes to the rules. Ultimately this should probably be fixed in"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3946099e_13602083","line":13,"range":{"start_line":9,"start_character":0,"end_line":13,"end_character":0},"updated":"2021-01-26 19:32:01.000000000","message":"yeah this is issue and oslo policy should work on their own copy of rules not original.\n\nBut I am still not understanding that with placement_policy.reset() in policy fixture how this is happening ?","commit_id":"c22a19a326a511383e34eed3a8297adfd59fe83e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"97d9dd439b16485ee9fb72830648b37bf2a2c064","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"policy: Don\u0027t persist default rule changes in tests"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"\u0027oslo.policy\u0027 expects a list of default rules be provided when"},{"line_number":10,"context_line":"initialising the enforcer. Unfortunately, it demonstrates a nasty habit"},{"line_number":11,"context_line":"of modifying these defaults once it has them [1]. This can result in"},{"line_number":12,"context_line":"configuration from one test bleeding through to the next."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Resolve this by initialising the enforcer with a copy of the default"},{"line_number":15,"context_line":"rules, rather than the originals, allowing \u0027oslo.policy\u0027 to do whatever"},{"line_number":16,"context_line":"it likes to the rules. Ultimately this should probably be fixed in"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"ef20f41a_07127378","line":13,"range":{"start_line":9,"start_character":0,"end_line":13,"end_character":0},"in_reply_to":"3946099e_13602083","updated":"2021-01-26 23:18:46.000000000","message":"i guess oslo policy is not working on a copy\n\npython memory modele is best tought of as pass by pointer.\n\nits not pass by value where a copy is passed to the a function or pass by reference when the orginal is passed.\n\nits pass by pointer since if you assing to an object that is passed in the \"porinter\" is just repointed\nsuch that the name given to the parmater not refercnes the new object but is you assing to a filed of paremater it derefrenbces the pointer and modifce the thing that was passed in.\n\nso unless it did copy.deepcopy(default rules) internally oslo policy is not working on a copy.\n\nyou can do that by doing the copy your self or with a code change to oslo.polcy","commit_id":"c22a19a326a511383e34eed3a8297adfd59fe83e"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"4868694eb9761ea51b193a0088cee839e781f59b","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"policy: Don\u0027t persist default rule changes in tests"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"\u0027oslo.policy\u0027 expects a list of default rules be provided when"},{"line_number":10,"context_line":"initialising the enforcer. Unfortunately, it demonstrates a nasty habit"},{"line_number":11,"context_line":"of modifying these defaults once it has them [1]. This can result in"},{"line_number":12,"context_line":"configuration from one test bleeding through to the next."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Resolve this by initialising the enforcer with a copy of the default"},{"line_number":15,"context_line":"rules, rather than the originals, allowing \u0027oslo.policy\u0027 to do whatever"},{"line_number":16,"context_line":"it likes to the rules. Ultimately this should probably be fixed in"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"ef41d1ab_008cbd42","line":13,"range":{"start_line":9,"start_character":0,"end_line":13,"end_character":0},"in_reply_to":"6ffb50c8_74264a1b","updated":"2021-02-02 19:01:02.000000000","message":"yeah, self.check changes is kept over to on original object which is what is being checked in rule checks. Thanks for clarification and patience :).","commit_id":"c22a19a326a511383e34eed3a8297adfd59fe83e"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"e8f8946d565434e8ca6f233d76e7ea4f179a81eb","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"policy: Don\u0027t persist default rule changes in tests"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"\u0027oslo.policy\u0027 expects a list of default rules be provided when"},{"line_number":10,"context_line":"initialising the enforcer. Unfortunately, it demonstrates a nasty habit"},{"line_number":11,"context_line":"of modifying these defaults once it has them [1]. This can result in"},{"line_number":12,"context_line":"configuration from one test bleeding through to the next."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Resolve this by initialising the enforcer with a copy of the default"},{"line_number":15,"context_line":"rules, rather than the originals, allowing \u0027oslo.policy\u0027 to do whatever"},{"line_number":16,"context_line":"it likes to the rules. Ultimately this should probably be fixed in"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"6ffb50c8_74264a1b","line":13,"range":{"start_line":9,"start_character":0,"end_line":13,"end_character":0},"in_reply_to":"934bfad0_a56b0538","updated":"2021-01-27 21:24:43.000000000","message":"I think I\u0027ve confused you by giving \u0027check_str\u0027 as the example above. It\u0027s not the \u0027RuleDefault.check_str\u0027 value that changes, it\u0027s the \u0027RuleDefault.check\u0027 value, which is a parsed version of the \u0027check_str\u0027 [1]. That modification happens at [2], which I linked in the commit message below, if there\u0027s a \u0027deprecated_rule\u0027 attribute, the \u0027check_str\u0027 values don\u0027t match, and \u0027[oslo_policy] enforce_new_defaults\u0027 is set to False. Basically it ORs together the deprecated rule\u0027s \u0027check_str\u0027 value. The fact that it doesn\u0027t update \u0027check_str\u0027 after is a mistake and something I agree we should fix, however, there\u0027s definitely a bug here that we need to fix. If you want to debug this yourself, look at the bug when I demonstrated my wonderful printf-debugging skills 😄\n\n[1] https://github.com/openstack/oslo.policy/blob/3.6.0/oslo_policy/policy.py#L1177\n[2] https://github.com/openstack/oslo.policy/blob/3.6.0/oslo_policy/policy.py#L762-L764","commit_id":"c22a19a326a511383e34eed3a8297adfd59fe83e"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"9c88f0dd0736121c683b482bcba6f297d1da9aef","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"policy: Don\u0027t persist default rule changes in tests"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"\u0027oslo.policy\u0027 expects a list of default rules be provided when"},{"line_number":10,"context_line":"initialising the enforcer. Unfortunately, it demonstrates a nasty habit"},{"line_number":11,"context_line":"of modifying these defaults once it has them [1]. This can result in"},{"line_number":12,"context_line":"configuration from one test bleeding through to the next."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Resolve this by initialising the enforcer with a copy of the default"},{"line_number":15,"context_line":"rules, rather than the originals, allowing \u0027oslo.policy\u0027 to do whatever"},{"line_number":16,"context_line":"it likes to the rules. Ultimately this should probably be fixed in"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"934bfad0_a56b0538","line":13,"range":{"start_line":9,"start_character":0,"end_line":13,"end_character":0},"in_reply_to":"c7c30d33_795e5eef","updated":"2021-01-27 19:11:19.000000000","message":"Yeah if you modify the check_str then it is issue. I understand the concept of modifying on object. But in oslo.policy nothing modify the check_str on registered default rules. oslo polocy only modify the self.check[1] which is different attribute prepared locally and different[2] from what service defined like check_str or scope etc. So registered rule check_str does not get modified in original object at any time.\n\nI ran few of the scenario and did not find that registered original rule check_str is modified with policy run, it stay same what service define. did you get tests conflict due to change in rule enforcement because of rule original check_str is changed by oslo polocy?\n\n[1] https://github.com/openstack/oslo.policy/blob/e103baa002e54303b08630c436dfc7b0b8a013de/oslo_policy/policy.py#L762 \n[2] https://github.com/openstack/oslo.policy/blob/e103baa002e54303b08630c436dfc7b0b8a013de/oslo_policy/policy.py#L1176-L1177","commit_id":"c22a19a326a511383e34eed3a8297adfd59fe83e"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"b5b639fc6e5d386623a4d114e35b28938f4e736b","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"policy: Don\u0027t persist default rule changes in tests"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"\u0027oslo.policy\u0027 expects a list of default rules be provided when"},{"line_number":10,"context_line":"initialising the enforcer. Unfortunately, it demonstrates a nasty habit"},{"line_number":11,"context_line":"of modifying these defaults once it has them [1]. This can result in"},{"line_number":12,"context_line":"configuration from one test bleeding through to the next."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Resolve this by initialising the enforcer with a copy of the default"},{"line_number":15,"context_line":"rules, rather than the originals, allowing \u0027oslo.policy\u0027 to do whatever"},{"line_number":16,"context_line":"it likes to the rules. Ultimately this should probably be fixed in"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"c7c30d33_795e5eef","line":13,"range":{"start_line":9,"start_character":0,"end_line":13,"end_character":0},"in_reply_to":"ef20f41a_07127378","updated":"2021-01-27 13:40:11.000000000","message":"Yup. Consider this example:\n\n  \u003e\u003e\u003e class Enforcer:\n  ...     def __init__(self, rules):\n  ...             self.rules \u003d rules\n  ...     def reset(self):\n  ...             self.rules \u003d None\n  ...     def set_rules(self, rules):\n  ...             self.rules \u003d rules\n  ... \n  \u003e\u003e\u003e class Rule:\n  ...     def __init__(self, check_str):\n  ...             self.check_str \u003d check_str\n  ... \n  \u003e\u003e\u003e rules \u003d [Rule(\u0027foo\u0027), Rule(\u0027bar\u0027)]\n  \u003e\u003e\u003e enforcer \u003d Enforcer(rules)\n  \u003e\u003e\u003e enforcer.rules\n  [\u003c__main__.Rule object at 0x7fc08099eca0\u003e, \u003c__main__.Rule object at 0x7fc08099ed30\u003e]\n  \u003e\u003e\u003e [r.check_str for r in enforcer.rules]\n  [\u0027foo\u0027, \u0027bar\u0027]\n  \u003e\u003e\u003e enforcer.rules[0].check_str \u003d \u0027baz\u0027\n  \u003e\u003e\u003e [r.check_str for r in enforcer.rules]\n  [\u0027baz\u0027, \u0027bar\u0027]\n  \u003e\u003e\u003e enforcer.reset()\n  \u003e\u003e\u003e enforcer.rules\n  \u003e\u003e\u003e enforcer.set_rules(rules)\n  \u003e\u003e\u003e [r.check_str for r in enforcer.rules]\n  [\u0027baz\u0027, \u0027bar\u0027]\n\nThe \u0027Rule\u0027 objects are persistent, which means if we modify them inside the enforcer then those changes are persistent.","commit_id":"c22a19a326a511383e34eed3a8297adfd59fe83e"}],"placement/tests/unit/policy_fixture.py":[{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"317ff6626be33a98effef173801cd9f87c8b02e4","unresolved":true,"context_lines":[{"line_number":33,"context_line":"        super(PolicyFixture, self).setUp()"},{"line_number":34,"context_line":"        policy_file \u003d paths.state_path_def(\u0027etc/placement/policy.yaml\u0027)"},{"line_number":35,"context_line":"        self.conf_fixture.config(group\u003d\u0027oslo_policy\u0027, policy_file\u003dpolicy_file)"},{"line_number":36,"context_line":"        placement_policy.reset()"},{"line_number":37,"context_line":"        # because oslo.policy has a nasty habit of modifying the default rules"},{"line_number":38,"context_line":"        # we provide, we must pass a copy of the rules rather then the rules"},{"line_number":39,"context_line":"        # themselves"}],"source_content_type":"text/x-python","patch_set":1,"id":"b31fae47_82536ea4","line":36,"range":{"start_line":36,"start_character":0,"end_line":36,"end_character":32},"updated":"2021-01-26 19:32:01.000000000","message":"should not this take care of the cleaning the things for each tests which internally call Enforcer\u0027s clean ?\n https://github.com/openstack/oslo.policy/blob/0a228dea2ee96ec3eabed3361ca22502d0bbd4a1/oslo_policy/policy.py#L570","commit_id":"c22a19a326a511383e34eed3a8297adfd59fe83e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"97d9dd439b16485ee9fb72830648b37bf2a2c064","unresolved":true,"context_lines":[{"line_number":33,"context_line":"        super(PolicyFixture, self).setUp()"},{"line_number":34,"context_line":"        policy_file \u003d paths.state_path_def(\u0027etc/placement/policy.yaml\u0027)"},{"line_number":35,"context_line":"        self.conf_fixture.config(group\u003d\u0027oslo_policy\u0027, policy_file\u003dpolicy_file)"},{"line_number":36,"context_line":"        placement_policy.reset()"},{"line_number":37,"context_line":"        # because oslo.policy has a nasty habit of modifying the default rules"},{"line_number":38,"context_line":"        # we provide, we must pass a copy of the rules rather then the rules"},{"line_number":39,"context_line":"        # themselves"}],"source_content_type":"text/x-python","patch_set":1,"id":"dc99a282_b93ac12a","line":36,"range":{"start_line":36,"start_character":0,"end_line":36,"end_character":32},"in_reply_to":"b31fae47_82536ea4","updated":"2021-01-26 23:18:46.000000000","message":"no this would set the rules to {}\nbut if any of the test modifed the rule object returned by policies.list_rules()\n\nthose modifed rules would persist.\n\npolicies.list_rules()\nwas previousl implemented as \n\n    return itertools.chain(\n        base.list_rules(),\n        resource_provider.list_rules(),\n        resource_class.list_rules(),\n        inventory.list_rules(),\n        aggregate.list_rules(),\n        usage.list_rules(),\n        trait.list_rules(),\n        allocation.list_rules(),\n        allocation_candidate.list_rules(),\n        reshaper.list_rules(),\n    )\n\neach of those list_rules() calls just return a module level global list of rule objects\n\nso that was returning an interator over a set of global lists.\n\nif you modified any of the item pointed too by that iterator it would modify the global state and \nthat would preciste for the lifetime of that python interperter instance.\n\nmenaing if you were running the test in muliple cores depeneng on what test ran on the current thread previously you might get different behavior.","commit_id":"c22a19a326a511383e34eed3a8297adfd59fe83e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"97d9dd439b16485ee9fb72830648b37bf2a2c064","unresolved":true,"context_lines":[{"line_number":40,"context_line":"        placement_policy.init("},{"line_number":41,"context_line":"            self.conf_fixture.conf,"},{"line_number":42,"context_line":"            suppress_deprecation_warnings\u003dTrue,"},{"line_number":43,"context_line":"            rules\u003dcopy.deepcopy(policies.list_rules()))"},{"line_number":44,"context_line":"        self.addCleanup(placement_policy.reset)"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"    @staticmethod"}],"source_content_type":"text/x-python","patch_set":1,"id":"00760b3a_a401b943","line":43,"range":{"start_line":43,"start_character":17,"end_line":43,"end_character":54},"updated":"2021-01-26 23:18:46.000000000","message":"the deep copy here will prevent that sharing of global state although you could have done\n\ncopy.deepcopy(list(policies.list_rules()))\n\ninstead of modifying list_rules to do return list(itertootls.chain(...))\n\nthat would at least limit the cases where you copy the lists.\n\nim also not sure if we are actully copying the data twice here but its defintly not going to share state anymore","commit_id":"c22a19a326a511383e34eed3a8297adfd59fe83e"}]}