Merge ~mgerdts/cloud-init:lp1746605 into cloud-init:master

[Mike Gerdts]

DataSourceSmartOS: add locking of serial device.

cloud-init and mdata-get each have their own implementation of the
SmartOS metadata protocol. If cloud-init and other services that call
mdata-get are run concurrently, crosstalk on the serial port can cause
them both to become confused.

This change makes it so that cloud-init uses the same cooperative
locking scheme that's used by mdata-get, thus preventing cross-talk
between mdata-get and cloud-init.

For testing, a VM running on a SmartOS host and pyserial are required.
If the tests are run on a platform other than SmartOS, those that use a
real serial port are skipped. pyserial remains commented in
requirements.txt because most testers will not be running atop SmartOS.

LP: #1746605

[Mike Gerdts (mgerdts)]

The fix for bug #1667735 is a better fix for the retries that was originally part of this via

https://code.launchpad.net/~mgerdts/cloud-init/+git/cloud-init/+merge/337271

This merge proposal obsoletes merge proposal 337271.

The change to DataSourceSmartOS is now quite small. The new tests will only run on VMs running on a SmartOS host and then only if they have write access to /dev/ttyS1. This test code includes Python 2.6 fixes offered by smoser as part of the previous merge proposal.

Proof that the new test runs under tox:

$ tox tests.unittests.test_datasource.test_smartos.TestSerialConcurrency
GLOB sdist-make: /home/mgerdts/cloud-init/setup.py
py27 inst-nodeps: /home/mgerdts/cloud-init/.tox/dist/cloud-init-18.2.zip
py27 installed: certifi==2018.1.18,chardet==3.0.4,cloud-init==18.2,configobj==5.0.6,contextlib2==0.5.5,coverage==4.5.1,funcsigs==1.0.2,functools32==3.2.3.post2,httpretty==0.8.14,idna==2.6,Jinja2==2.10,jsonpatch==1.23,jsonpointer==2.0,jsonschema==2.6.0,linecache2==1.0.0,MarkupSafe==1.0,mock==2.0.0,nose==1.3.7,oauthlib==2.0.7,pbr==4.0.2,pkg-resources==0.0.0,pyserial==3.4,PyYAML==3.12,requests==2.18.4,six==1.11.0,traceback2==1.4.0,unittest2==1.1.0,urllib3==1.22
py27 runtests: PYTHONHASHSEED='1425861219'
py27 runtests: commands[0] | python -m nose tests.unittests.test_datasource.test_smartos.TestSerialConcurrency

----------------------------------------------------------------------
Ran 0 tests in 0.000s

OK
py3 inst-nodeps: /home/mgerdts/cloud-init/.tox/dist/cloud-init-18.2.zip
py3 installed: certifi==2018.1.18,chardet==3.0.4,cloud-init==18.2,configobj==5.0.6,contextlib2==0.5.5,coverage==4.5.1,httpretty==0.8.14,idna==2.6,Jinja2==2.10,jsonpatch==1.23,jsonpointer==2.0,jsonschema==2.6.0,linecache2==1.0.0,MarkupSafe==1.0,mock==2.0.0,nose==1.3.7,nose-timer==0.7.1,oauthlib==2.0.7,pbr==4.0.2,pkg-resources==0.0.0,pyserial==3.4,PyYAML==3.12,requests==2.18.4,six==1.11.0,termcolor==1.1.0,traceback2==1.4.0,unittest2==1.1.0,urllib3==1.22
py3 runtests: PYTHONHASHSEED='1425861219'
py3 runtests: commands[0] | /home/mgerdts/cloud-init/.tox/py3/bin/python -m nose --with-timer --timer-top-n 10 tests.unittests.test_datasource.test_smartos.TestSerialConcurrency
.
[success] 100.00% tests.unittests.test_datasource.test_smartos.TestSerialConcurrency.test_all_keys: 22.6319s
----------------------------------------------------------------------
Ran 1 test in 22.633s

