cloud-init

Merge ~redriver/cloud-init:fix-mount-issue-for-frbsd-on-Azure into cloud-init:master

  1. Git
  2. lp:~redriver/cloud-init
  3. fix-mount-issue-for-frbsd-on-Azure
  4. Merge into master
Proposed by Hongjiang Zhang on 2017-06-07
Status: Merged
Approved by: Scott Moser on 2017-06-14
Approved revision: c519fa8eafd8bb9551729da969d5da00df43f1df
Merged at revision: 8a06a1244c8ee20902db050e142c5a0b2fd777a9
Proposed branch: ~redriver/cloud-init:fix-mount-issue-for-frbsd-on-Azure
Merge into: cloud-init:master
Diff against target: 59 lines (+18/-9)
2 files modified
cloudinit/sources/DataSourceAzure.py (+7/-8)
tests/unittests/test_datasource/test_azure.py (+11/-1)
Reviewer Review Type Date Requested Status
Scott Moser 2017-06-07 Approve on 2017-06-14
Server Team CI bot continuous-integration Approve on 2017-06-13
Review via email: mp+325207@code.launchpad.net

Description of the Change

FreeBSD: Check whether the CDROM is configured or not

The current method is to attempt to mount the cdrom (/dev/cd0), if it is successful, /dev/cd0 is configured, otherwise, it is not configured. The problem is it forgets to check whether the mounting destination folder is created or not. As a result, mounting attempt failed even if cdrom is ready.

LP: #1696295

To post a comment you must log in.
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Scott Moser (smoser) wrote :

Hi,
So I think the basic idea here is to attempt to mount the cdrom to verify that there is in fact a cdrom.
Thats not an entirely bad idea, but it seems like it might be sufficient to either:

a.) always add 'dev/cd0' on freebsd and let the code in get_data just attempt to mount, which means we would not be mounting the thing twice.
b.) attempt an 'open' and read of /dev/cd0.
  this would be quicker than a mount, and at least on linux the 'open' would fail if the cdrom does not have any disk in it.

ie:

  if util.is_FreeBSD():
      cdrom_dev = "/dev/cd0"
      try:
         with open(cdrom_dev) as fp:
            fp.read(1024)
            devlist.append(cdrom_dev)
      except IOError:
         pass

Scott Moser (smoser) wrote :

Would the above work? Tests might have to be adjusted.

Thanks Hongjiang!

Scott

review: Needs Information
Scott Moser (smoser) wrote :

One more thing.
I'm going to put this into 'Work in progress'.
Please
a.) implement/test my suggestion (verifying that tests still work)
b.) update your 'Commit message' to be of the right format
    Subject Line Here
    <blank line>
    info about the problem and fix
    <blank line>
    LP: #1696295

c.) move it back to 'Needs Review'.

Thanks!

Hongjiang Zhang (redriver) wrote :

> One more thing.
> I'm going to put this into 'Work in progress'.
> Please
> a.) implement/test my suggestion (verifying that tests still work)
> b.) update your 'Commit message' to be of the right format
> Subject Line Here
> <blank line>
> info about the problem and fix
> <blank line>
> LP: #1696295
>
> c.) move it back to 'Needs Review'.
>
> Thanks!
The "open" method works for me. Thanks.

Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
~redriver/cloud-init:fix-mount-issue-for-frbsd-on-Azure updated on 2017-06-13
c519fa8... by Hongjiang Zhang on 2017-06-13

add one more blank line

Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Scott Moser (smoser) wrote :

Looks good, thanks.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
2index a0b9eae..a8bad90 100644
3--- a/cloudinit/sources/DataSourceAzure.py
4+++ b/cloudinit/sources/DataSourceAzure.py
5@@ -799,18 +799,17 @@ def encrypt_pass(password, salt_id="$6$"):
6
7
8 def list_possible_azure_ds_devs():
9- # return a sorted list of devices that might have a azure datasource
10 devlist = []
11 if util.is_FreeBSD():
12+ # add '/dev/cd0' to devlist if it is configured
13+ # here wants to test whether '/dev/cd0' is available
14 cdrom_dev = "/dev/cd0"
15 try:
16- util.subp(["mount", "-o", "ro", "-t", "udf", cdrom_dev,
17- "/mnt/cdrom/secure"])
18- except util.ProcessExecutionError:
19- LOG.debug("Fail to mount cd")
20- return devlist
21- util.subp(["umount", "/mnt/cdrom/secure"])
22- devlist.append(cdrom_dev)
23+ with open(cdrom_dev) as fp:
24+ fp.read(1024)
25+ devlist.append(cdrom_dev)
26+ except IOError:
27+ LOG.debug("cdrom (%s) is not configured", cdrom_dev)
28 else:
29 for fstype in ("iso9660", "udf"):
30 devlist.extend(util.find_devs_with("TYPE=%s" % fstype))
31diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
32index 42f49e0..26239da 100644
33--- a/tests/unittests/test_datasource/test_azure.py
34+++ b/tests/unittests/test_datasource/test_azure.py
35@@ -7,7 +7,8 @@ from cloudinit.util import find_freebsd_part
36 from cloudinit.util import get_path_dev_freebsd
37
38 from ..helpers import (CiTestCase, TestCase, populate_dir, mock,
39- ExitStack, PY26, SkipTest)
40+ ExitStack, PY26, PY3, SkipTest)
41+from mock import patch, mock_open
42
43 import crypt
44 import os
45@@ -542,6 +543,15 @@ fdescfs /dev/fd fdescfs rw 0 0
46 ds.get_data()
47 self.assertEqual(self.instance_id, ds.metadata['instance-id'])
48
49+ def test_list_possible_azure_ds_devs(self):
50+ devlist = []
51+ with patch('platform.platform',
52+ mock.MagicMock(return_value="FreeBSD")):
53+ name = 'builtins.open' if PY3 else '__builtin__.open'
54+ with patch(name, mock_open(read_data="data")):
55+ devlist.extend(dsaz.list_possible_azure_ds_devs())
56+ self.assertEqual(devlist, ['/dev/cd0'])
57+
58
59 class TestAzureBounce(TestCase):
60

Subscribers

People subscribed via source and target branches