fix location constraint bug 1791791 #562
Conversation
|
Can one of the admins verify this patch? |
|
test this please |
pcs/constraint.py
Outdated
| report_list = [] | ||
| if node not in existing_nodes: | ||
| if warn : | ||
| return False |
There was a problem hiding this comment.
- This function returns a list of ReportItem objects. It must never return
False. - Instead of calling a function and passing it an argument to tell the function to do nothing, it is better not to call the function at all.
- The function doesn't seem to be called with
warnparameter anywhere. What's the parameter purpose than?
pcs/constraint.py
Outdated
| @@ -1195,6 +1197,19 @@ def location_rule(lib, argv, modifiers): | |||
| "resource-discovery": None, | |||
| }, | |||
| ) | |||
|
|
|||
| if options["node"] == None or not options["node"]: | |||
| warn("There's no actual 'location' specified in this constraint, you can specify it by node. ") | |||
There was a problem hiding this comment.
This is a misunderstanding of how location constraints with rules work.
There is no node option. The node is specified by the rule itself. For example, rule node_size gte 42 applies to all nodes which have the value of their attribute node_size greater than or equal to 42.
If, however, there are only date expression in the rule, like shown in #235, then the rule applies to all nodes and is therefore pointless in a location constraint.
You can check pcs.lib.cib.rule.tools to see how to analyze a rule to figure out if it contains node expressions. The module, however, is part of pcs library, new architecture, and is not compatible with the rule parser used in location constraints.
pcs/rule.py
Outdated
| if dom_element.hasAttribute("node"): | ||
| dom_element.removeAttribute("node") | ||
| #if dom_element.hasAttribute("node"): | ||
| # dom_element.removeAttribute("node") |
There was a problem hiding this comment.
I'm not sure what is the point of this change.
pcs/rule.py
Outdated
| @@ -22,7 +22,7 @@ def parse_argv(argv, extra_options=None): | |||
| """ | |||
| Commandline options: no options | |||
| """ | |||
| options = {"id": None, "role": None, "score": None, "score-attribute": None} | |||
| options = {"id": None, "role": None, "node":None, "score": None, "score-attribute": None} | |||
There was a problem hiding this comment.
I'm not sure what is the point of this change. As explained in the other comment, there is no node option in location constraints.
|
Thank for your reply. When I see this issue, I thought it was mean to add a [node] option for rule command to make sure the specific resource can run in the node under the rule. |
The correct way is to analyze the constraint rule and check if it contains any node expressions ( |
|
Ok, thanks for your explanation. I will modify it later. |
Hi:
In this Pr, I made some fixes for the #235