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

Disallow setting order constraints on resources within a group #509

Merged
merged 5 commits into from
May 27, 2022

Conversation

F16shen
Copy link
Contributor

@F16shen F16shen commented May 6, 2022

Found an Issue when sets constraint on resources within a group:
I have a two node cluster and resources like this:

  * Resource Group: test:
    * dummy1	(ocf:pacemaker:Dummy):	 Started node1
    * dummy2	(ocf:pacemaker:Dummy):	 Started node1
    * dummy3	(ocf:pacemaker:Dummy):	 Started node1

and set an order constraint for dummy3 dummy1
pcs constraint order set dummy3 dummy1
It seems ok, but when I tried to move the group to another node, pcs reports error below

[root@node1 pcs]# pcs resource move test node2
Error: Unable to simulate changes in CIB: Transition failed: terminated
An invalid transition was produced

It looks like pacemaker doesn't allow this operation, but pcs has no warning about it(it really confused me until I delete that order constraint).Since resources in a group have their on boot sequence define by their position in the group, perhaps it's better to stop user set order constraint for them.

for now I still some complie issue unresolved, I thike it needs to modify tests but I'm not quite sure how to do it.

ERROR: pcs_test (unittest.loader._FailedTest)
----------------------------------------------------------------------
AttributeError: type object '_FailedTest' has no attribute 'pcs_test'

======================================================================
ERROR: test_returns_export_of_found_elements (pcs_test.tier0.lib.commands.test_constraint_common.ConfigTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/pcs/BUILD/pcs-0.10.12/pcs_test/tier0/lib/commands/test_constraint_common.py", line 198, in test_returns_export_of_found_elements
    {"ids": ["A", "B"], "options": {"role": "Master"}},
  File "/root/pcs/BUILD/pcs-0.10.12/pcs_test/tier0/lib/commands/test_constraint_common.py", line 190, in create
    {"id": "some_id", "symmetrical": "true"},
  File "/root/pcs/BUILD/pcs-0.10.12/pcs/lib/commands/constraint/common.py", line 48, in create_with_set
    resource_set.is_resource_in_group(resource_set_item['ids'], cib)
  File "/root/pcs/BUILD/pcs-0.10.12/pcs/lib/cib/constraint/resource_set.py", line 97, in is_resource_in_group
    resource_section = get_resources(cib)
  File "/root/pcs/BUILD/pcs-0.10.12/pcs/lib/cib/tools.py", line 457, in get_resources
    return sections.get(tree, sections.RESOURCES)
  File "/root/pcs/BUILD/pcs-0.10.12/pcs/lib/cib/sections.py", line 61, in get
    reports.messages.CibCannotFindMandatorySection(section_name)
pcs.lib.errors.LibraryError: ReportItem(severity=ReportItemSeverity(level='ERROR', force_code=None), message=CibCannotFindMandatorySection(section='configuration/resources'), context=None)

Thanks

@knet-ci-bot
Copy link

Can one of the admins verify this patch?

@tomjelinek
Copy link
Member

test this please

Copy link
Member

@tomjelinek tomjelinek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @F16shen,

Thank you for reporting this issue and submitting a pull request.

There are, however, several serious issues with your fix:

  • get_parent_resource returns None for resources which are not in a group, clone or bundle. If you pass that to group.is_group, as you do, it fails with a traceback.
  • resource_set.is_resource_in_group is called from pcs.lib.commands.constrait.common.create_with_set unconditionally and therefore it applies to order constraints (which is good) as well as colocation and ticket constraints (which is not desired).
  • On the other hand, the check is not applied for plain order constraints - pcs.constraint._order_add
  • The check prevents creating a constraint if any resource in the constraint is in an arbitrary group. That seems to be unnecessarily strict. I think it would be sufficient to only disallow creating order constraint which apply to resources that are in the same group.

Also:

  • If you called resource_set.prepare_set first and then is_resource_in_group, you wouldn't need to check if a resource exists. resource_set.prepare_set would do that for you.

@tomjelinek
Copy link
Member

pcs.lib.errors.LibraryError: ReportItem(severity=ReportItemSeverity(level='ERROR', force_code=None), message=CibCannotFindMandatorySection(section='configuration/resources'), context=None)

Sorry about that, it's a bug in tests. I fixed it in #515 and #516.

@F16shen
Copy link
Contributor Author

F16shen commented May 24, 2022

Hi @tomjelinek ,
I have fixed those issues you mentioned before, please check it when you are available.

@F16shen F16shen requested a review from tomjelinek May 24, 2022 09:41
@tomjelinek
Copy link
Member

test this please

@F16shen
Copy link
Contributor Author

F16shen commented May 25, 2022

Hi @tomjelinek ,
I have some problem when I try to get cib in resource_set.is_resource_in_same_group, when I import utils.py in resource_set.py it will cause a circular import in test.
I also tried to get cib when pcs.lib.cib.constraint.constraint.create_with_set is called, It also cause other test failed.
So now I only import it in resource_set.is_resource_in_same_group, is there a better way to do this?

@tomjelinek
Copy link
Member

test this please

@tomjelinek
Copy link
Member

Hi @F16shen, I fixed it for you.

@F16shen
Copy link
Contributor Author

F16shen commented May 26, 2022

Thanks, this really help me a lot

@tomjelinek
Copy link
Member

test this please

@tomjelinek
Copy link
Member

retest this please

1 similar comment
@tomjelinek
Copy link
Member

retest this please

Fix several crashes and bugs, clean up code
@tomjelinek
Copy link
Member

retest this please

@tomjelinek tomjelinek merged commit a324122 into ClusterLabs:main May 27, 2022
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.

3 participants