OK
flake8 inst-nodeps: /home/mgerdts/cloud-init/.tox/dist/cloud-init-18.2.zip
flake8 installed: certifi==2018.1.18,chardet==3.0.4,cloud-init==18.2,configobj==5.0.6,flake8==3.3.0,hacking==0.13.0,idna==2.6,Jinja2==2.10,jsonpatch==1.23,jsonpointer==2.0,jsonschema==2.6.0,MarkupSafe==1.0,mccabe==0.6.1,oauthlib==2.0.7,pbr==4.0.2,pep8==1.5.7,pkg-resources==0.0.0,pycodestyle==2.3.1,pyflakes==1.5.0,PyYAML==3.12,requests==2.18.4,six==1.11.0,urllib3==1.22
flake8 runtests: PYTHONHASHSEED='1425861219'
flake8 runtests: commands[0] | /home/mgerdts/cloud-init/.tox/flake8/bin/python -m flake8 tests.unittests.test_datasource.test_smartos.TestSerialConcurrency
xenial inst-nodeps: /home/mgerdts/cloud-init/.tox/dist/cloud-init-18.2.zip
xenial installed: cloud-init==18.2,configobj==5.0.6,contextlib2==0.5.1,...

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1035/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1035/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

fcntl.lockf(fp, fcntl.LOCK_EX)

