Skip to content

Commit

Permalink
Throw proper Exception instead of generic one
Browse files Browse the repository at this point in the history
This to solve check flake8 B017

Signed-off-by: Luca Carrogu <carrogu@amazon.com>
  • Loading branch information
lukeseawalker committed Jan 16, 2023
1 parent 554c96f commit 7a27c6c
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,11 @@ def __init__(self, config):
self._compute_fleet_data = {}
self.set_config(config)

class ClusterStatusUpdateEventError(Exception):
"""Raised when there is a failure in updating the status due to an error on update event handler execution."""

pass

def set_config(self, config): # noqa: D102
if self._config != config:
log.info("Applying new clusterstatusmgtd config: %s", config)
Expand Down Expand Up @@ -366,7 +371,7 @@ def _call_update_event(self):
_write_json_to_file(self._config.computefleet_status_path, compute_fleet_data)
except Exception as e:
log.error("Update event handler failed during fleet status translation: %s", e)
raise
raise ClusterStatusManager.ClusterStatusUpdateEventError(e)

cinc_log_file = "/var/log/chef-client.log"
log.info("Calling update event handler, log can be found at %s", cinc_log_file)
Expand All @@ -384,9 +389,9 @@ def _call_update_event(self):
)
try:
_run_command(cmd, self._config.update_event_timeout_minutes)
except Exception:
except Exception as e:
log.error("Update event handler failed. Check log file %s", cinc_log_file)
raise
raise ClusterStatusManager.ClusterStatusUpdateEventError(e)

def _update_status(self, request_status, in_progress_status, final_status):
if self._compute_fleet_status == request_status:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ class CriticalError(Exception):
pass


class ConfigurationFieldNotFoundError(Exception):
"""Field not found in configuration"""

pass


def generate_fleet_config_file(output_file, input_file):
"""
Generate configuration file used by Fleet Manager in node daemon package.
Expand Down Expand Up @@ -87,7 +93,7 @@ def generate_fleet_config_file(output_file, input_file):
}

else:
raise Exception(
raise ConfigurationFieldNotFoundError(
"Instances or InstanceType field not found "
f"in queue: {queue}, compute resource: {compute_resource} configuration"
)
Expand Down
2 changes: 1 addition & 1 deletion test/unit/clusterstatusmgtd/test_clusterstatusmgtd.py
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ def test_call_update_event(self, mocker, status, translated_status, exception):
run_command_mock = mocker.patch("clusterstatusmgtd._run_command")
if isinstance(exception, Exception):
run_command_mock.side_effect = exception
with pytest.raises(Exception):
with pytest.raises(ClusterStatusManager.ClusterStatusUpdateEventError):
clusterstatus_manager._call_update_event()
else:
file_writer_mock = mocker.mock_open()
Expand Down
32 changes: 24 additions & 8 deletions test/unit/slurm/test_fleet_config_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,34 +12,42 @@

import pytest
from assertpy import assert_that
from slurm.pcluster_fleet_config_generator import generate_fleet_config_file
from slurm.pcluster_fleet_config_generator import (
ConfigurationFieldNotFoundError,
CriticalError,
generate_fleet_config_file,
)


@pytest.mark.parametrize(
"cluster_config, expected_message",
"cluster_config, expected_exception, expected_message",
[
({}, "Unable to find key 'Scheduling' in the configuration file"),
({"Scheduling": {}}, "Unable to find key 'SlurmQueues' in the configuration file"),
({"Scheduling": {"SlurmQueues": []}}, None),
({}, CriticalError, "Unable to find key 'Scheduling' in the configuration file"),
({"Scheduling": {}}, CriticalError, "Unable to find key 'SlurmQueues' in the configuration file"),
({"Scheduling": {"SlurmQueues": []}}, None, None),
(
{"Scheduling": {"SlurmQueues": [{"ComputeResources": []}]}},
CriticalError,
"Unable to find key 'Name' in the configuration file",
),
(
{"Scheduling": {"SlurmQueues": [{"Name": "q1"}]}},
CriticalError,
"Unable to find key 'CapacityType' in the configuration of queue: q1",
),
(
{"Scheduling": {"SlurmQueues": [{"Name": "q1", "CapacityType": "ONDEMAND"}]}},
CriticalError,
"Unable to find key 'ComputeResources' in the configuration of queue: q1",
),
({"Scheduling": {"SlurmQueues": [{"Name": "q1", "CapacityType": "SPOT", "ComputeResources": []}]}}, None),
({"Scheduling": {"SlurmQueues": [{"Name": "q1", "CapacityType": "SPOT", "ComputeResources": []}]}}, None, None),
(
{
"Scheduling": {
"SlurmQueues": [{"Name": "q1", "CapacityType": "SPOT", "ComputeResources": [{"Instances": []}]}]
}
},
CriticalError,
"Unable to find key 'Name' in the configuration of queue: q1",
),
(
Expand All @@ -55,6 +63,7 @@
]
}
},
ConfigurationFieldNotFoundError,
"Instances or InstanceType field not found in queue: q1, compute resource: cr1 configuration",
),
(
Expand All @@ -74,6 +83,7 @@
}
},
None,
None,
),
(
{
Expand All @@ -92,6 +102,7 @@
}
},
None,
None,
),
(
{
Expand All @@ -114,6 +125,7 @@
}
},
None,
None,
),
(
{
Expand All @@ -128,6 +140,7 @@
]
}
},
CriticalError,
"Unable to find key 'SpotPrice' in the configuration of queue: q1, compute resource: cr1",
),
(
Expand All @@ -146,6 +159,7 @@
}
},
None,
None,
),
(
{
Expand All @@ -161,6 +175,7 @@
]
}
},
CriticalError,
"Unable to find key 'Networking' in the configuration of queue: q1, compute resource: cr1",
),
(
Expand All @@ -178,16 +193,17 @@
]
}
},
CriticalError,
"Unable to find key 'SubnetIds' in the configuration of queue: q1, compute resource: cr1",
),
],
)
def test_generate_fleet_config_file_error_cases(mocker, tmpdir, cluster_config, expected_message):
def test_generate_fleet_config_file_error_cases(mocker, tmpdir, cluster_config, expected_exception, expected_message):
mocker.patch("slurm.pcluster_fleet_config_generator._load_cluster_config", return_value=cluster_config)
output_file = f"{tmpdir}/fleet-config.json"

if expected_message:
with pytest.raises(Exception, match=expected_message):
with pytest.raises(expected_exception, match=expected_message):
generate_fleet_config_file(output_file, input_file="fake")
else:
generate_fleet_config_file(output_file, input_file="fake")
Expand Down

0 comments on commit 7a27c6c

Please sign in to comment.