charmed-hpc / slurmutils

Utilities and APIs for interfacing with the Slurm workload manager ⚙️🔌
GNU Lesser General Public License v3.0
1 stars 3 forks source link

[Bug]: `SlurmConfig.from_dict` not verifying correct shape of dict-like and array-like configs #17

Open jedel1043 opened 3 weeks ago

jedel1043 commented 3 weeks ago

Bug Description

The SlurmConfig.from_dict method doesn't verify that the input fields for dict-like and array-like configs are of the correct shape; Nodes, DownNodes, Partitions, etc.

To Reproduce

config = SlurmConfig.from_dict({
    "Nodes": [
        {
            "NodeAddr": "10.152.28.98",
            "CPUs": "9",
            "RealMemory": "9000",
            "TmpDisk": "90000",
        },
    ],
    "DownNodes": {
        "DownNodes": ["juju-c9fc6f-6", "juju-c9fc6f-7"],
        "State": "DOWN",
        "Reason": "New nodes",
    },
    "Partitions": [
        {
            "MaxTime": "10",
            "MaxNodes": "5",
            "State": "UP",
        },
    ],
})

print(config.nodes) // Prints the malformed "Nodes" array
print(config.down_nodes) // Prints the malformed "DownNodes" object
print(config.partitions) // Prints the malformed "Partitions" array

Environment

Ran locally on Ubuntu Noble (24.04)

> python3 -m pip --version                                                                      
pip 24.0 from *** (python 3.12)

Additional context

This operation should probably throw when the input configs are not of the correct shape.

NucciTheBoss commented 3 weeks ago

Yeah, this is kinda expected. The from_dict(...) constructor is really no thrills for SlurmConfig. It just checks against the SlurmConfigOptionSet to ensure that there are no unexpected configuration keys passed to __init__.

This should be a milestone for a future slurmutils. Ideally we can add this form checking back in when we refine the story around the NodeMap, PartitionMap, etc data structures that can help verify is the data is good or not.

daniell-olaitan commented 13 hours ago

I believe adding a temporary fix to validate the shape of dict-like and array-like fields (e.g., Nodes, DownNodes, Partitions) could significantly improve the from_dict method's robustness. While I understand that the current implementation primarily checks for unexpected keys, adding basic structural validation could prevent malformed data from being passed through and causing subtle bugs.

A simple check for fields like Nodes (ensuring it's a list of dicts, for example) would help catch configuration errors early, without requiring a major overhaul. This would serve as an interim solution until the more refined NodeMap and PartitionMap data structures are implemented.

I’d love to contribute to implementing this temporary fix if the team agrees it’s a helpful step forward.

@NucciTheBoss