Skip to content

Commit

Permalink
Merge pull request #4135 from cheesesashimi/zzlotnik/fix-maxunavailab…
Browse files Browse the repository at this point in the history 
…le-bug

OCPBUGS-24705: consider currentImage and desiredImage annotations
  • Loading branch information
@openshift-merge-bot
openshift-merge-bot[bot] committed Feb 1, 2024
2 parents 030bcf7 + 124dd45 commit 523ea84
Show file tree
Hide file tree
Showing 4 changed files with 304 additions and 4 deletions.
52 changes: 51 additions & 1 deletion pkg/controller/common/layered_node_state.go
View file
Expand Up @@ -131,16 +131,66 @@ func isNodeDone(node *corev1.Node) bool {
if node.Annotations == nil {
return false
}

if !isNodeConfigDone(node) {
return false
}

if !isNodeImageDone(node) {
return false
}

if !isNodeMCDState(node, daemonconsts.MachineConfigDaemonStateDone) {
return false
}

return true
}

// Determines if a node's configuration is done based upon the presence and
// equality of the current / desired config annotations.
func isNodeConfigDone(node *corev1.Node) bool {
cconfig, ok := node.Annotations[daemonconsts.CurrentMachineConfigAnnotationKey]
if !ok || cconfig == "" {
return false
}

dconfig, ok := node.Annotations[daemonconsts.DesiredMachineConfigAnnotationKey]
if !ok || dconfig == "" {
return false
}

return cconfig == dconfig && isNodeMCDState(node, daemonconsts.MachineConfigDaemonStateDone)
return cconfig == dconfig
}

// Determines if a node's image is done based upon the presence of the current
// / desired image annotations. Note: Unlike the above function, if both
// annotations are missing, we return "True" because we do not want to take
// these annotations into consideration. Only when one (or both) of these
// annotations is present should we take them into consideration.
// them into consideration.
func isNodeImageDone(node *corev1.Node) bool {
desired, desiredOK := node.Annotations[daemonconsts.DesiredImageAnnotationKey]
current, currentOK := node.Annotations[daemonconsts.CurrentImageAnnotationKey]

// If neither annotation exists, we are "done" because there are no image
// annotations to consider.
if !desiredOK && !currentOK {
return true
}

// If the desired annotation is empty, we are not "done" yet.
if desired == "" {
return false
}

// If the current annotation is empty, we are not "done" yet.
if current == "" {
return false
}

// If the current image equals the desired image and neither are empty, we are done.
return desired == current
}

// isNodeDoneAt checks whether a node is fully updated to a targetConfig
Expand Down
142 changes: 140 additions & 2 deletions pkg/controller/common/layered_node_state_test.go
View file
Expand Up @@ -25,6 +25,7 @@ func newLayeredNode(currentConfig, desiredConfig, currentImage, desiredImage str
nb := helpers.NewNodeBuilder("")
nb.WithCurrentConfig(currentConfig).WithDesiredConfig(desiredConfig)
nb.WithCurrentImage(currentImage).WithDesiredImage(desiredImage)
nb.WithNodeReady()
return nb.Node()
}

