[ClusterLabs] [ClusterLabsu] [patch][crmsh] Rework function next_nodeid.

[Jan Pokorný]

Andrei,

On 06/04/16 14:39 +0300, Andrei Maruha wrote:
> Attached patch contains a little bit reworked function next_nodeid> 
> 
> [...]

there are two better aligned channels to propose patches (ordered
by preference, at least judging based on
https://github.com/ClusterLabs/crmsh/pulls?q=is%3Apr+is%3Aclosed):

1. pull request against https://github.com/ClusterLabs/crmsh

2. patch (per git conventions, which is abided here) sent to
   developers at c.o ML, with appropriately labeled topic so that
   the respective upstream folks can hook on easily (in this case,
   the prefix should contain "crmsh"; see how I modified the topic,
   along with cross-posting to the correct list)

There is a reason behind having two mailing lists, different audience
being the most prominent one (true, devels will likely read both,
but the rest of "users" would be better off without such a traffic)

Thanks for understanding.

-- Jan

> From 56d99aa764abb2af8d638425b10a1e493d935e4b Mon Sep 17 00:00:00 2001
> From: Andrei Maruha <Andrei_Maruha at epam.com>
> Date: Wed, 6 Apr 2016 12:33:27 +0300
> Subject: low: corosync: Don't take next node id based on max value, if some
>  smaller node id is free.
> 
> Do not assign node id equals to 'maxid + 1' in case if some node was
> removed and free node id can be reused.
> 
> diff --git a/crmsh/corosync.py b/crmsh/corosync.py
> index e9950b8..6401f52 100644
> --- a/crmsh/corosync.py
> +++ b/crmsh/corosync.py
> @@ -327,11 +327,16 @@ def diff_configuration(nodes, checksum=False):
>          utils.remote_diff(local_path, nodes)
>  
>  
> -def next_nodeid(parser):
> +def get_free_nodeid(parser):
>      ids = parser.get_all('nodelist.node.nodeid')
>      if not ids:
>          return 1
> -    return max([int(i) for i in ids]) + 1
> +    ids = [int(i) for i in ids]
> +    max_id = max(ids) + 1
> +    for i in xrange(1, max_id):
> +        if i not in ids:
> +            return i
> +    return max_id
>  
>  
>  def get_ip(node):
> @@ -399,7 +404,7 @@ def add_node(addr, name=None):
>      p = Parser(f)
>  
>      node_addr = addr
> -    node_id = next_nodeid(p)
> +    node_id = get_free_nodeid(p)
>      node_name = name
>      node_value = (make_value('nodelist.node.ring0_addr', node_addr) +
>                    make_value('nodelist.node.nodeid', str(node_id)))
> diff --git a/test/unittests/test_corosync.py b/test/unittests/test_corosync.py
> index db8dd8c..d2a25b6 100644
> --- a/test/unittests/test_corosync.py
> +++ b/test/unittests/test_corosync.py
> @@ -5,6 +5,7 @@
>  
>  import os
>  import unittest
> +import mock
>  from crmsh import corosync
>  from crmsh.corosync import Parser, make_section, make_value
>  
> @@ -67,7 +68,7 @@ class TestCorosyncParser(unittest.TestCase):
>          p.add('nodelist',
>                make_section('nodelist.node',
>                             make_value('nodelist.node.ring0_addr', '10.10.10.10') +
> -                           make_value('nodelist.node.nodeid', str(corosync.next_nodeid(p)))))
> +                           make_value('nodelist.node.nodeid', str(corosync.get_free_nodeid(p)))))
>          _valid(p)
>          self.assertEqual(p.count('nodelist.node'), 6)
>          self.assertEqual(p.get_all('nodelist.node.nodeid'),
> @@ -75,11 +76,11 @@ class TestCorosyncParser(unittest.TestCase):
>  
>      def test_add_node_no_nodelist(self):
>          "test checks that if there is no nodelist, no node is added"
> -        from crmsh.corosync import make_section, make_value, next_nodeid
> +        from crmsh.corosync import make_section, make_value, get_free_nodeid
>  
>          p = Parser(F1)
>          _valid(p)
> -        nid = next_nodeid(p)
> +        nid = get_free_nodeid(p)
>          self.assertEqual(p.count('nodelist.node'), nid - 1)
>          p.add('nodelist',
>                make_section('nodelist.node',
> @@ -89,11 +90,11 @@ class TestCorosyncParser(unittest.TestCase):
>          self.assertEqual(p.count('nodelist.node'), nid - 1)
>  
>      def test_add_node_nodelist(self):
> -        from crmsh.corosync import make_section, make_value, next_nodeid
> +        from crmsh.corosync import make_section, make_value, get_free_nodeid
>  
>          p = Parser(F2)
>          _valid(p)
> -        nid = next_nodeid(p)
> +        nid = get_free_nodeid(p)
>          c = p.count('nodelist.node')
>          p.add('nodelist',
>                make_section('nodelist.node',
> @@ -101,7 +102,7 @@ class TestCorosyncParser(unittest.TestCase):
>                             make_value('nodelist.node.nodeid', str(nid))))
>          _valid(p)
>          self.assertEqual(p.count('nodelist.node'), c + 1)
> -        self.assertEqual(next_nodeid(p), nid + 1)
> +        self.assertEqual(get_free_nodeid(p), nid + 1)
>  
>      def test_remove_node(self):
>          p = Parser(F2)
> @@ -118,5 +119,14 @@ class TestCorosyncParser(unittest.TestCase):
>          _valid(p)
>          self.assertEqual(p.count('service.ver'), 1)
>  
> +    def test_get_free_nodeid(self):
> +        mock_parser = mock.Mock()
> +        mock_parser.get_all.return_value = ['2','5']
> +        self.assertEqual(corosync.get_free_nodeid(mock_parser), 1)
> +        mock_parser.get_all.return_value = ['1','2','5']
> +        self.assertEqual(corosync.get_free_nodeid(mock_parser), 3)
> +        mock_parser.get_all.return_value = ['1','2','3']
> +        self.assertEqual(corosync.get_free_nodeid(mock_parser), 4)
> +
>  if __name__ == '__main__':
>      unittest.main()
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.clusterlabs.org/pipermail/users/attachments/20160406/377b6434/attachment-0004.sig>