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.

Sign up for GitHub

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

Jump to bottom

Colocation constraints with pcs provider are broken, incorrectly assume bidirectionality, swap order of primitives around #150

Closed
fghaas opened this issue Jul 14, 2015 · 3 comments
Closed

Colocation constraints with pcs provider are broken, incorrectly assume bidirectionality, swap order of primitives around #150

fghaas opened this issue Jul 14, 2015 · 3 comments

Comments

@fghaas
Copy link
Contributor

fghaas commented Jul 14, 2015

https://github.com/puppet-community/puppet-corosync/blob/master/lib/puppet/provider/cs_colocation/pcs.rb#L39 says that the order of primitives in colocation constraints does not matter. It does.

<rsc_colocation id="c_rbd_volume2_on_target1" rsc="g_target1" score="INFINITY" with-rsc="g_rbd_volume2"/>
<rsc_colocation id="c_rbd_volume1_on_target1" rsc="g_rbd_volume1" score="INFINITY" with-rsc="g_target1"/>

These two constraints are not identical, but were generated from equivalent cs_colocation resources.
@fghaas fghaas changed the title Colocation constraints are broken, incorrectly assume bidirectionality, swap order of primitives around Colocation constraints with pcs provider are broken, incorrectly assume bidirectionality, swap order of primitives around Jul 14, 2015
fghaas added a commit to fghaas/puppet-corosync that referenced this issue Jul 14, 2015
@fghaas
Issue voxpupuli#150
c312d1d
fghaas added a commit to fghaas/puppet-corosync that referenced this issue Jul 14, 2015
@fghaas
fixup! Issue voxpupuli#150
908f02b
fghaas added a commit to fghaas/puppet-corosync that referenced this issue Jul 14, 2015
@fghaas
Stop ignoring order of primitives in colocation constraints
b161978 
cs_colocation, when used with the pcs provider, makes the incorrect
assumption that the order of primitives in colocation constraints is
arbitrary. It is not, and the following must not be equivalent:

cs_colocation { 'vip_with_service':
  primitives => [ 'nginx_vip', 'nginx_service' ],
}

cs_colocation { 'vip_with_service':
  primitives => [ 'nginx_service', 'nginx_vip' ],
}

These mean different things. The first example ensures that nginx_vip
runs on whatever node nginx_service runs on, and if nginx_service
can't be started on any node, Pacemaker won't even attempt to start
nginx_vip. However, if nginx_vip cannot be started on any node,
nginx_service will continue to be available.

The second example means the opposite. In it, nginx_service is
configured to run wherever nginx_vip runs, and if nginx_vip can't run
anywhere, nginx_service won't start. And if nginx_vip cannot be
started on any node, nginx_service will not be available.

As a result, drop artificial sorting for the primitives, and use them
exactly as specified.

Fixes issue voxpupuli#150.
@fghaas fghaas mentioned this issue Jul 14, 2015
Closed
fghaas added a commit to fghaas/puppet-iscsi-rbd-ha that referenced this issue Jul 14, 2015
@fghaas
Switch puppet-corosync repository to fghaas
969a951 
Includes fix for
voxpupuli/puppet-corosync#150
fghaas added a commit to fghaas/puppet-iscsi-rbd-ha that referenced this issue Jul 15, 2015
@fghaas
Switch puppet-corosync repository to fghaas
836caf1 
Includes fix for
voxpupuli/puppet-corosync#150
@fghaas fghaas mentioned this issue Jul 15, 2015
Merged
fghaas added a commit to fghaas/puppet-iscsi-rbd-ha that referenced this issue Jul 15, 2015
@fghaas
Switch puppet-corosync repository to fghaas
9883ff0 
Includes fix for
voxpupuli/puppet-corosync#150
@fghaas
Copy link
Contributor Author

fghaas commented Jul 16, 2015

Note that the author of the cs_primitive type realized that colocation constraints are directional:

    newproperty(:primitives, :array_matching => :all) do
      desc "At least two Pacemaker primitives to be located together. Order of primitives
        in colocation groups is important. In Pacemaker, a colocation of 2 primitives
        behaves different than a colocation between more than 2 primitives. Here the
        behavior is altered to be more consistent.

The cs_colocation PCS provider author apparently disregarded that information.

fghaas added a commit to fghaas/puppet-corosync that referenced this issue Jul 20, 2015
@fghaas
Stop ignoring order of primitives in colocation constraints
c8c1cf5 
cs_colocation, when used with the pcs provider, makes the incorrect
assumption that the order of primitives in colocation constraints is
arbitrary. It is not, and the following must not be equivalent:

cs_colocation { 'vip_with_service':
  primitives => [ 'nginx_service', 'nginx_vip' ],
}

cs_colocation { 'vip_with_service':
  primitives => [ 'nginx_vip', 'nginx_service' ],
}

These mean different things. The first example ensures that nginx_vip
runs on whatever node nginx_service runs on, and if nginx_service
can't be started on any node, Pacemaker won't even attempt to start
nginx_vip. However, if nginx_vip cannot be started on any node,
nginx_service will continue to be available.

The second example means the opposite. In it, nginx_service is
configured to run wherever nginx_vip runs, and if nginx_vip can't run
anywhere, nginx_service won't start. But if nginx_service cannot be
started, nginx_vip is still available.

Note that the provider's expected order is directly reversed compared
to the one that standalone Pacemaker would use (no matter whether pcs
or crm is used).

At any rate, for the pcs provider the order of resources is wrong, and
injecting the sort to mask that issue is nonsense. As a result, drop
artificial sorting for the primitives, fix the order, and use them
exactly as specified.

Fixes issue voxpupuli#150.
fghaas added a commit to fghaas/puppet-corosync that referenced this issue Jul 20, 2015
@fghaas
Stop ignoring order of primitives in colocation constraints
275e8ba 
cs_colocation, when used with the pcs provider, makes the incorrect
assumption that the order of primitives in colocation constraints is
arbitrary. It is not, and the following must not be equivalent:

cs_colocation { 'vip_with_service':
  primitives => [ 'nginx_service', 'nginx_vip' ],
}

cs_colocation { 'vip_with_service':
  primitives => [ 'nginx_vip', 'nginx_service' ],
}

These mean different things. The first example ensures that nginx_vip
runs on whatever node nginx_service runs on, and if nginx_service
can't be started on any node, Pacemaker won't even attempt to start
nginx_vip. However, if nginx_vip cannot be started on any node,
nginx_service will continue to be available.

The second example means the opposite. In it, nginx_service is
configured to run wherever nginx_vip runs, and if nginx_vip can't run
anywhere, nginx_service won't start. But if nginx_service cannot be
started, nginx_vip is still available.

Note that the provider's expected order is directly reversed compared
to the one that standalone Pacemaker would use (no matter whether pcs
or crm is used). This is intentional as per commit 8e218d9.

At any rate, for the pcs provider the order of resources is wrong, and
injecting the sort to mask that issue is nonsense. As a result, drop
artificial sorting for the primitives, fix the order, and use them
exactly as specified.

Fixes issue voxpupuli#150.
@igalic
Copy link
Contributor

igalic commented Oct 14, 2015

@fghaas, @mkrakowitzer are you interested in providing your patches as pull request(s)?

@fghaas
Copy link
Contributor Author

fghaas commented Oct 14, 2015

@igalic See #153.

fghaas added a commit to fghaas/puppet-corosync that referenced this issue Nov 2, 2015
@fghaas
Stop ignoring order of primitives in colocation constraints
8ad3f7d 
cs_colocation, when used with the pcs provider, makes the incorrect
assumption that the order of primitives in colocation constraints is
arbitrary. It is not, and the following must not be equivalent:

cs_colocation { 'vip_with_service':
  primitives => [ 'nginx_service', 'nginx_vip' ],
}

cs_colocation { 'vip_with_service':
  primitives => [ 'nginx_vip', 'nginx_service' ],
}

These mean different things. The first example ensures that nginx_vip
runs on whatever node nginx_service runs on, and if nginx_service
can't be started on any node, Pacemaker won't even attempt to start
nginx_vip. However, if nginx_vip cannot be started on any node,
nginx_service will continue to be available.

The second example means the opposite. In it, nginx_service is
configured to run wherever nginx_vip runs, and if nginx_vip can't run
anywhere, nginx_service won't start. But if nginx_service cannot be
started, nginx_vip is still available.

Note that the provider's expected order is directly reversed compared
to the one that standalone Pacemaker would use (no matter whether pcs
or crm is used). This is intentional as per commit 8e218d9.

At any rate, for the pcs provider the order of resources is wrong, and
injecting the sort to mask that issue is nonsense. As a result, drop
artificial sorting for the primitives, fix the order, and use them
exactly as specified.

Fixes issue voxpupuli#150.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@roidelapluie @igalic @fghaas