Skip to content

Commit

Permalink
ensure that build jobs are always reconciled
Browse files Browse the repository at this point in the history 
If the machine-os-builder pod is stopped and an active build job
completes  before the machine-os-builder pod is rescheduled, it will be
ignored. In this situation, we should check if the job is in a terminal
state and take the appropriate action if it is.

This also opportunistically cleans up the buildprogress -> conditions
mapping and adds additional test cases for detecting state changes.
  • Loading branch information
@cheesesashimi
cheesesashimi committed Feb 6, 2025
1 parent cfdda14 commit dd1daba
Show file tree
Hide file tree
Showing 13 changed files with 824 additions and 297 deletions.
216 changes: 216 additions & 0 deletions pkg/apihelpers/machineosbuild.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,219 @@ func IsMachineOSBuildConditionPresentAndEqual(conditions []metav1.Condition, con
}
return false
}

// Represents the successful conditions for a MachineOSBuild.
func MachineOSBuildSucceededConditions() []metav1.Condition {
return []metav1.Condition{
{
Type: string(mcfgv1.MachineOSBuildPrepared),
Status: metav1.ConditionFalse,
Reason: "Prepared",
Message: "Build Prepared and Pending",
},
{
Type: string(mcfgv1.MachineOSBuilding),
Status: metav1.ConditionFalse,
Reason: "Building",
Message: "Image Build In Progress",
},
{
Type: string(mcfgv1.MachineOSBuildFailed),
Status: metav1.ConditionFalse,
Reason: "Failed",
Message: "Build Failed",
},
{
Type: string(mcfgv1.MachineOSBuildInterrupted),
Status: metav1.ConditionFalse,
Reason: "Interrupted",
Message: "Build Interrupted",
},
{
Type: string(mcfgv1.MachineOSBuildSucceeded),
Status: metav1.ConditionTrue,
Reason: "Ready",
Message: "Build Ready",
},
}
}

// Represents the pending conditions for a MachineOSBuild.
func MachineOSBuildPendingConditions() []metav1.Condition {
return []metav1.Condition{
{
Type: string(mcfgv1.MachineOSBuildPrepared),
Status: metav1.ConditionTrue,
Reason: "Prepared",
Message: "Build Prepared and Pending",
},
{
Type: string(mcfgv1.MachineOSBuilding),
Status: metav1.ConditionFalse,
Reason: "Building",
Message: "Image Build In Progress",
},
{
Type: string(mcfgv1.MachineOSBuildFailed),
Status: metav1.ConditionFalse,
Reason: "Failed",
Message: "Build Failed",
},
{
Type: string(mcfgv1.MachineOSBuildInterrupted),
Status: metav1.ConditionFalse,
Reason: "Interrupted",
Message: "Build Interrupted",
},
{
Type: string(mcfgv1.MachineOSBuildSucceeded),
Status: metav1.ConditionFalse,
Reason: "Ready",
Message: "Build Ready",
},
}
}

// Represents the running conditions for a MachineOSBuild.
func MachineOSBuildRunningConditions() []metav1.Condition {
return []metav1.Condition{
{
Type: string(mcfgv1.MachineOSBuildPrepared),
Status: metav1.ConditionFalse,
Reason: "Prepared",
Message: "Build Prepared and Pending",
},
{
Type: string(mcfgv1.MachineOSBuilding),
Status: metav1.ConditionTrue,
Reason: "Building",
Message: "Image Build In Progress",
},
{
Type: string(mcfgv1.MachineOSBuildFailed),
Status: metav1.ConditionFalse,
Reason: "Failed",
Message: "Build Failed",
},
{
Type: string(mcfgv1.MachineOSBuildInterrupted),
Status: metav1.ConditionFalse,
Reason: "Interrupted",
Message: "Build Interrupted",
},
{
Type: string(mcfgv1.MachineOSBuildSucceeded),
Status: metav1.ConditionFalse,
Reason: "Ready",
Message: "Build Ready",
},
}
}

