Fix raid config#2263
Open
janhorstmann wants to merge 2 commits intomainfrom
Open
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider avoiding in-place mutation of
node_attributeswhen poppingtarget_raid_config(e.g. work on a shallow copy) so that callers or subsequent logic that may rely on the full attributes dict are not surprised by missing keys. - When creating the stub node, the direct access to
node_attributes["driver"]will raise aKeyErrorif the driver is not present; if this is a realistic scenario, guard this lookup or raise a clearer error to aid debugging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider avoiding in-place mutation of `node_attributes` when popping `target_raid_config` (e.g. work on a shallow copy) so that callers or subsequent logic that may rely on the full attributes dict are not surprised by missing keys.
- When creating the stub node, the direct access to `node_attributes["driver"]` will raise a `KeyError` if the driver is not present; if this is a realistic scenario, guard this lookup or raise a clearer error to aid debugging.
## Individual Comments
### Comment 1
<location path="osism/tasks/conductor/ironic.py" line_range="368-369" />
<code_context>
+ # transitioned fast from managable to available later. It is also safer
+ # to not clean during sync, so that nodes may later be adopted with
+ # their provisioned data.
+ node = openstack.baremetal_node_create(
+ device.name, dict(automated_clean=False, driver=node_attributes["driver"])
+ )
+
</code_context>
<issue_to_address>
**issue:** Guard against missing `driver` in `node_attributes` when creating the stub node
This indexing will raise a `KeyError` if `driver` is ever omitted from `node_attributes`, which would abort the sync. Please either use `node_attributes.get("driver")` with a clear error, or validate/assert the presence of `driver` earlier so failures are explicit and easier to debug.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
The `target_raid_config` was popped from `node_attributes` before create and therefore was not available for comparision during node updates. Instead of also duplicating this code, the node creation was refactored to just create a stub node with the device name if it does not exist and unconditionally run the node update procedure afterwards. This way all `target_raid_config` releated code is situated in the update path and does not need to be duplicated. Signed-off-by: Jan Horstmann <horstmann@osism.tech>
The erase_devices_metadata deploy step is required when creating software RAID on prepartitioned devices. Signed-off-by: Jan Horstmann <horstmann@osism.tech>
e07053a to
ba92624
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.