Skip to content

Conversation

@stefanhipfel
Copy link
Contributor

This change reads the critical SEL (system event logs) from a BMC to update the server health condition.

open points:

  • what if there are multiple critical logs. Condition can only be set to false once
  • should the logs be cleared when adding a new server to metal-api?

Copy link
Contributor

@Nuckal777 Nuckal777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. 👍

Comment on lines +348 to +362
for _, logEntry := range logs {
serverBase := server.DeepCopy()
if err := acc.UpdateSlice(
&server.Status.Conditions,
ServerHealthyConditionType,
conditionutils.UpdateStatus(corev1.ConditionFalse),
conditionutils.UpdateReason(logEntry.Name),
conditionutils.UpdateMessage(logEntry.Message),
); err != nil {
return fmt.Errorf("failed to patch condition %s: %w", corev1.ConditionFalse, err)
}
if err := r.Status().Patch(ctx, server, client.MergeFrom(serverBase)); err != nil {
return fmt.Errorf("failed to patch BMC conditions: %w", err)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop only stores the last entry in the conditions slice, as it is overwriting any previous reasons and messages. I think the logs should be concatenated into a single message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants