Repository: Azure/WALinuxAgent
Pull request #390 (2016-08-25 19:11:47)

Fix issue Azure/WALinuxAgent#388

msftclas: Hi __@krkhan__, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
    <span>
        This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.
    </span>

TTYL, MSBOT;

hglkrijger: lgtm
brendandixon: @hglkrijger Tests would be good. I did verify the code against the Windows Guest Agent. This is the correct format.
hglkrijger: @brendandixon, we can also take this now if we need to and add tests later. 
brendandixon: @hglkrijger @krkhan I'm fine with that. It's a small change. It matches the Windows code. Oddly, I do not see corresponding code in the waagent2.0 file. Perhaps there was a format change and the v2.1.x branch missed full implementation?
brendandixon: @hglkrijger Will our pipeline catch errors here? That is, if an extension writes status, will the pipeline detect that the agent correctly forwards it?
hglkrijger: @brendandixon - not sufficiently. Our automation today checks the local settings file for the expected status, but does not check that it is forwarded out. We can certainly add more rigorous checks in the upcoming replacement pipeline. 

Patch:
diff --git a/README.md b/README.md
index 05af556e..82c37c45 100644
--- a/README.md
+++ b/README.md
@@ -183,6 +183,7 @@ Provisioning.PasswordCryptSaltLength=10
 ResourceDisk.Format=y
 ResourceDisk.Filesystem=ext4
 ResourceDisk.MountPoint=/mnt/resource
+ResourceDisk.MountOptions=None
 ResourceDisk.EnableSwap=n
 ResourceDisk.SwapSizeMB=0
 LBProbeResponder=y
@@ -318,6 +319,12 @@ Type: String Default: /mnt/resource
 
 This specifies the path at which the resource disk is mounted.
 
+ResourceDisk.MountOptions:
+Type: String Default: None  
+
+Specifies disk mount options to be passed to the mount -o command. This is a comma
+separated list of values, ex. 'nodev,nosuid'. See mount(8) for details.
+
 ResourceDisk.EnableSwap:
 Type: Boolean Default: n
 
diff --git a/azurelinuxagent/common/conf.py b/azurelinuxagent/common/conf.py
index 1a3b0da4..9de3d22b 100644
--- a/azurelinuxagent/common/conf.py
+++ b/azurelinuxagent/common/conf.py
@@ -164,6 +164,9 @@ def get_resourcedisk_enable_swap(conf=__conf__):
 def get_resourcedisk_mountpoint(conf=__conf__):
     return conf.get("ResourceDisk.MountPoint", "/mnt/resource")
 
+def get_resourcedisk_mountoptions(conf=__conf__):
+    return conf.get("ResourceDisk.MountOptions", None)
+
 def get_resourcedisk_filesystem(conf=__conf__):
     return conf.get("ResourceDisk.Filesystem", "ext3")
 
diff --git a/azurelinuxagent/common/protocol/wire.py b/azurelinuxagent/common/protocol/wire.py
index 29a1663f..287da33c 100644
--- a/azurelinuxagent/common/protocol/wire.py
+++ b/azurelinuxagent/common/protocol/wire.py
@@ -269,7 +269,7 @@ def ext_status_to_v1(ext_name, ext_status):
         "timestampUTC": timestamp
     }
     if len(v1_sub_status) != 0:
-        v1_ext_status['substatus'] = v1_sub_status
+        v1_ext_status['status']['substatus'] = v1_sub_status
     return v1_ext_status
 
 
diff --git a/azurelinuxagent/daemon/resourcedisk/default.py b/azurelinuxagent/daemon/resourcedisk/default.py
index 2621d8e9..a5f4cf66 100644
--- a/azurelinuxagent/daemon/resourcedisk/default.py
+++ b/azurelinuxagent/daemon/resourcedisk/default.py
@@ -28,23 +28,25 @@ import azurelinuxagent.common.utils.shellutil as shellutil
 from azurelinuxagent.common.exception import ResourceDiskError
 from azurelinuxagent.common.osutil import get_osutil
 
-DATALOSS_WARNING_FILE_NAME="DATALOSS_WARNING_README.txt"
-DATA_LOSS_WARNING="""\
+DATALOSS_WARNING_FILE_NAME = "DATALOSS_WARNING_README.txt"
+DATA_LOSS_WARNING = """\
 WARNING: THIS IS A TEMPORARY DISK.
 
 Any data stored on this drive is SUBJECT TO LOSS and THERE IS NO WAY TO RECOVER IT.
 
 Please do not use this disk for storing any personal or application data.
 