Expand Down Expand Up @@ -90,9 +91,146 @@ func TestLayeredNodeState(t *testing.T) {
pool: newLayeredMachineConfigPoolWithImage(machineConfigV1, imageV1),
},
{
name: "layered node machineconfig outdated",
node: newLayeredNode(machineConfigV0, machineConfigV1, imageV1, imageV1),
pool: newLayeredMachineConfigPoolWithImage(machineConfigV1, imageV1),
name: "layered node machineconfig outdated",
isUnavailable: true,
isDesiredEqualToPool: true,
},
{
name: "Node becoming layered should be unavailable",
node: helpers.NewNodeBuilder("").
WithEqualConfigs(machineConfigV0).
WithDesiredImage(imageV0).
WithMCDState(daemonconsts.MachineConfigDaemonStateWorking).
WithNodeReady().
Node(),
pool: newLayeredMachineConfigPoolWithImage(machineConfigV0, imageV0),
isUnavailable: true,
isDesiredEqualToPool: true,
},
{
name: "Node becoming layered should be unavailable even if the MCD hasn't started yet",
node: helpers.NewNodeBuilder("").
WithEqualConfigs(machineConfigV0).
WithDesiredImage(imageV0).
WithMCDState(daemonconsts.MachineConfigDaemonStateDone).
WithNodeReady().
Node(),
pool: newLayeredMachineConfigPoolWithImage(machineConfigV0, imageV0),
isUnavailable: true,
isDesiredEqualToPool: true,
},
{
name: "Node changing configs should be unavailable",
node: helpers.NewNodeBuilder("").
WithConfigs(machineConfigV0, machineConfigV1).
WithMCDState(daemonconsts.MachineConfigDaemonStateWorking).
WithNodeReady().
Node(),
pool: newLayeredMachineConfigPool(machineConfigV0),
isUnavailable: true,
},
{
name: "Node changing configs should be unavailable even if the MCD hasn't started yet",
node: helpers.NewNodeBuilder("").
WithConfigs(machineConfigV0, machineConfigV1).
WithMCDState(daemonconsts.MachineConfigDaemonStateDone).
WithNodeReady().
Node(),
pool: newLayeredMachineConfigPool(machineConfigV0),
isUnavailable: true,
},
{
name: "Node changing images should be unavailable",
node: helpers.NewNodeBuilder("").
WithConfigs(machineConfigV0, machineConfigV0).
WithImages(imageV0, imageV1).
WithMCDState(daemonconsts.MachineConfigDaemonStateWorking).
WithNodeReady().
Node(),
pool: newLayeredMachineConfigPoolWithImage(machineConfigV0, imageV1),
isUnavailable: true,
isDesiredEqualToPool: true,
},
{
name: "Node changing images should be unavailable even if the MCD hasn't started yet",
node: helpers.NewNodeBuilder("").
WithConfigs(machineConfigV0, machineConfigV0).
WithImages(imageV0, imageV1).
WithMCDState(daemonconsts.MachineConfigDaemonStateDone).
WithNodeReady().
Node(),
pool: newLayeredMachineConfigPoolWithImage(machineConfigV0, imageV1),
isUnavailable: true,
isDesiredEqualToPool: true,
},
{
name: "Degraded node should be unavailable",
node: helpers.NewNodeBuilder("").
WithEqualConfigs(machineConfigV0).
WithMCDState(daemonconsts.MachineConfigDaemonStateDegraded).
WithNodeReady().
Node(),
pool: newLayeredMachineConfigPool(machineConfigV0),
isUnavailable: true,
isDesiredEqualToPool: true,
},
{
name: "Degraded layered node should be unavailable",
node: helpers.NewNodeBuilder("").
WithEqualConfigs(machineConfigV0).
WithEqualImages(imageV0).
WithMCDState(daemonconsts.MachineConfigDaemonStateDegraded).
WithNodeReady().
Node(),
pool: newLayeredMachineConfigPoolWithImage(machineConfigV0, imageV0),
isUnavailable: true,
isDesiredEqualToPool: true,
},
{
name: "Degraded layered node should be unavailable while transitioning images",
node: helpers.NewNodeBuilder("").
WithCurrentImage(imageV0).
WithDesiredImage(imageV1).
WithMCDState(daemonconsts.MachineConfigDaemonStateDegraded).
WithNodeReady().
Node(),
pool: newLayeredMachineConfigPoolWithImage(machineConfigV0, imageV1),
isUnavailable: true,
},
{
name: "Rebooting node should be unavailable",
node: helpers.NewNodeBuilder("").
WithEqualConfigs(machineConfigV0).
WithMCDState(daemonconsts.MachineConfigDaemonStateRebooting).
WithNodeReady().
Node(),
pool: newLayeredMachineConfigPool(machineConfigV0),
isUnavailable: true,
isDesiredEqualToPool: true,
},
{
name: "Rebooting layered node should be unavailable",
node: helpers.NewNodeBuilder("").
WithEqualConfigs(machineConfigV0).
WithEqualImages(imageV0).
WithMCDState(daemonconsts.MachineConfigDaemonStateRebooting).
WithNodeReady().
Node(),
pool: newLayeredMachineConfigPoolWithImage(machineConfigV0, imageV0),
isUnavailable: true,
isDesiredEqualToPool: true,
},
{
name: "Unready node should be unavailable",
node: helpers.NewNodeBuilder("").
WithEqualConfigs(machineConfigV0).
WithEqualImages(imageV0).
WithMCDState(daemonconsts.MachineConfigDaemonStateRebooting).
WithNodeNotReady().
Node(),
pool: newLayeredMachineConfigPoolWithImage(machineConfigV0, imageV0),
isUnavailable: true,
isDesiredEqualToPool: true,
},
Expand All @@ -108,7 +246,7 @@ func TestLayeredNodeState(t *testing.T) {
if test.pool != nil {
assert.Equal(t, test.isDoneAt, lns.IsDoneAt(test.pool), "IsDoneAt()")
assert.Equal(t, test.isDesiredEqualToPool, lns.IsDesiredEqualToPool(test.pool), "IsDesiredEqualToPool()")
assert.Equal(t, test.isUnavailable, lns.IsUnavailable(test.pool), "IsUnavailablePool()")
assert.Equal(t, test.isUnavailable, lns.IsUnavailable(test.pool), "IsUnavailable()")
}

if t.Failed() {
Expand Down
57 changes: 56 additions & 1 deletion pkg/controller/node/node_controller_test.go
View file
Expand Up @@ -709,7 +709,62 @@ func TestGetCandidateMachines(t *testing.T) {
otherCandidates: []string{"node-5", "node-6"},
capacity: 2,
layeredPool: true,
}}
}, {
// Targets https://issues.redhat.com/browse/OCPBUGS-24705.
name: "Node has received desiredImage annotation but MCD has not yet started working",
progress: 1,
nodes: []*corev1.Node{
// Need to set WithNodeReady() on all nodes to avoid short-circuiting.
helpers.NewNodeBuilder("node-0").
WithEqualConfigs(machineConfigV1).
WithDesiredImage(imageV1).
WithNodeReady().
WithMCDState(daemonconsts.MachineConfigDaemonStateDone).
Node(),
helpers.NewNodeBuilder("node-1").
WithEqualConfigs(machineConfigV1).
WithNodeReady().
WithMCDState(daemonconsts.MachineConfigDaemonStateDone).
Node(),
helpers.NewNodeBuilder("node-2").
WithEqualConfigs(machineConfigV1).
WithNodeReady().
WithMCDState(daemonconsts.MachineConfigDaemonStateDone).
Node(),
},
expected: nil,
otherCandidates: nil,
capacity: 0,
layeredPool: true,
}, {
// Targets https://issues.redhat.com/browse/OCPBUGS-24705.
name: "Node has received desiredImage annotation and the MCD has started working",
progress: 1,
nodes: []*corev1.Node{
// Need to set WithNodeReady() on all nodes to avoid short-circuiting.
helpers.NewNodeBuilder("node-0").
WithEqualConfigs(machineConfigV1).
WithDesiredImage(imageV1).
WithNodeReady().
WithMCDState(daemonconsts.MachineConfigDaemonStateWorking).
Node(),
helpers.NewNodeBuilder("node-1").
WithEqualConfigs(machineConfigV1).
WithNodeReady().
WithMCDState(daemonconsts.MachineConfigDaemonStateDone).
Node(),
helpers.NewNodeBuilder("node-2").
WithEqualConfigs(machineConfigV1).
WithNodeReady().
WithMCDState(daemonconsts.MachineConfigDaemonStateDone).
Node(),
},
expected: nil,
otherCandidates: nil,
capacity: 0,
layeredPool: true,
},
}

for _, test := range tests {
test := test
Expand Down
57 changes: 57 additions & 0 deletions pkg/controller/node/status_test.go
View file
Expand Up @@ -509,6 +509,63 @@ func TestGetUnavailableMachines(t *testing.T) {
helpers.NewNodeBuilder("node-4").WithEqualConfigs(machineConfigV0).WithEqualImages(imageV1).WithNodeReady().Node(),
},
unavail: []string{"node-0"},
}, {
// Targets https://issues.redhat.com/browse/OCPBUGS-24705.
name: "nodes working toward layered should not be considered available",
nodes: []*corev1.Node{
// Need to set WithNodeReady() on all nodes to avoid short-circuiting.
helpers.NewNodeBuilder("node-0").
WithEqualConfigs(machineConfigV0).
WithNodeReady().
Node(),
helpers.NewNodeBuilder("node-1").
WithEqualConfigs(machineConfigV0).
WithNodeReady().
Node(),
helpers.NewNodeBuilder("node-2").
WithEqualConfigs(machineConfigV0).
WithDesiredImage(imageV1).
WithMCDState(daemonconsts.MachineConfigDaemonStateWorking).
WithNodeReady().
Node(),
helpers.NewNodeBuilder("node-3").
WithEqualConfigs(machineConfigV0).
WithDesiredImage(imageV1).WithCurrentImage("").
WithNodeReady().
Node(),
},
layeredPoolWithImage: true,
unavail: []string{"node-2", "node-3"},
}, {
// Targets https://issues.redhat.com/browse/OCPBUGS-24705.
name: "nodes with desiredImage annotation that have not yet started working should not be considered available",
nodes: []*corev1.Node{
// Need to set WithNodeReady() on all nodes to avoid short-circuiting.
helpers.NewNodeBuilder("node-0").
WithEqualConfigs(machineConfigV0).
WithMCDState(daemonconsts.MachineConfigDaemonStateDone).
WithNodeReady().
Node(),
helpers.NewNodeBuilder("node-1").
WithEqualConfigs(machineConfigV0).
WithMCDState(daemonconsts.MachineConfigDaemonStateDone).
WithNodeReady().
Node(),
helpers.NewNodeBuilder("node-2").
WithEqualConfigs(machineConfigV0).
WithDesiredImage(imageV1).
WithMCDState(daemonconsts.MachineConfigDaemonStateDone).
WithNodeReady().
Node(),
helpers.NewNodeBuilder("node-3").
WithEqualConfigs(machineConfigV0).
WithDesiredImage(imageV1).WithCurrentImage("").
WithMCDState(daemonconsts.MachineConfigDaemonStateDone).
WithNodeReady().
Node(),
},
layeredPoolWithImage: true,
unavail: []string{"node-2", "node-3"},
},
}

Expand Down

0 comments on commit 523ea84

Please sign in to comment.