Merge #681

681: Cleanse backend memory args r=Saviq a=ricab Fixes #677 On investigation, back-end tools use slightly different formats to represent digital info sizes (bytes). So, our previous assumption that a *bytes string* would be correctly interpreted everywhere is not actually true. IOW, neither plain strings nor plain numbers are adequate to convey memory sizes uniformly, so the earlier *shortcut implementation* falls short. This PR encapsulates memory sizes in a dedicated type: `MemorySize`. This type does the necessary arithmetic, but it is the responsibility of each (internal) back-end implementation to derive the string representation they need (unit and unit representation). Co-authored-by: Ricardo Abreu <[email protected]>
canonical · Mar 18, 2019 · 05a27c5 · 05a27c5
2 parents cd98786 + 352f9e6
commit 05a27c5
Show file tree

Hide file tree

Showing 20 changed files with 524 additions and 273 deletions.
diff --git a/include/multipass/exceptions/invalid_memory_size_exception.h b/include/multipass/exceptions/invalid_memory_size_exception.h
@@ -0,0 +1,37 @@
+/*
+ * Copyright (C) 2019 Canonical, Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef MULTIPASS_INVALID_MEMORY_SIZE_EXCEPTION_H
+#define MULTIPASS_INVALID_MEMORY_SIZE_EXCEPTION_H
+
+#include <fmt/format.h>
+
+#include <stdexcept>
+#include <string>
+
+namespace multipass
+{
+class InvalidMemorySizeException : public std::runtime_error
+{
+public:
+    InvalidMemorySizeException(const std::string& val)
+        : runtime_error(fmt::format("{} is not a valid memory size", val))
+    {
+    }
+};
+} // namespace multipass
+#endif // MULTIPASS_INVALID_MEMORY_SIZE_EXCEPTION_H
diff --git a/include/multipass/memory_size.h b/include/multipass/memory_size.h
@@ -0,0 +1,55 @@
+/*
+ * Copyright (C) 2019 Canonical, Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef MULTIPASS_MEMORY_SIZE_H
+#define MULTIPASS_MEMORY_SIZE_H
+
+#include <string>
+
+namespace multipass
+{
+class MemorySize
+{
+public:
+    friend bool operator==(const MemorySize& a, const MemorySize& b);
+    friend bool operator!=(const MemorySize& a, const MemorySize& b);
+    friend bool operator<(const MemorySize& a, const MemorySize& b);
+    friend bool operator>(const MemorySize& a, const MemorySize& b);
+    friend bool operator<=(const MemorySize& a, const MemorySize& b);
+    friend bool operator>=(const MemorySize& a, const MemorySize& b);
+
+    MemorySize();
+    explicit MemorySize(const std::string& val);
+    long long in_bytes() const noexcept;
+    long long in_kilobytes() const noexcept;
+    long long in_megabytes() const noexcept;
+    long long in_gigabytes() const noexcept;
+
+private:
+    long long bytes;
+};
+
+bool operator==(const MemorySize& a, const MemorySize& b);
+bool operator!=(const MemorySize& a, const MemorySize& b);
+bool operator<(const MemorySize& a, const MemorySize& b);
+bool operator>(const MemorySize& a, const MemorySize& b);
+bool operator<=(const MemorySize& a, const MemorySize& b);
+bool operator>=(const MemorySize& a, const MemorySize& b);
+
+} // namespace multipass
+
+#endif // MULTIPASS_MEMORY_SIZE_H
diff --git a/include/multipass/utils.h b/include/multipass/utils.h
@@ -18,7 +18,6 @@
 #ifndef MULTIPASS_UTILS_H
 #define MULTIPASS_UTILS_H
 
-#include <multipass/optional.h>
 #include <multipass/path.h>
 #include <multipass/virtual_machine.h>
 
@@ -52,8 +51,6 @@ QString make_uuid();
 std::string contents_of(const multipass::Path& file_path);
 bool has_only_digits(const std::string& value);
 void validate_server_address(const std::string& value);
-optional<long long> in_bytes(const std::string& mem_value);
-std::string in_bytes_string(long long bytes);
 bool valid_hostname(const std::string& name_string);
 bool invalid_target_path(const QString& target_path);
 std::string to_cmd(const std::vector<std::string>& args, QuoteType type);

diff --git a/include/multipass/virtual_machine_description.h b/include/multipass/virtual_machine_description.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017 Canonical, Ltd.
+ * Copyright (C) 2017-2019 Canonical, Ltd.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -20,6 +20,7 @@
 #ifndef MULTIPASS_VIRTUAL_MACHINE_DESCRIPTION_H
 #define MULTIPASS_VIRTUAL_MACHINE_DESCRIPTION_H
 
+#include <multipass/memory_size.h>
 #include <multipass/vm_image.h>
 #include <string>
 
@@ -32,8 +33,8 @@ class VirtualMachineDescription
     using MBytes = size_t;
 
     int num_cores;
-    std::string mem_size;
-    std::string disk_space;
+    MemorySize mem_size;
+    MemorySize disk_space;
     std::string vm_name;
     std::string mac_addr;
     std::string ssh_username;

diff --git a/src/client/cmd/launch.cpp b/src/client/cmd/launch.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017-2018 Canonical, Ltd.
+ * Copyright (C) 2017-2019 Canonical, Ltd.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -86,10 +86,16 @@ mp::ParseCode cmd::Launch::parse_args(mp::ArgParser* parser)
                                   "format.\n",
                                   "[[<remote:>]<image> | <url>]");
     QCommandLineOption cpusOption({"c", "cpus"}, "Number of CPUs to allocate", "cpus", "1");
-    QCommandLineOption diskOption({"d", "disk"}, "Disk space to allocate in bytes, or with K, M, G suffix", "disk",
-                                  "default");
-    QCommandLineOption memOption({"m", "mem"}, "Amount of memory to allocate in bytes, or with K, M, G suffix", "mem",
-                                 "1024"); // In MB's
+    QCommandLineOption diskOption({"d", "disk"},
+                                  QString::fromStdString(fmt::format("Disk space to allocate. Positive integers, in "
+                                                                     "bytes, or with K, M, G suffix. Minimum: {}.",
+                                                                     min_disk_size)),
+                                  "disk", "default");
+    QCommandLineOption memOption({"m", "mem"},
+                                 QString::fromStdString(fmt::format("Amount of memory to allocate. Positive integers, "
+                                                                    "in bytes, or with K, M, G suffix. Mimimum: {}.",
+                                                                    min_memory_size)),
+                                 "mem", "1024"); // In MB's
     QCommandLineOption nameOption({"n", "name"}, "Name for the instance", "name");
     QCommandLineOption cloudInitOption("cloud-init", "Path to a user-data cloud-init configuration, or '-' for stdin",
                                        "file");
@@ -257,15 +263,11 @@ mp::ReturnCode cmd::Launch::request_launch()
         {
             if (error == LaunchError::INVALID_DISK_SIZE)
             {
-                error_details =
-                    fmt::format("Invalid disk size value supplied: {}. Note that a minimum of {} is required.",
-                                request.disk_space(), min_disk_size);
+                error_details = fmt::format("Invalid disk size value supplied: {}.", request.disk_space());
             }
             else if (error == LaunchError::INVALID_MEM_SIZE)
             {
-                error_details =
-                    fmt::format("Invalid memory size value supplied: {}. Note that a minimum of {} is required.",
-                                request.mem_size(), min_memory_size);
+                error_details = fmt::format("Invalid memory size value supplied: {}.", request.mem_size());
             }
             else if (error == LaunchError::INVALID_HOSTNAME)
             {

diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp
@@ -22,6 +22,7 @@
 #include <multipass/cloud_init_iso.h>
 #include <multipass/constants.h>
 #include <multipass/exceptions/exitless_sshprocess_exception.h>
+#include <multipass/exceptions/invalid_memory_size_exception.h>
 #include <multipass/exceptions/sshfs_missing_error.h>
 #include <multipass/exceptions/start_exception.h>
 #include <multipass/logging/client_logger.h>
@@ -71,8 +72,6 @@ constexpr auto reboot_cmd = "sudo reboot";
 constexpr auto up_timeout = 2min; // This may be tweaked as appropriate and used in places that wait for ssh to be up
 constexpr auto stop_ssh_cmd = "sudo systemctl stop ssh";
 constexpr auto max_install_sshfs_retries = 3;
-const auto normalized_min_mem = mp::utils::in_bytes(mp::min_memory_size);
-const auto normalized_min_disk = mp::utils::in_bytes(mp::min_disk_size);
 
 mp::Query query_from(const mp::LaunchRequest* request, const std::string& name)
 {
@@ -155,7 +154,7 @@ void prepare_user_data(YAML::Node& user_data_config, YAML::Node& vendor_config)
 }
 
 mp::VirtualMachineDescription to_machine_desc(const mp::LaunchRequest* request, const std::string& name,
-                                              const std::string& mem_size, const std::string& disk_space,
+                                              const mp::MemorySize& mem_size, const mp::MemorySize& disk_space,
                                               const std::string& mac_addr, const std::string& ssh_username,
                                               const mp::VMImage& image, YAML::Node& meta_data_config,
                                               YAML::Node& user_data_config, YAML::Node& vendor_data_config,
@@ -255,8 +254,8 @@ std::unordered_map<std::string, mp::VMSpecs> load_db(const mp::Path& data_path,
         }
 
         reconstructed_records[key] = {num_cores,
-                                      mem_size.toStdString(),
-                                      disk_space.toStdString(),
+                                      mp::MemorySize{mem_size.toStdString()},
+                                      mp::MemorySize{disk_space.toStdString()},
                                       mac_addr.toStdString(),
                                       ssh_username.toStdString(),
                                       static_cast<mp::VirtualMachine::State>(state),
@@ -278,23 +277,40 @@ auto fetch_image_for(const std::string& name, const mp::FetchType& fetch_type, m
     return vault.fetch_image(fetch_type, query, stub_prepare, stub_progress);
 }
 
+auto try_mem_size(const std::string& val) -> mp::optional<mp::MemorySize>
+{
+    try
+    {
+        return mp::MemorySize{val};
+    }
+    catch (mp::InvalidMemorySizeException& /*unused*/)
+    {
+        return mp::nullopt;
+    }
+}
+
 auto validate_create_arguments(const mp::LaunchRequest* request)
 {
-    auto mem_size = request->mem_size();
-    auto disk_space = request->disk_space();
+    static const auto min_mem = try_mem_size(mp::min_memory_size);
+    static const auto min_disk = try_mem_size(mp::min_disk_size);
+    assert(min_mem && min_disk);
+
+    auto mem_size_str = request->mem_size();
+    auto disk_space_str = request->disk_space();
     auto instance_name = request->instance_name();
     auto option_errors = mp::LaunchError{};
 
-    const auto opt_mem_size = mp::utils::in_bytes(mem_size.empty() ? "1G" : mem_size);
-    const auto opt_disk_space = mp::utils::in_bytes(disk_space.empty() ? "5G" : disk_space);
+    const auto opt_mem_size = try_mem_size(mem_size_str.empty() ? "1G" : mem_size_str);
+    const auto opt_disk_space = try_mem_size(disk_space_str.empty() ? "5G" : disk_space_str);
 
-    if (opt_mem_size && *opt_mem_size >= normalized_min_mem)
-        mem_size = mp::utils::in_bytes_string(*opt_mem_size);
+    mp::MemorySize mem_size{}, disk_space{};
+    if (opt_mem_size && *opt_mem_size >= min_mem)
+        mem_size = *opt_mem_size;
     else
         option_errors.add_error_codes(mp::LaunchError::INVALID_MEM_SIZE);
 
-    if (opt_disk_space && *opt_disk_space >= normalized_min_disk)
-        disk_space = mp::utils::in_bytes_string(*opt_disk_space);
+    if (opt_disk_space && *opt_disk_space >= min_disk)
+        disk_space = *opt_disk_space;
     else
         option_errors.add_error_codes(mp::LaunchError::INVALID_DISK_SIZE);
 
@@ -303,8 +319,8 @@ auto validate_create_arguments(const mp::LaunchRequest* request)
 
     struct CheckedArguments
     {
-        std::string mem_size;
-        std::string disk_space;
+        mp::MemorySize mem_size;
+        mp::MemorySize disk_space;
         std::string instance_name;
         mp::LaunchError option_errors;
     } ret{mem_size, disk_space, instance_name, option_errors};
@@ -1854,8 +1870,8 @@ void mp::Daemon::persist_instances()
     auto vm_spec_to_json = [](const mp::VMSpecs& specs) -> QJsonObject {
         QJsonObject json;
         json.insert("num_cores", specs.num_cores);
-        json.insert("mem_size", QString::fromStdString(specs.mem_size));
-        json.insert("disk_space", QString::fromStdString(specs.disk_space));
+        json.insert("mem_size", QString::number(specs.mem_size.in_bytes()));
+        json.insert("disk_space", QString::number(specs.disk_space.in_bytes()));
         json.insert("mac_addr", QString::fromStdString(specs.mac_addr));
         json.insert("ssh_username", QString::fromStdString(specs.ssh_username));
         json.insert("state", static_cast<int>(specs.state));

diff --git a/src/daemon/daemon.h b/src/daemon/daemon.h
@@ -22,6 +22,7 @@
 #include "daemon_rpc.h"
 
 #include <multipass/delayed_shutdown_timer.h>
+#include <multipass/memory_size.h>
 #include <multipass/metrics_provider.h>
 #include <multipass/sshfs_mount/sshfs_mount.h>
 #include <multipass/virtual_machine.h>
@@ -42,8 +43,8 @@ struct VMMount
 struct VMSpecs
 {
     int num_cores;
-    std::string mem_size;
-    std::string disk_space;
+    MemorySize mem_size;
+    MemorySize disk_space;
     std::string mac_addr;
     std::string ssh_username;
     VirtualMachine::State state;

diff --git a/src/platform/backends/libvirt/libvirt_virtual_machine.cpp b/src/platform/backends/libvirt/libvirt_virtual_machine.cpp
@@ -92,29 +92,6 @@ auto instance_ip_for(virConnectPtr connection, const std::string& mac_addr)
     return ip_address;
 }
 
-void parsed_memory_values_for(const std::string& mem_size, std::string& memory, std::string& mem_unit)
-{
-    auto mem{QString::fromStdString(mem_size)};
-    QString unit;
-
-    for (const auto& c : mem)
-    {
-        if (c.isDigit())
-            memory.append(1, c.toLatin1());
-        else if (c.isLetter())
-            unit.append(c);
-    }
-
-    if (unit.isEmpty())
-        mem_unit = "b";
-    else if (unit.startsWith("K"))
-        mem_unit = "KiB";
-    else if (unit.startsWith("M"))
-        mem_unit = "MiB";
-    else if (unit.startsWith("G"))
-        mem_unit = "GiB";
-}
-
 auto host_architecture_for(virConnectPtr connection)
 {
     std::string arch;
@@ -139,8 +116,9 @@ auto host_architecture_for(virConnectPtr connection)
 auto generate_xml_config_for(const mp::VirtualMachineDescription& desc, const std::string& bridge_name,
                              const std::string& arch)
 {
-    std::string memory, mem_unit;
-    parsed_memory_values_for(desc.mem_size, memory, mem_unit);
+    static constexpr auto mem_unit = "k"; // see https://libvirt.org/formatdomain.html#elementsMemoryAllocation
+    const auto memory = desc.mem_size.in_kilobytes(); /* floored here, but then "[...] the value will be rounded up to
+    the nearest kibibyte by libvirt, and may be further rounded to the granularity supported by the hypervisor [...]" */
 
     auto qemu_path = fmt::format("/usr/bin/qemu-system-{}", arch);
 

diff --git a/src/platform/backends/qemu/qemu_vm_process_spec.cpp b/src/platform/backends/qemu/qemu_vm_process_spec.cpp
@@ -34,11 +34,8 @@ QString mp::QemuVMProcessSpec::program() const
 
 QStringList mp::QemuVMProcessSpec::arguments() const
 {
-    auto mem_size = QString::fromStdString(desc.mem_size);
-    if (mem_size.endsWith("B"))
-    {
-        mem_size.chop(1);
-    }
+    auto mem_size = QString::number(desc.mem_size.in_megabytes()) + 'M'; /* flooring here; format documented in
+    `man qemu-system`, under `-m` option; including suffix to avoid relying on default unit */
 
     QStringList args{"--enable-kvm"};
     // The VM image itself

diff --git a/src/platform/backends/shared/linux/backend_utils.cpp b/src/platform/backends/shared/linux/backend_utils.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2018 Canonical, Ltd.
+ * Copyright (C) 2018-2019 Canonical, Ltd.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -20,6 +20,7 @@
 #include "process_factory.h"
 #include "qemuimg_process_spec.h"
 #include <multipass/logging/log.h>
+#include <multipass/memory_size.h>
 #include <multipass/utils.h>
 
 #include <fmt/format.h>
@@ -133,14 +134,10 @@ void mp::backend::check_hypervisor_support()
     }
 }
 
-void mp::backend::resize_instance_image(const ProcessFactory* process_factory, const std::string& disk_space,
+void mp::backend::resize_instance_image(const ProcessFactory* process_factory, const MemorySize& disk_space,
                                         const mp::Path& image_path)
 {
-    auto disk_size = QString::fromStdString(disk_space);
-
-    if (disk_size.endsWith("B"))
-        disk_size.chop(1);
-
+    auto disk_size = QString::number(disk_space.in_bytes()); // format documented in `man qemu-img` (look for "size")
     auto qemuimg_spec = std::make_unique<mp::QemuImgProcessSpec>();
     auto qemuimg_process = process_factory->create_process(std::move(qemuimg_spec));