-For additional details to please refer to the MSDN documentation at : http://msdn.microsoft.com/en-us/library/windowsazure/jj672979.aspx
+For additional details to please refer to the MSDN documentation at :
+http://msdn.microsoft.com/en-us/library/windowsazure/jj672979.aspx
 """
 
+
 class ResourceDiskHandler(object):
     def __init__(self):
         self.osutil = get_osutil()
 
     def start_activate_resource_disk(self):
-        disk_thread = threading.Thread(target = self.run)
+        disk_thread = threading.Thread(target=self.run)
         disk_thread.start()
 
     def run(self):
@@ -61,16 +63,17 @@ class ResourceDiskHandler(object):
             mount_point = conf.get_resourcedisk_mountpoint()
             fs = conf.get_resourcedisk_filesystem()
             mount_point = self.mount_resource_disk(mount_point, fs)
-            warning_file = os.path.join(mount_point, DATALOSS_WARNING_FILE_NAME)
+            warning_file = os.path.join(mount_point,
+                                        DATALOSS_WARNING_FILE_NAME)
             try:
                 fileutil.write_file(warning_file, DATA_LOSS_WARNING)
             except IOError as e:
-                logger.warn("Failed to write data loss warnning:{0}", e)
+                logger.warn("Failed to write data loss warning:{0}", e)
             return mount_point
         except ResourceDiskError as e:
             logger.error("Failed to mount resource disk {0}", e)
             add_event(name="WALA", is_success=False, message=ustr(e),
-                              op=WALAEventOperation.ActivateResourceDisk)
+                      op=WALAEventOperation.ActivateResourceDisk)
 
     def enable_swap(self, mount_point):
         logger.info("Enable swap")
@@ -85,62 +88,98 @@ class ResourceDiskHandler(object):
         if device is None:
             raise ResourceDiskError("unable to detect disk topology")
 
-        device = "/dev/" + device
-        mountlist = shellutil.run_get_output("mount")[1]
-        existing = self.osutil.get_mount_point(mountlist, device)
+        device = "/dev/{0}".format(device)
+        partition = device + "1"
+        mount_list = shellutil.run_get_output("mount")[1]
+        existing = self.osutil.get_mount_point(mount_list, device)
 
-        if(existing):
-            logger.info("Resource disk {0}1 is already mounted", device)
+        if existing:
+            logger.info("Resource disk [{0}] is already mounted [{1}]",
+                        partition,
+                        existing)
             return existing
 
         fileutil.mkdir(mount_point, mode=0o755)
-
-        logger.info("Detect GPT...")
-        partition = device + "1"
+        logger.info("Examining partition table")
         ret = shellutil.run_get_output("parted {0} print".format(device))
         if ret[0]:
-            raise ResourceDiskError("({0}) {1}".format(device, ret[1]))
+            raise ResourceDiskError("Could not determine partition info for "
+                                    "{0}: {1}".format(device, ret[1]))
+
+        mkfs_string = "mkfs.{0} {1} -F".format(fs, partition)
 
         if "gpt" in ret[1]:
-            logger.info("GPT detected")
-            logger.info("Get GPT partitions")
-            parts = [x for x in ret[1].split("\n") if re.match("^\s*[0-9]+", x)]
-            logger.info("Found more than {0} GPT partitions.", len(parts))
+            logger.info("GPT detected, finding partitions")
+            parts = [x for x in ret[1].split("\n") if
+                     re.match("^\s*[0-9]+", x)]
+            logger.info("Found {0} GPT partition(s).", len(parts))
             if len(parts) > 1:
-                logger.info("Remove old GPT partitions")
+                logger.info("Removing old GPT partitions")
                 for i in range(1, len(parts) + 1):
-                    logger.info("Remove partition: {0}", i)
+                    logger.info("Remove partition {0}", i)
                     shellutil.run("parted {0} rm {1}".format(device, i))
 
-                logger.info("Create a new GPT partition using entire disk space")
+                logger.info("Creating new GPT partition")
                 shellutil.run("parted {0} mkpart primary 0% 100%".format(device))
 
-                logger.info("Format partition: {0} with fstype {1}",partition,fs)
-                shellutil.run("mkfs." + fs + " " + partition + " -F")
+                logger.info("Format partition [{0}]", mkfs_string)
+                shellutil.run(mkfs_string)
         else:
-            logger.info("GPT not detected")
-            logger.info("Check fstype")
+            logger.info("GPT not detected, determining filesystem")
             ret = shellutil.run_get_output("sfdisk -q -c {0} 1".format(device))
-            if ret[1].rstrip() == "7" and fs != "ntfs":
-                logger.info("The partition is formatted with ntfs")
-                logger.info("Format partition: {0} with fstype {1}",partition,fs)
+            ptype = ret[1].rstrip()
+            if ptype == "7" and fs != "ntfs":
+                logger.info("The partition is formatted with ntfs, updating "
+                            "partition type to 83")
                 shellutil.run("sfdisk -c {0} 1 83".format(device))
-                shellutil.run("mkfs." + fs + " " + partition + " -F")
-
-        logger.info("Mount resource disk")
-        ret = shellutil.run("mount {0} {1}".format(partition, mount_point),
-                                chk_err=False)
+                logger.info("Format partition [{0}]", mkfs_string)
+                shellutil.run(mkfs_string)
+            else:
+                logger.info("The partition type is {0}", ptype)
+
+        mount_options = conf.get_resourcedisk_mountoptions()
+        mount_string = self.get_mount_string(mount_options,
+                                             partition,
+                                             mount_point)
+        logger.info("Mount resource disk [{0}]", mount_string)
+        ret = shellutil.run(mount_string, chk_err=False)
         if ret:
-            logger.warn("Failed to mount resource disk. Retry mounting")
-            shellutil.run("mkfs." + fs + " " + partition + " -F")
-            ret = shellutil.run("mount {0} {1}".format(partition, mount_point))
+            # Some kernels seem to issue an async partition re-read after a
+            # 'parted' command invocation. This causes mount to fail if the
+            # partition re-read is not complete by the time mount is
+            # attempted. Seen in CentOS 7.2. Force a sequential re-read of
+            # the partition and try mounting.
+            logger.warn("Failed to mount resource disk. "
+                        "Retry mounting after re-reading partition info.")
+            if shellutil.run("sfdisk -R {0}".format(device), chk_err=False):
+                shellutil.run("blockdev --rereadpt {0}".format(device),
+                              chk_err=False)
+            ret = shellutil.run(mount_string, chk_err=False)
             if ret:
-                raise ResourceDiskError("({0}) {1}".format(partition, ret))
-
-        logger.info("Resource disk ({0}) is mounted at {1} with fstype {2}",
-                    device, mount_point, fs)
+                logger.warn("Failed to mount resource disk. "
+                            "Attempting to format and retry mount.")
+                shellutil.run(mkfs_string)
+                ret = shellutil.run(mount_string)
+                if ret:
+                    raise ResourceDiskError("Could not mount {0} "
+                                            "after syncing partition table: "
+                                            "{1}".format(partition, ret))
+
+        logger.info("Resource disk {0} is mounted at {1} with {2}",
+                    device,
+                    mount_point,
+                    fs)
         return mount_point
 
+    @staticmethod
+    def get_mount_string(mount_options, partition, mount_point):
+        if mount_options is not None:
+            return 'mount -o {0} {1} {2}'.format(mount_options,
+                                                 partition,
+                                                 mount_point)
+        else:
+            return 'mount {0} {1}'.format(partition, mount_point)
+
     def create_swap_space(self, mount_point, size_mb):
         size_kb = size_mb * 1024
         size = size_kb * 1024
@@ -166,13 +205,17 @@ class ResourceDiskHandler(object):
 
     def mkfile(self, filename, nbytes):
         """
