Fix for Issue 447 #450

timdeluxe · 2018-09-20T13:12:45Z

Pull Request (PR) description

Fixes the issue (#447), that resource_discovery parameter on cs_location is never set, when using provider crm. The provider did not set the feature "discovery" and also had no method for it. Just simply setting the feature would not be good, since it is first available since pacemaker 1.1.13, so i included a version check.

This Pull Request (PR) fixes the following issues

Fixes #447

Updating fork

The crm provider for cs_location did never set that. A version check ensures that parameter is only set when running pacemakerd >= 1.1.13 Fixes voxpupuli#447

bastelfreak · 2018-09-23T08:58:07Z

Hi @timdeluxe. thanks for the patch. Can you take a look at the failing travis tests?

timdeluxe · 2018-09-23T11:32:30Z

@bastelfreak I have no clue yet about the other failures, only about the 2nd one: It expects the resource discovery parameter on RedHat systems (obviously because those use the pcs provider) and do not expect it on non RedHat systems (because those usually use the crm provider).
Code here:

puppet-corosync/spec/acceptance/cs_location_spec.rb

Lines 109 to 115 in 90169bb

    
           if fact('osfamily') != 'RedHat' 
        
             it 'creates a location without resource-discovery=exclusive on non-RH systems' do 
        
               shell('cibadmin --query | grep duncan_vip_there') do |r| 
        
                 expect(r.stdout).not_to match(%r{resource-discovery="exclusive"}) 
        
               end 
        
             end 
        
           end

However that resource-discovery feature is not bound to the provider (crm or pcs), but more to the pacemaker version, where my fix is for. That said i am still unsure how to rewrite the acceptance test. Either the pacemakerd version is also gathered in the acceptance test similar to the patch i provided or we find out which OS families and versions provide pacemakerd >= 1.1.13. What do you think?

Corrected it. Still fixes voxpupuli#447

lib/puppet/provider/cs_location/crm.rb

bastelfreak · 2018-10-14T08:49:56Z

@timdeluxe I don't have a preference here. A fact would probably be nice, as it could be used in other places as well.

(my fault). Adding rescue to prevent issues when command is not yet found on fresh systems, where pacemaker is not yet installed. Still fixes voxpupuli#447

Still fixes voxpupuli#447

timdeluxe · 2018-10-16T11:21:50Z

@timdeluxe I don't have a preference here. A fact would probably be nice, as it could be used in other places as well.

Okay, i will try to do that in the next days, sorry for the constantly failing builds so far... :-/

Updating own fork

and acceptance tests according to it. Still trying to fix voxpupuli#447

Still trying to fix voxpupuli#447

timdeluxe · 2019-05-08T11:01:22Z

@bastelfreak Sorry for doing nothing for so long.

As you can see i introduced a fact for the pacemaker version, but now there is a racing condition in the beaker tests. The facts are gathered at the beginning and since no pacemaker is installed there yet, no pacemakerd_version is given. This leads to a failing test for the resource-discovery stuff.

Maybe the tests may be fixable, but i think this could also raise a problem in the real world, because the resource-discovery feature would require two puppet runs to successfully get applied (first run: pacemakerd_version fact empty because no pacemaker is installed yet -> location would be created without resource-discovery parameter; second run: pacemakerd_version now filled and location gets resource-discovery parameter added, if pacemakerd_version is >= 1.1.13).
I would suggest to revert the idea with the pacemakerd_version fact and trying to let the previous implementation (gathering the pacemakerd version within the provider) passing the tests.

However i don't know how to fix the acceptance tests then. As said previously redhat vs. non-redhat is definitly wrong. Would it be feasible to check the OS and the major version there and see, which one has pacemaker version >= 1.1.13 and which < 1.1.13, rather than trying to check the version itself directly? Would that be a valid way to do it?

vox-pupuli-tasks · 2020-05-01T11:30:32Z

Dear @timdeluxe, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs?
If you need any help, you can reach out to us on our IRC channel voxpupuli on Freenode or our Slack channel voxpupuli at slack.puppet.com.
You can find my sourcecode at voxpupuli/vox-pupuli-tasks

dploeger · 2020-07-06T07:46:45Z

@bastelfreak Can you help @timdeluxe here? We're still running on this branch and would like to use the official module.

igalic

my suggested change isn't great, but it's better than being stuck with this pr staying open forever

lib/puppet/provider/cs_location/crm.rb

…m provider. Updating acceptance tests to work correctly with resource-discovery. Still trying to fix voxpupuli#447

timdeluxe · 2020-07-09T17:46:12Z

Yes, failing tests i know. Sorry. :-/
I executed the beaker tests locally, but as it now turns out wrongly. Now i found out how to do them correctly and can test it locally. Will troubleshoot it.

igalic

🙋🏻‍♀️

lib/puppet/provider/cs_location/crm.rb

…uest hints. Fixing setting of discovery parameter. Still trying to fix voxpupuli#447

…OS as proposed in voxpupuli#455 to fix beaker tests

timdeluxe · 2020-07-10T10:12:03Z

Finally 😌
Tests are green now. Needed to fix other stuff on cs_order, which was not related to this issue at all. Also i needed to implement the proposed workaround of #455, since centos tests were failing on travis as well on my local machine because of that. I made single commits for all of that, so that it is cleanly separated.

I hope the PR can be merged now, if not please highlight what is missing. Thanks!

igalic

more 👀

lib/puppet/provider/cs_location/crm.rb

lib/puppet/provider/cs_order/crm.rb

lib/puppet/type/cs_order.rb

…rder score parameter.

igalic

👍

…ovider.

ekohl · 2020-07-10T17:07:17Z

lib/puppet/provider/cs_location/crm.rb

+  # we need to check if we run at least pacemakerd version 1.1.13 before enabling feature discovery
+  # see http://blog.clusterlabs.org/blog/2014/feature-spotlight-controllable-resource-discovery
+  begin
+    pacemakerd_version_string = pacemakerd('--version')


Is this executed on the agent or during catalog compilation?

It is executed on the agent.
(We are already using this code in our environment. If it would be executed during compilation it would be done on the puppetserver, where there is no pacemakerd installed and we then wouldn't have the discovery feature, which we have.)

igalic · 2020-07-24T06:35:45Z

an we merge this?

bastelfreak · 2020-07-24T06:52:01Z

@igalic looks good to me, but I am far from being an expert for this.

timdeluxe · 2020-08-30T14:47:11Z

@igalic @bastelfreak @ekohl Is anything holding back from merging this? If not, please do so. If yes, please tell me, so i can work on it.

igalic · 2020-08-30T14:51:49Z

let's see what happens if we do merge it

timdeluxe and others added 2 commits September 18, 2018 17:31

Merge pull request #1 from voxpupuli/master

052bcc4

Updating fork

Fixing setting of resource_discovery parameter.

6ecabc1

The crm provider for cs_location did never set that. A version check ensures that parameter is only set when running pacemakerd >= 1.1.13 Fixes voxpupuli#447

Command for the fix was defined in wrong way.

6e090a0

Corrected it. Still fixes voxpupuli#447

bastelfreak reviewed Oct 14, 2018

View reviewed changes

lib/puppet/provider/cs_location/crm.rb Outdated Show resolved Hide resolved

bastelfreak mentioned this pull request Oct 14, 2018

Implement option to disable watchdog device use of corosync #451

Closed

timdeluxe added 2 commits October 16, 2018 11:25

Fixing command back to correct ruby 1.9 syntax

354b276

(my fault). Adding rescue to prevent issues when command is not yet found on fresh systems, where pacemaker is not yet installed. Still fixes voxpupuli#447

Improving code for case when version is nil

09c8aac

Still fixes voxpupuli#447

timdeluxe and others added 4 commits May 7, 2019 16:40

Merge pull request #2 from voxpupuli/master

1bf6b51

Updating own fork

Introducing custom fact for pacemakerd version and changing providers

aa87e89

and acceptance tests according to it. Still trying to fix voxpupuli#447

Fixing rubocop issues

5e8b2c1

Still trying to fix voxpupuli#447

Fixing rubocop issues

7af4a0e

Still trying to fix voxpupuli#447

vox-pupuli-tasks bot added the tests-fail label May 1, 2020

igalic suggested changes Jul 6, 2020

View reviewed changes

lib/puppet/provider/cs_location/crm.rb Show resolved Hide resolved

timdeluxe added 3 commits July 9, 2020 15:45

Merge remote-tracking branch 'upstream/master'

a3505f8

Merge branch 'master' into issue-447

386632f

Removing custom fact and adding direct command execution back into cr…

9c3b04c

…m provider. Updating acceptance tests to work correctly with resource-discovery. Still trying to fix voxpupuli#447

igalic suggested changes Jul 9, 2020

View reviewed changes

lib/puppet/provider/cs_location/crm.rb Outdated Show resolved Hide resolved

lib/puppet/provider/cs_location/crm.rb Outdated Show resolved Hide resolved

timdeluxe added 4 commits July 10, 2020 11:22

Updating pacemaker version check for discovery feature after pull req…

4621d8f

…uest hints. Fixing setting of discovery parameter. Still trying to fix voxpupuli#447

Fixing score/kind setting on cs_order

9ee500c

Implementing workaround for hanging service restarts on RedHat-based …

26b81c0

…OS as proposed in voxpupuli#455 to fix beaker tests

Fixing code style highlighted by rubocop on cs_order crm provider

503a96f

igalic suggested changes Jul 10, 2020

View reviewed changes

lib/puppet/provider/cs_location/crm.rb Show resolved Hide resolved

lib/puppet/provider/cs_order/crm.rb Outdated Show resolved Hide resolved

lib/puppet/provider/cs_order/crm.rb Outdated Show resolved Hide resolved

lib/puppet/type/cs_order.rb Outdated Show resolved Hide resolved

Fixing kind handling on cs_order crm provider. Fixing wording on cs_o…

a173a12

…rder score parameter.

igalic approved these changes Jul 10, 2020

View reviewed changes

Fixing cs_order_crm unit tests after fixing kind stuff in cs_order pr…

8e15000

…ovider.

ekohl reviewed Jul 10, 2020

View reviewed changes

vox-pupuli-tasks bot removed the tests-fail label Jul 22, 2020

bastelfreak approved these changes Jul 24, 2020

View reviewed changes

igalic merged commit 802dd4b into voxpupuli:master Aug 30, 2020

timdeluxe deleted the issue-447 branch September 1, 2020 09:02

zpeschke mentioned this pull request Dec 21, 2020

cs_order fails with pcs provider because of score parameter #485

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for Issue 447 #450

Fix for Issue 447 #450

timdeluxe commented Sep 20, 2018

bastelfreak commented Sep 23, 2018

timdeluxe commented Sep 23, 2018

bastelfreak commented Oct 14, 2018

timdeluxe commented Oct 16, 2018

timdeluxe commented May 8, 2019

vox-pupuli-tasks bot commented May 1, 2020

dploeger commented Jul 6, 2020

igalic left a comment

timdeluxe commented Jul 9, 2020

igalic left a comment

timdeluxe commented Jul 10, 2020

igalic left a comment

igalic left a comment

ekohl Jul 10, 2020

timdeluxe Jul 10, 2020

igalic commented Jul 24, 2020

bastelfreak commented Jul 24, 2020

timdeluxe commented Aug 30, 2020

igalic commented Aug 30, 2020

Fix for Issue 447 #450

Fix for Issue 447 #450

Conversation

timdeluxe commented Sep 20, 2018

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

bastelfreak commented Sep 23, 2018

timdeluxe commented Sep 23, 2018

bastelfreak commented Oct 14, 2018

timdeluxe commented Oct 16, 2018

timdeluxe commented May 8, 2019

vox-pupuli-tasks bot commented May 1, 2020

dploeger commented Jul 6, 2020

igalic left a comment

Choose a reason for hiding this comment

timdeluxe commented Jul 9, 2020

igalic left a comment

Choose a reason for hiding this comment

timdeluxe commented Jul 10, 2020

igalic left a comment

Choose a reason for hiding this comment

igalic left a comment

Choose a reason for hiding this comment

ekohl Jul 10, 2020

Choose a reason for hiding this comment

timdeluxe Jul 10, 2020

Choose a reason for hiding this comment

igalic commented Jul 24, 2020

bastelfreak commented Jul 24, 2020

timdeluxe commented Aug 30, 2020

igalic commented Aug 30, 2020