sts-sponsors team mailing list archive

[Björn Tillenius <mp+442664@xxxxxxxxxxxxxxxxxx>]

Review: Approve

+1 with some comments inline.

Diff comments:

> diff --git a/src/metadataserver/builtin_scripts/commissioning_scripts/40-maas-01-machine-config-hints b/src/metadataserver/builtin_scripts/commissioning_scripts/40-maas-01-machine-config-hints
> new file mode 100644
> index 0000000..e68907e
> --- /dev/null
> +++ b/src/metadataserver/builtin_scripts/commissioning_scripts/40-maas-01-machine-config-hints
> @@ -0,0 +1,94 @@
> +#!/usr/bin/env python3
> +#
> +# machine-config-hints - Generate hints for machine configuration.
> +# Currently provides subarchitecture for DGX systems, but the idea is
> +# to allow updating a subset of machine configuration from the
> +# commissioning scripts without modifying the region source code.
> +#
> +# Copyright (C) 2023 Canonical
> +#
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU Affero General Public License as
> +# published by the Free Software Foundation, either version 3 of the
> +# License, or (at your option) any later version.
> +#
> +# 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 Affero General Public License for more details.
> +#
> +# You should have received a copy of the GNU Affero General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +# --- Start MAAS 1.0 script metadata ---
> +# name: 40-maas-01-machine-config-hints
> +# title: Generate machine configuration hints
> +# description: Generate machine configuration hints
> +# script_type: commissioning
> +# timeout: 60
> +# --- End MAAS 1.0 script metadata ---
> +import json
> +import os
> +import sys
> +
> +
> +def read_json_file(path):
> +    try:
> +        with open(path) as fd:
> +            return json.load(fd)
> +    except OSError as e:
> +        sys.exit(f"Failed to read {path}: {e}")
> +    except json.JSONDecodeError as e:
> +        sys.exit(f"Failed to parse {path}: {e}")
> +
> +
> +def detect_nvidia_dgx(mr):

what's "mr"? Could you spell it out please?

> +    """Returns whether machine-resources output suggests a DGX system"""
> +    if "resources" not in mr:
> +        return False
> +    if "system" not in mr["resources"]:
> +        return False
> +    if "motherboard" not in mr["resources"]["system"]:
> +        return False
> +    motherboard = mr["resources"]["system"]["motherboard"]
> +    vendor = motherboard.get("vendor", None)
> +    product = motherboard.get("product", None)
> +    return (
> +        vendor.lower() == "nvidia"
> +        and product.lower().removeprefix("nvidia ") == "dgx"
> +    )
> +
> +
> +PLATFORMS = {
> +    "nvidia-dgx": detect_nvidia_dgx,
> +}
> +
> +
> +def detect_platform(machine_resources):
> +    """Calls detection methods from PLATFORM and returns platform name"""
> +    for platform_name, f in PLATFORMS.items():
> +        if f(machine_resources):

Maybe "is_platform" rather than "f"?

> +            return platform_name
> +    return "generic"
> +
> +
> +def provide_hints(resources):
> +    """Returns a hints dictionary"""
> +    return {"platform": detect_platform(resources), "tags": []}
> +
> +
> +def main():
> +    machine_resources = read_json_file(os.environ["MAAS_RESOURCES_FILE"])
> +
> +    hints_path = os.environ.get("MAAS_MACHINE_CONFIG_HINTS_FILE")
> +    result = provide_hints(machine_resources)
> +    serialized = json.dumps(result, indent=4, sort_keys=True)
> +    print(serialized)
> +    with open(hints_path, "w") as f:
> +        f.write(serialized)
> +
> +    return 0
> +
> +
> +if __name__ == "__main__":
> +    sys.exit(main())
> diff --git a/src/provisioningserver/refresh/50-maas-01-commissioning b/src/provisioningserver/refresh/50-maas-01-commissioning
> index 45ff285..44b114c 100755
> --- a/src/provisioningserver/refresh/50-maas-01-commissioning
> +++ b/src/provisioningserver/refresh/50-maas-01-commissioning
> @@ -48,4 +48,10 @@ if storage_path and os.path.exists(storage_path):
>      storage_config = read_json_file(storage_path)
>      data["storage-extra"] = storage_config
>  
> +# add machine config hints if present
> +hints_path = os.environ.get("MAAS_MACHINE_CONFIG_HINTS_FILE")
> +if hints_path and os.path.exists(hints_path):
> +    hints = read_json_file(hints_path)
> +    data["machine-config-hints"] = hints

I wonder if we shouldn't be consistent with "storage-extra" and call it "machine-extra"?

> +
>  print(json.dumps(data, indent=2))


-- 
https://code.launchpad.net/~igor-brovtsin/maas/+git/maas/+merge/442664
Your team MAAS Committers is subscribed to branch maas:master.