// Represents the failure conditions for a MachineOSBuild.
func MachineOSBuildFailedConditions() []metav1.Condition {
return []metav1.Condition{
{
Type: string(mcfgv1.MachineOSBuildPrepared),
Status: metav1.ConditionFalse,
Reason: "Prepared",
Message: "Build Prepared and Pending",
},
{
Type: string(mcfgv1.MachineOSBuilding),
Status: metav1.ConditionFalse,
Reason: "Building",
Message: "Image Build In Progress",
},
{
Type: string(mcfgv1.MachineOSBuildFailed),
Status: metav1.ConditionTrue,
Reason: "Failed",
Message: "Build Failed",
},
{
Type: string(mcfgv1.MachineOSBuildInterrupted),
Status: metav1.ConditionFalse,
Reason: "Interrupted",
Message: "Build Interrupted",
},
{
Type: string(mcfgv1.MachineOSBuildSucceeded),
Status: metav1.ConditionFalse,
Reason: "Ready",
Message: "Build Ready",
},
}
}

// Represents the interrupted conditions for a MachineOSBuild.
func MachineOSBuildInterruptedConditions() []metav1.Condition {
return []metav1.Condition{
{
Type: string(mcfgv1.MachineOSBuildPrepared),
Status: metav1.ConditionFalse,
Reason: "Prepared",
Message: "Build Prepared and Pending",
},
{
Type: string(mcfgv1.MachineOSBuilding),
Status: metav1.ConditionFalse,
Reason: "Building",
Message: "Image Build In Progress",
},
{
Type: string(mcfgv1.MachineOSBuildFailed),
Status: metav1.ConditionFalse,
Reason: "Failed",
Message: "Build Failed",
},
{
Type: string(mcfgv1.MachineOSBuildInterrupted),
Status: metav1.ConditionTrue,
Reason: "Interrupted",
Message: "Build Interrupted",
},
{
Type: string(mcfgv1.MachineOSBuildSucceeded),
Status: metav1.ConditionFalse,
Reason: "Ready",
Message: "Build Ready",
},
}
}

// Represents the initial MachineOSBuild state (all conditions false).
func MachineOSBuildInitialConditions() []metav1.Condition {
return []metav1.Condition{
{
Type: string(mcfgv1.MachineOSBuildPrepared),
Status: metav1.ConditionFalse,
Reason: "Prepared",
Message: "Build Prepared and Pending",
},
{
Type: string(mcfgv1.MachineOSBuilding),
Status: metav1.ConditionFalse,
Reason: "Building",
Message: "Image Build In Progress",
},
{
Type: string(mcfgv1.MachineOSBuildFailed),
Status: metav1.ConditionFalse,
Reason: "Failed",
Message: "Build Failed",
},
{
Type: string(mcfgv1.MachineOSBuildInterrupted),
Status: metav1.ConditionFalse,
Reason: "Interrupted",
Message: "Build Interrupted",
},
{
Type: string(mcfgv1.MachineOSBuildSucceeded),
Status: metav1.ConditionFalse,
Reason: "Ready",
Message: "Build Ready",
},
}
}
6 changes: 4 additions & 2 deletions pkg/controller/build/helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,11 @@ func isMachineOSBuildStatusUpdateNeeded(oldStatus, curStatus mcfgv1.MachineOSBui
return true, fmt.Sprintf("transitioned from initial state -> transient state (%s)", curTransientState)
}

// From pending -> building.
// From pending -> building, but not building -> pending.
if oldState.IsInTransientState() && curState.IsInTransientState() && oldTransientState != curTransientState {
return true, fmt.Sprintf("transitioned from transient state (%s) -> transient state (%s)", oldTransientState, curTransientState)
reason := fmt.Sprintf("transitioned from transient state (%s) -> transient state (%s)", oldTransientState, curTransientState)
isValid := oldTransientState == mcfgv1.MachineOSBuildPrepared && curTransientState == mcfgv1.MachineOSBuilding
return isValid, reason
}

oldTerminalState := oldState.GetTerminalState()
Expand Down
118 changes: 118 additions & 0 deletions pkg/controller/build/helpers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ package build

import (
"context"
"fmt"
"testing"

mcfgv1 "github.com/openshift/api/machineconfiguration/v1"
"github.com/openshift/machine-config-operator/pkg/apihelpers"
"github.com/openshift/machine-config-operator/pkg/controller/build/fixtures"
ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -80,3 +82,119 @@ func TestValidateOnClusterBuildConfig(t *testing.T) {
})
}
}