Do we not need to unlock at some point ? (fcntl.lockf(fp, fcntl.LOCK_UN)
Or are you relying on the lock being let go of on process exit?
or am I just missing something entirely?

[Mike Gerdts (mgerdts)]

The lock is automatically released on close() or process exit.

On Fri, Apr 20, 2018 at 1:33 PM, Scott Moser <email address hidden>
wrote:

> fcntl.lockf(fp, fcntl.LOCK_EX)
>
> Do we not need to unlock at some point ? (fcntl.lockf(fp, fcntl.LOCK_UN)
> Or are you relying on the lock being let go of on process exit?
> or am I just missing something entirely?
>
>
> --
> https://code.launchpad.net/~mgerdts/cloud-init/+git/cloud-
> init/+merge/343576
> You are the owner of ~mgerdts/cloud-init:lp1746605.
>

[Scott Moser (smoser)]

review: Approve

[Scott Moser (smoser)]

Mike,

I enabled unused variables warnings in pylint a few commits
back. if you rebase this you'll see the errors.

the warning looks like this:
  tests/unittests/test_datasource/test_smartos.py:1053: [W0612(unused-variable), TestSerialConcurrency.test_all_keys] Unused variable 'it'

The fix is just to name that variable '_it' rather than 'it'. Or
even just '_'.

then it wont' complain.

Easiest thing is for you to commit that and push.
as it is right nwo our tools rebase yo to trunk and run tox and then merge... and if i want to use the tools (I do) then you fixing this branch is the easiest thing.

2b4bdbb... by Mike Gerdts on 2018-04-21

feedback from smoser: unused variable

[Scott Moser (smoser)]

An upstream commit landed for this bug.

To view that commit see the following URL:
https://git.launchpad.net/cloud-init/commit/?id=4ed16459

diff --git a/cloudinit/sources/DataSourceSmartOS.py b/cloudinit/sources/DataSourceSmartOS.py
index 0ef1003..4ea00eb 100644
--- a/cloudinit/sources/DataSourceSmartOS.py
+++ b/cloudinit/sources/DataSourceSmartOS.py
@@ -23,6 +23,7 @@
 import base64
 import binascii
 import errno
+import fcntl
 import json
 import os
 import random
@@ -526,6 +527,7 @@ class JoyentMetadataSerialClient(JoyentMetadataClient):
             if not ser.isOpen():
                 raise SystemError("Unable to open %s" % self.device)
             self.fp = ser
+            fcntl.lockf(ser, fcntl.LOCK_EX)
         self._flush()
         self._negotiate()
 
diff --git a/tests/unittests/test_datasource/test_smartos.py b/tests/unittests/test_datasource/test_smartos.py
index b926263..706e8eb 100644
--- a/tests/unittests/test_datasource/test_smartos.py
+++ b/tests/unittests/test_datasource/test_smartos.py
@@ -16,23 +16,27 @@ from __future__ import print_function
 
 from binascii import crc32
 import json
+import multiprocessing
 import os
 import os.path
 import re
 import shutil
+import signal
 import stat
 import tempfile
+import unittest2
 import uuid
 
 from cloudinit import serial
 from cloudinit.sources import DataSourceSmartOS
 from cloudinit.sources.DataSourceSmartOS import (
-    convert_smartos_network_data as convert_net)
+    convert_smartos_network_data as convert_net,
+    SMARTOS_ENV_KVM, SERIAL_DEVICE, get_smartos_environ)
 
 import six
 
 from cloudinit import helpers as c_helpers
-from cloudinit.util import b64e
+from cloudinit.util import (b64e, subp)
 
 from cloudinit.tests.helpers import mock, FilesystemMockingTestCase, TestCase
 
@@ -1023,4 +1027,63 @@ class TestNetworkConversion(TestCase):
         found = convert_net(SDC_NICS_SINGLE_GATEWAY)
         self.assertEqual(expected, found)
 
+
[e-mail](get_smartos_environ() == SMARTOS_ENV_KVM,
+                      "Only supported on KVM and bhyve guests under SmartOS")
[e-mail](os.access(SERIAL_DEVICE, os.W_OK),
+                      "Requires write access to " + SERIAL_DEVICE)
+class TestSerialConcurrency(TestCase):
+    """
+       This class tests locking on an actual serial port, and as such can only
+       be run in a kvm or bhyve guest running on a SmartOS host.  A test run on
+       a metadata socket will not be valid because a metadata socket ensures
+       there is only one session over a connection.  In contrast, in the
+       absence of proper locking multiple processes opening the same serial
+       port can corrupt each others' exchanges with the metadata server.
+    """
+    def setUp(self):
+        self.mdata_proc = multiprocessing.Process(target=self.start_mdata_loop)
+        self.mdata_proc.start()
+        super(TestSerialConcurrency, self).setUp()
+
+    def tearDown(self):
+        # os.kill() rather than mdata_proc.terminate() to avoid console spam.
+        os.kill(self.mdata_proc.pid, signal.SIGKILL)
+        self.mdata_proc.join()
+        super(TestSerialConcurrency, self).tearDown()
+
+    def start_mdata_loop(self):
+        """
+           The mdata-get command is repeatedly run in a separate process so
+           that it may try to race with metadata operations performed in the
+           main test process.  Use of mdata-get is better than two processes
+           using the protocol implementation in DataSourceSmartOS because we
+           are testing to be sure that cloud-init and mdata-get respect each
+           others locks.
+        """
+        rcs = list(range(0, 256))
+        while True:
+            subp(['mdata-get', 'sdc:routes'], rcs=rcs)
+
+    def test_all_keys(self):
+        self.assertIsNotNone(self.mdata_proc.pid)
+        ds = DataSourceSmartOS
+        keys = [tup[0] for tup in ds.SMARTOS_ATTRIB_MAP.values()]
+        keys.extend(ds.SMARTOS_ATTRIB_JSON.values())
+
+        client = ds.jmc_client_factory()
+        self.assertIsNotNone(client)
+
+        # The behavior that we are testing for was observed mdata-get running
+        # 10 times at roughly the same time as cloud-init fetched each key
+        # once.  cloud-init would regularly see failures before making it
+        # through all keys once.
+        for _ in range(0, 3):
+            for key in keys:
+                # We don't care about the return value, just that it doesn't
+                # thrown any exceptions.
+                client.get(key)
+
+        self.assertIsNone(self.mdata_proc.exitcode)
+
 # vi: ts=4 expandtab