-        Create a non-sparse file of that size. Deletes and replaces existing file.
+        Create a non-sparse file of that size. Deletes and replaces existing
+        file.
 
-        To allow efficient execution, fallocate will be tried first. This includes
-        ``os.posix_fallocate`` on Python 3.3+ (unix) and the ``fallocate`` command
+        To allow efficient execution, fallocate will be tried first. This
+        includes
+        ``os.posix_fallocate`` on Python 3.3+ (unix) and the ``fallocate``
+        command
         in the popular ``util-linux{,-ng}`` package.
 
-        A dd fallback will be tried too. When size < 64M, perform single-pass dd.
+        A dd fallback will be tried too. When size < 64M, perform
+        single-pass dd.
         Otherwise do two-pass dd.
         """
 
@@ -200,7 +243,8 @@ class ResourceDiskHandler(object):
 
         # fallocate command
         fn_sh = shellutil.quote((filename,))
-        ret = shellutil.run(u"umask 0077 && fallocate -l {0} {1}".format(nbytes, fn_sh))
+        ret = shellutil.run(
+            u"umask 0077 && fallocate -l {0} {1}".format(nbytes, fn_sh))
         if ret == 0:
             return ret
 
@@ -208,7 +252,8 @@ class ResourceDiskHandler(object):
 
         # dd fallback
         dd_maxbs = 64 * 1024 ** 2
-        dd_cmd = "umask 0077 && dd if=/dev/zero bs={0} count={1} conv=notrunc of={2}"
+        dd_cmd = "umask 0077 && dd if=/dev/zero bs={0} count={1} " \
+                 "conv=notrunc of={2}"
 
         blocks = int(nbytes / dd_maxbs)
         if blocks > 0:
diff --git a/config/alpine/waagent.conf b/config/alpine/waagent.conf
index 083461e9..19380cd2 100644
--- a/config/alpine/waagent.conf
+++ b/config/alpine/waagent.conf
@@ -39,6 +39,9 @@ ResourceDisk.EnableSwap=n
 # Size of the swapfile.
 ResourceDisk.SwapSizeMB=0
 
+# Comma-seperated list of mount options. See man(8) for valid options.
+ResourceDisk.MountOptions=None
+
 # Respond to load balancer probes if requested by Windows Azure.
 LBProbeResponder=y
 
diff --git a/config/coreos/waagent.conf b/config/coreos/waagent.conf
index 69c4f510..0c6af8ed 100644
--- a/config/coreos/waagent.conf
+++ b/config/coreos/waagent.conf
@@ -48,6 +48,9 @@ ResourceDisk.EnableSwap=n
 # Size of the swapfile.
 ResourceDisk.SwapSizeMB=0
 
+# Comma-seperated list of mount options. See man(8) for valid options.
+ResourceDisk.MountOptions=None
+
 # Respond to load balancer probes if requested by Windows Azure.
 LBProbeResponder=y
 
diff --git a/config/freebsd/waagent.conf b/config/freebsd/waagent.conf
index 4499486e..6dbc89a3 100644
--- a/config/freebsd/waagent.conf
+++ b/config/freebsd/waagent.conf
@@ -45,6 +45,9 @@ ResourceDisk.EnableSwap=n
 # Size of the swapfile.
 ResourceDisk.SwapSizeMB=0
 
+# Comma-seperated list of mount options. See man(8) for valid options.
+ResourceDisk.MountOptions=None
+
 # Enable verbose logging (y|n)
 Logs.Verbose=n
 
diff --git a/config/suse/waagent.conf b/config/suse/waagent.conf
index 4b1ed879..25573a42 100644
--- a/config/suse/waagent.conf
+++ b/config/suse/waagent.conf
@@ -48,6 +48,9 @@ ResourceDisk.EnableSwap=n
 # Size of the swapfile.
 ResourceDisk.SwapSizeMB=0
 
+# Comma-seperated list of mount options. See man(8) for valid options.
+ResourceDisk.MountOptions=None
+
 # Respond to load balancer probes if requested by Microsoft Azure.
 LBProbeResponder=y
 
diff --git a/config/ubuntu/waagent.conf b/config/ubuntu/waagent.conf
index e6297446..5bdad9cb 100644
--- a/config/ubuntu/waagent.conf
+++ b/config/ubuntu/waagent.conf
@@ -48,6 +48,9 @@ ResourceDisk.EnableSwap=n
 # Size of the swapfile.
 ResourceDisk.SwapSizeMB=0
 
+# Comma-seperated list of mount options. See man(8) for valid options.
+ResourceDisk.MountOptions=None
+
 # Respond to load balancer probes if requested by Microsoft Azure.
 LBProbeResponder=y
 
diff --git a/config/waagent.conf b/config/waagent.conf
index a7964882..9e9c48c4 100644
--- a/config/waagent.conf
+++ b/config/waagent.conf
@@ -48,6 +48,9 @@ ResourceDisk.EnableSwap=n
 # Size of the swapfile.
 ResourceDisk.SwapSizeMB=0
 
+# Comma-seperated list of mount options. See man(8) for valid options.
+ResourceDisk.MountOptions=None
+
 # Enable verbose logging (y|n)
 Logs.Verbose=n
 


Test patch:
diff --git a/tests/daemon/test_resourcedisk.py b/tests/daemon/test_resourcedisk.py
new file mode 100644
index 00000000..d2fc8fb5
--- /dev/null
+++ b/tests/daemon/test_resourcedisk.py
@@ -0,0 +1,43 @@
+# Copyright 2014 Microsoft Corporation
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Requires Python 2.4+ and Openssl 1.0+
+#
+
+from tests.tools import *
+from azurelinuxagent.common.exception import *
+from azurelinuxagent.daemon import *
+from azurelinuxagent.daemon.resourcedisk.default import ResourceDiskHandler
+
+class TestResourceDisk(AgentTestCase):
+    def test_mount_flags_empty(self):
+        partition = '/dev/sdb1'
+        mountpoint = '/mnt/resource'
+        options = None
+        expected = 'mount /dev/sdb1 /mnt/resource'
+        rdh = ResourceDiskHandler()
+        mount_string = rdh.get_mount_string(options, partition, mountpoint)
+        self.assertEqual(expected, mount_string)
+
+    def test_mount_flags_many(self):
+        partition = '/dev/sdb1'
+        mountpoint = '/mnt/resource'
+        options = 'noexec,noguid,nodev'
+        expected = 'mount -o noexec,noguid,nodev /dev/sdb1 /mnt/resource'
+        rdh = ResourceDiskHandler()
+        mount_string = rdh.get_mount_string(options, partition, mountpoint)
+        self.assertEqual(expected, mount_string)
+
+if __name__ == '__main__':
+    unittest.main()