// This test validates that we have correctly identified if the MachineOSBuild
// should be updated based upon comparing the old and current status of the
// MachineOSBuild. It is worth noting that the current MachineOSBuild status
// can come from the imagebuilder.MachineOSBuildStatus() method which maps the
// current job state to the MachineOSBuild state.
func TestIsMachineOSBuildStatusUpdateNeeded(t *testing.T) {
t.Parallel()

initialConditions := func() map[mcfgv1.BuildProgress][]metav1.Condition {
return map[mcfgv1.BuildProgress][]metav1.Condition{
// This value is not part of the OCL API and is here solely for testing purposes.
"Initial": apihelpers.MachineOSBuildInitialConditions(),
}
}

testCases := []struct {
name string
old map[mcfgv1.BuildProgress][]metav1.Condition
current map[mcfgv1.BuildProgress][]metav1.Condition
expected bool
}{
// These are valid state transitions. In other words, when one of these
// state transitions is identified, the MachineOSBuild status object should
// be updated.
{
name: "Initial -> Terminal",
old: initialConditions(),
current: ctrlcommon.MachineOSBuildTerminalStates(),
expected: true,
},
{
name: "Initial -> Transient",
old: initialConditions(),
current: ctrlcommon.MachineOSBuildTransientStates(),
expected: true,
},
{
name: "Transient -> Terminal",
old: ctrlcommon.MachineOSBuildTransientStates(),
current: ctrlcommon.MachineOSBuildTerminalStates(),
expected: true,
},
{
name: "Pending -> Running",
old: map[mcfgv1.BuildProgress][]metav1.Condition{
mcfgv1.MachineOSBuildPrepared: ctrlcommon.MachineOSBuildTransientStates()[mcfgv1.MachineOSBuildPrepared],
},
current: map[mcfgv1.BuildProgress][]metav1.Condition{
mcfgv1.MachineOSBuilding: ctrlcommon.MachineOSBuildTransientStates()[mcfgv1.MachineOSBuilding],
},
expected: true,
},
// These are invalid state transitions. In other words, when one of these
// state transitions is observed, the MachineOSBuild object should not be
// updated because they are invalid and make no sense.
{
name: "Terminal -> Initial",
old: ctrlcommon.MachineOSBuildTerminalStates(),
current: initialConditions(),
expected: false,
},
{
name: "Transient -> Initial",
old: ctrlcommon.MachineOSBuildTransientStates(),
current: initialConditions(),
expected: false,
},
{
name: "Initial -> Initial",
old: initialConditions(),
current: initialConditions(),
expected: false,
},
{
name: "Terminal -> Terminal",
old: ctrlcommon.MachineOSBuildTerminalStates(),
current: ctrlcommon.MachineOSBuildTerminalStates(),
expected: false,
},
{
name: "Running -> Pending",
old: map[mcfgv1.BuildProgress][]metav1.Condition{
mcfgv1.MachineOSBuilding: ctrlcommon.MachineOSBuildTransientStates()[mcfgv1.MachineOSBuilding],
},
current: map[mcfgv1.BuildProgress][]metav1.Condition{
mcfgv1.MachineOSBuildPrepared: ctrlcommon.MachineOSBuildTransientStates()[mcfgv1.MachineOSBuildPrepared],
},
expected: false,
},
}

for _, testCase := range testCases {
for oldName, old := range testCase.old {
for currentName, current := range testCase.current {
t.Run(fmt.Sprintf("%s: %s -> %s", testCase.name, oldName, currentName), func(t *testing.T) {
oldStatus := mcfgv1.MachineOSBuildStatus{
Conditions: old,
}

curStatus := mcfgv1.MachineOSBuildStatus{
Conditions: current,
}

result, reason := isMachineOSBuildStatusUpdateNeeded(oldStatus, curStatus)

if testCase.expected {
assert.True(t, result, reason)
} else {
assert.False(t, result, reason)
}
})
}
}
}
}
Loading

0 comments on commit dd1daba

Please sign in to comment.