Skip to content

Update lastTransitionTime for volume and nic in the machineCondition #1428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Rohit-0505 wants to merge 3 commits into
base: main
Choose a base branch
from
+112 −17
Open

Update lastTransitionTime for volume and nic in the machineCondition #1428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c2be8b
update `lastTransitionTime` for volume and nic in the machineCondition
Rohit-0505 Dec 10, 2025
09731e7
update machineConditions to persist the conditions history
Rohit-0505 Dec 12, 2025
dd0b996
address review comments
Rohit-0505 Jan 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions api/compute/v1alpha1/machine_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,15 @@ const (
// MachineConditionType is a type a MachineCondition can have.
type MachineConditionType string

const (
// MachineConditionReady indicates that the machine is ready.
MachineConditionReady MachineConditionType = "MachineReady"
// MachineConditionVolumesReady indicates that the volumes are ready.
MachineConditionVolumesReady MachineConditionType = "VolumesReady"
// MachineConditionNetworkInterfacesReady indicates that the network interfaces are ready.
MachineConditionNetworkInterfacesReady MachineConditionType = "NetworkInterfacesReady"
)

// MachineCondition is one of the conditions of a machine.
type MachineCondition struct {
// Type is the type of the condition.
Expand Down
106 changes: 96 additions & 10 deletions poollet/machinepoollet/controllers/machine_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"context"
"errors"
"fmt"
"sort"
"strconv"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -504,7 +505,8 @@ func (r *MachineReconciler) updateMachineStatus(ctx context.Context, machine *co
machine.Status.ObservedGeneration = generation
machine.Status.Volumes = volumeStatuses
machine.Status.NetworkInterfaces = nicStatuses
machine.Status.Conditions = r.computeMachineConditions(state, volumeStatuses, nicStatuses, now)
computedConds := r.computeMachineConditions(state, volumeStatuses, nicStatuses, now)
machine.Status.Conditions = r.mergeMachineConditions(machine.Status.Conditions, computedConds)

if err := r.Status().Patch(ctx, machine, client.MergeFrom(base)); err != nil {
return fmt.Errorf("error patching status: %w", err)
Expand Down Expand Up @@ -551,7 +553,7 @@ func (r *MachineReconciler) computeMachineReadyCondition(state computev1alpha1.M
}

return computev1alpha1.MachineCondition{
Type: "Ready",
Type: computev1alpha1.MachineConditionReady,
Status: status,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Reason: reason,
Message: message,
Expand All @@ -565,22 +567,42 @@ func (r *MachineReconciler) computeVolumesReadyCondition(volumeStatuses []comput
}

status, reason, message := corev1.ConditionTrue, "VolumesReady", "All volumes are ready"
var lastTransitionTime *metav1.Time

for _, vs := range volumeStatuses {
volume := &vs.VolumeRef
volName := volume.Name
if volName == "" { //if local-disk volume is used, where no actual volume resource exists
volName = vs.Name
}
if vs.State != computev1alpha1.VolumeStateAttached {
status = corev1.ConditionFalse
reason = fmt.Sprintf("VolumeNotReady: %s", vs.Name)
message = fmt.Sprintf("Volume %s is not attached (state: %s)", vs.Name, vs.State)
reason = fmt.Sprintf("VolumeNotReady: %s", volName)
message = fmt.Sprintf("Volume %s is not attached (state: %s)", volName, vs.State)
lastTransitionTime = vs.LastStateTransitionTime
break
}
Comment thread
Rohit-0505 marked this conversation as resolved.
}

if status == corev1.ConditionTrue {
for _, vs := range volumeStatuses {
if vs.LastStateTransitionTime != nil && (lastTransitionTime == nil || vs.LastStateTransitionTime.After(lastTransitionTime.Time)) {
lastTransitionTime = vs.LastStateTransitionTime
}
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

transitionTime := now
if lastTransitionTime != nil {
transitionTime = *lastTransitionTime
}

return computev1alpha1.MachineCondition{
Type: computev1alpha1.MachineConditionType("VolumesReady"),
Type: computev1alpha1.MachineConditionVolumesReady,
Status: status,
Reason: reason,
Message: message,
LastTransitionTime: now,
LastTransitionTime: transitionTime,
}
}

Expand All @@ -590,25 +612,89 @@ func (r *MachineReconciler) computeNetworkInterfacesReadyCondition(nicStatuses [
}

status, reason, message := corev1.ConditionTrue, "NetworkInterfacesReady", "All network interfaces are ready"
var lastTransitionTime *metav1.Time

for _, nicStatus := range nicStatuses {
nic := &nicStatus.NetworkInterfaceRef
if nicStatus.State != computev1alpha1.NetworkInterfaceStateAttached {
status = corev1.ConditionFalse
reason = fmt.Sprintf("NetworkInterfaceNotReady: %s", nicStatus.Name)
message = fmt.Sprintf("Network interface %s is not attached (state: %s)", nicStatus.Name, nicStatus.State)
reason = fmt.Sprintf("NetworkInterfaceNotReady: %s", nic.Name)
message = fmt.Sprintf("Network interface %s is not attached (state: %s)", nic.Name, nicStatus.State)
lastTransitionTime = nicStatus.LastStateTransitionTime
break
}
Comment thread
Rohit-0505 marked this conversation as resolved.
}

if status == corev1.ConditionTrue {
for _, nicStatus := range nicStatuses {
if nicStatus.LastStateTransitionTime != nil && (lastTransitionTime == nil || nicStatus.LastStateTransitionTime.After(lastTransitionTime.Time)) {
lastTransitionTime = nicStatus.LastStateTransitionTime
}
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

transitionTime := now
if lastTransitionTime != nil {
transitionTime = *lastTransitionTime
}

return computev1alpha1.MachineCondition{
Type: computev1alpha1.MachineConditionType("NetworkInterfacesReady"),
Type: computev1alpha1.MachineConditionNetworkInterfacesReady,
Status: status,
Reason: reason,
Message: message,
LastTransitionTime: now,
LastTransitionTime: transitionTime,
}
}

func (r *MachineReconciler) mergeMachineConditions(
existing []computev1alpha1.MachineCondition,
current []computev1alpha1.MachineCondition,
) []computev1alpha1.MachineCondition {
updated := append([]computev1alpha1.MachineCondition{}, existing...)

lastStatus := make(map[string]corev1.ConditionStatus, len(existing))
for _, c := range existing {
lastStatus[string(c.Type)] = c.Status
}

for _, newCond := range current {
conditionType := string(newCond.Type)
if prev, ok := lastStatus[conditionType]; !ok || prev != newCond.Status {
updated = append(updated, newCond)
lastStatus[conditionType] = newCond.Status
}
}

// Group conditions by type and keep only the latest 10 per type (by LastTransitionTime)
const maxConditionsPerType = 10
conditionsByType := make(map[string][]computev1alpha1.MachineCondition)
for _, cond := range updated {
condType := string(cond.Type)
conditionsByType[condType] = append(conditionsByType[condType], cond)
}

var result []computev1alpha1.MachineCondition
// Sort condition types for deterministic ordering
types := make([]string, 0, len(conditionsByType))
for t := range conditionsByType {
types = append(types, t)
}
sort.Strings(types)
for _, t := range types {
conds := conditionsByType[t]
sort.Slice(conds, func(i, j int) bool {
return conds[i].LastTransitionTime.Before(&conds[j].LastTransitionTime)
})
if len(conds) > maxConditionsPerType {
conds = conds[len(conds)-maxConditionsPerType:]
}
result = append(result, conds...)
}

return result
}
Comment on lines +650 to +696
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jan 21, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if there's documentation or comments about condition history retention behavior
rg -n -i "stale|history|retain|persist" poollet/machinepoollet/controllers/machine_controller.go

# Check if there are tests covering the stale condition scenario
rg -n -A 5 "mergeMachineConditions" poollet/machinepoollet/controllers/machine_controller_test.go

Repository: ironcore-dev/ironcore

Length of output: 47

🏁 Script executed:

#!/bin/bash
# List files to understand directory structure
find . -name "machine_controller.go" -type f
find . -name "machine_controller_test.go" -type f

# Check if the files exist and look for any comments around condition handling
find . -name "machine_controller.go" -type f -exec head -700 {} \; | tail -100

Repository: ironcore-dev/ironcore

Length of output: 3575

🏁 Script executed:

#!/bin/bash
# Check for mergeMachineConditions tests
grep -n -A 20 "mergeMachineConditions" ./poollet/machinepoollet/controllers/machine_controller_test.go

# Check how current conditions are generated - look for function calls to condition functions
grep -n "computeVolumesReadyCondition\|computeNetworkInterfacesReadyCondition" ./poollet/machinepoollet/controllers/machine_controller.go

# Find where mergeMachineConditions is called to understand the flow
grep -n "mergeMachineConditions" ./poollet/machinepoollet/controllers/machine_controller.go

Repository: ironcore-dev/ironcore

Length of output: 728

🏁 Script executed:

#!/bin/bash
# Read the computeVolumesReadyCondition function to see what it returns when empty
sed -n '564,608p' ./poollet/machinepoollet/controllers/machine_controller.go

# Check where computedConds is built to understand the full flow
sed -n '520,545p' ./poollet/machinepoollet/controllers/machine_controller.go

Repository: ironcore-dev/ironcore

Length of output: 2410

🏁 Script executed:

#!/bin/bash
# Search for mergeMachineConditions tests more thoroughly
grep -n "mergeMachineConditions\|TestMergeMachine" ./poollet/machinepoollet/controllers/machine_controller_test.go

# Also check if there are any tests about condition removal or stale conditions
grep -n -i "stale\|remove.*condition\|condition.*remov" ./poollet/machinepoollet/controllers/machine_controller_test.go

# Look at the actual test file around mergeMachineConditions if it exists
grep -n -B 5 -A 30 "func.*mergeMachine" ./poollet/machinepoollet/controllers/machine_controller_test.go

Repository: ironcore-dev/ironcore

Length of output: 47

🏁 Script executed:

#!/bin/bash
# List test functions in the test file
grep -n "^func Test" ./poollet/machinepoollet/controllers/machine_controller_test.go | head -20

# Get overview of test file
wc -l ./poollet/machinepoollet/controllers/machine_controller_test.go

# Search for any mention of conditions in tests
grep -n -i "condition" ./poollet/machinepoollet/controllers/machine_controller_test.go | head -20

Repository: ironcore-dev/ironcore

Length of output: 1454

🏁 Script executed:

#!/bin/bash
# Search for any documentation files mentioning conditions
find . -name "*.md" -type f -exec grep -l "condition" {} \; | head -10

# Search for any design docs in common locations
find . -path "*/docs/*" -name "*.md" -type f 2>/dev/null | head -10

# Check if there are any comments in the reconciliation logic about condition handling
sed -n '490,550p' ./poollet/machinepoollet/controllers/machine_controller.go | grep -n -i "condition\|TODO\|FIXME"

# Look at the entire reconciliation function to understand condition flow
grep -n "func (r \*MachineReconciler) Reconcile" ./poollet/machinepoollet/controllers/machine_controller.go

Repository: ironcore-dev/ironcore

Length of output: 1857

🏁 Script executed:

#!/bin/bash
# Get the full mergeMachineConditions function and surrounding context
sed -n '650,700p' ./poollet/machinepoollet/controllers/machine_controller.go

# Check if there's any cleanup or removal logic elsewhere
grep -n "Status.Conditions\s*=" ./poollet/machinepoollet/controllers/machine_controller.go | head -10

Repository: ironcore-dev/ironcore

Length of output: 1851

🏁 Script executed:

#!/bin/bash
# Search for condition-related documentation
grep -n -i "condition\|history" ./docs/architecture/*.md ./docs/concepts/*.md 2>/dev/null | grep -i "condition"

# Check if there are any proposals or design decisions about conditions
grep -r -i "condition.*history\|audit\|retain" ./docs/proposals/ 2>/dev/null

# Look for any comments in related files about condition lifecycle
grep -n -i "stale\|obsolete\|expired\|cleanup" ./poollet/machinepoollet/controllers/*.go

Repository: ironcore-dev/ironcore

Length of output: 3404

Remove stale conditions when their corresponding resources are deleted.

The current merge logic preserves all existing conditions and only appends new ones when status changes. This causes conditions from removed resources (e.g., VolumesReady after all volumes are deleted) to persist indefinitely. For example, a machine that previously had volumes with VolumesReady=True will continue showing that condition even after all volumes are removed.

While the maxConditionsPerType cap at 10 prevents unbounded growth, the fundamental issue is that obsolete condition types remain in the status, creating misleading information about the machine's current state.

Consider either: (1) explicitly removing condition types that are no longer applicable (when a resource type goes from having items to zero), or (2) adding a mechanism to mark/filter stale conditions so they're preserved for audit/history but clearly distinguished from current state.

🤖 Prompt for AI Agents 
In `@poollet/machinepoollet/controllers/machine_controller.go` around lines 650 -
696, The mergeMachineConditions function currently keeps all existing conditions
even when a condition type no longer appears in current (causing stale types to
persist); update mergeMachineConditions to detect condition types present in
current (build a set from the current slice) and drop any existing conditions
whose type is absent from that set so obsolete types are removed from the merged
result; keep the rest of the logic (appending new status changes, grouping by
type, enforcing maxConditionsPerType, sorting) intact and reference the symbols
mergeMachineConditions, existing, current, conditionsByType and
maxConditionsPerType when making the change.


func (r *MachineReconciler) prepareIRIPower(power computev1alpha1.Power) (iri.Power, error) {
switch power {
case computev1alpha1.PowerOn:
Expand Down
14 changes: 7 additions & 7 deletions poollet/machinepoollet/controllers/machine_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ var _ = Describe("MachineController", func() {
))
Eventually(Object(machine)).Should(SatisfyAll(
HaveField("Status.Conditions", ContainElement(MatchFields(IgnoreExtras, Fields{
"Type": Equal(computev1alpha1.MachineConditionType("Ready")),
"Type": Equal(computev1alpha1.MachineConditionType("MachineReady")),
"Status": Equal(corev1.ConditionFalse),
"Reason": Equal("Pending"),
"Message": Equal("Machine is pending"),
Expand All @@ -206,15 +206,15 @@ var _ = Describe("MachineController", func() {
HaveField("Status.Conditions", ContainElement(MatchFields(IgnoreExtras, Fields{
"Type": Equal(computev1alpha1.MachineConditionType("VolumesReady")),
"Status": Equal(corev1.ConditionFalse),
"Reason": Equal(fmt.Sprintf("VolumeNotReady: %s", "primary")),
"Message": Equal(fmt.Sprintf("Volume %s is not attached (state: %s)", "primary", "Pending")),
"Reason": Equal(fmt.Sprintf("VolumeNotReady: %s", volume.Name)),
"Message": Equal(fmt.Sprintf("Volume %s is not attached (state: %s)", volume.Name, "Pending")),
"LastTransitionTime": Not(BeNil()),
}))),
HaveField("Status.Conditions", ContainElement(MatchFields(IgnoreExtras, Fields{
"Type": Equal(computev1alpha1.MachineConditionType("NetworkInterfacesReady")),
"Status": Equal(corev1.ConditionFalse),
"Reason": Equal(fmt.Sprintf("NetworkInterfaceNotReady: %s", "primary")),
"Message": Equal(fmt.Sprintf("Network interface %s is not attached (state: %s)", "primary", "Pending")),
"Reason": Equal(fmt.Sprintf("NetworkInterfaceNotReady: %s", nic.Name)),
"Message": Equal(fmt.Sprintf("Network interface %s is not attached (state: %s)", nic.Name, "Pending")),
"LastTransitionTime": Not(BeNil()),
}))),
))
Expand Down Expand Up @@ -507,7 +507,7 @@ var _ = Describe("MachineController", func() {
By("waiting for the machine conditions to be updated")
Eventually(Object(machine)).Should(SatisfyAll(
HaveField("Status.Conditions", ContainElement(MatchFields(IgnoreExtras, Fields{
"Type": Equal(computev1alpha1.MachineConditionType("Ready")),
"Type": Equal(computev1alpha1.MachineConditionType("MachineReady")),
"Status": Equal(corev1.ConditionTrue),
"Reason": Equal("Running"),
"Message": Equal("Machine is running"),
Expand All @@ -527,7 +527,7 @@ var _ = Describe("MachineController", func() {
By("waiting for the machine conditions to be updated")
Eventually(Object(machine)).Should(SatisfyAll(
HaveField("Status.Conditions", ContainElement(MatchFields(IgnoreExtras, Fields{
"Type": Equal(computev1alpha1.MachineConditionType("Ready")),
"Type": Equal(computev1alpha1.MachineConditionType("MachineReady")),
"Status": Equal(corev1.ConditionFalse),
"Reason": Equal("Terminating"),
"Message": Equal("Machine is terminating or terminated"),
Expand Down
Loading