Skip to content

NO-ISSUE: Fix Failure Status condition to True when kubeletconfig or container runtime config validation fails #5542

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

Merged
openshift-merge-bot merged 1 commit into from
Jan 22, 2026
Merged

NO-ISSUE: Fix Failure Status condition to True when kubeletconfig or container runtime config validation fails #5542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion pkg/controller/container-runtime-config/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ func wrapErrorWithCondition(err error, args ...interface{}) mcfgv1.ContainerRunt
if err != nil {
condition = apihelpers.NewContainerRuntimeConfigCondition(
mcfgv1.ContainerRuntimeConfigFailure,
corev1.ConditionFalse,
corev1.ConditionTrue,
fmt.Sprintf("Error: %v", err),
Comment on lines 351 to 354
Copy link
Member

@isabella-janssen isabella-janssen Jan 13, 2026

Choose a reason for hiding this comment

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

Is only one condition being set here? If we hit an error case here, does it mean that there could still be a mcfgv1.ContainerRuntimeConfigSuccess condition set to True from a previous update that persists?

Copy link
Contributor Author

@ngopalak-redhat ngopalak-redhat Jan 16, 2026

Choose a reason for hiding this comment

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

I see your point. I have raised https://issues.redhat.com/browse/OCPNODE-4040 to fix it. The scope of this fix is to make sure that the reported "Failure" is set to "False"

The scope of this immediate fix is to ensure that the reported 'Failure' condition is set to 'False'.

A larger fix (potentially as part of OCPNODE-4040 or a separate task) might need to define a new status type, such as KubeletConfigAccepted, rather than relying on generic 'Failure' or 'Success' types. That would likely require an API change (see here: [Link]). Therefore, I am limiting the scope of this current change to just fixing the 'Failure' condition logic.

The larger fix as part of https://issues.redhat.com/browse/OCPNODE-4040 might need to define a new status instead of using "Failure" or "Success". It needs to be more meaningful like "KubeletConfigAccepted" or something similar. May be an API change also https://github.com/openshift/api/blob/master/machineconfiguration/v1/types.go#L801.
Hence scope of this fix is to just fix the condition "Failure".

cc: @haircommander @sairameshv

Copy link
Member

@isabella-janssen isabella-janssen Jan 22, 2026

Choose a reason for hiding this comment

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

Thank you for the context. In this case I feel it is fair to limit the scope of this fix to the changes in your PR. Maybe the conditions can be reconsidered as part of your future work.

)
} else {
Expand Down
64 changes: 64 additions & 0 deletions pkg/controller/container-runtime-config/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
b64 "encoding/base64"
"encoding/json"
"errors"
"fmt"
"os"
"reflect"
"testing"
Expand All @@ -20,6 +21,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"golang.org/x/exp/maps"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/diff"
Expand Down Expand Up @@ -2235,3 +2237,65 @@ func TestImagePolicyConfigFileListDeterministicOrder(t *testing.T) {
require.Equal(t, namespaceJSONs[namespace], result1[i].data, "Data should match expected data of namespace")
}
}

func TestWrapErrorWithCondition(t *testing.T) {
tests := []struct {
name string
err error
args []interface{}
expectedType mcfgv1.ContainerRuntimeConfigStatusConditionType
expectedStatus corev1.ConditionStatus
expectedMessage string
}{
{
name: "error without args produces Failure condition with status True",
err: fmt.Errorf("invalid container runtime configuration"),
args: nil,
expectedType: mcfgv1.ContainerRuntimeConfigFailure,
expectedStatus: corev1.ConditionTrue,
expectedMessage: "Error: invalid container runtime configuration",
},
{
name: "error with formatted args produces Failure condition with status True",
err: fmt.Errorf("validation failed"),
args: []interface{}{"Failed to validate %s: %v", "runtime config", "invalid field"},
expectedType: mcfgv1.ContainerRuntimeConfigFailure,
expectedStatus: corev1.ConditionTrue,
expectedMessage: "Failed to validate runtime config: invalid field",
},
{
name: "nil error produces Success condition with status True",
err: nil,
args: nil,
expectedType: mcfgv1.ContainerRuntimeConfigSuccess,
expectedStatus: corev1.ConditionTrue,
expectedMessage: "Success",
},
{
name: "nil error with args still produces Success condition",
err: nil,
args: []interface{}{"Custom success message"},
expectedType: mcfgv1.ContainerRuntimeConfigSuccess,
expectedStatus: corev1.ConditionTrue,
expectedMessage: "Custom success message",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
condition := wrapErrorWithCondition(tt.err, tt.args...)

if condition.Type != tt.expectedType {
t.Errorf("expected condition type %v, got %v", tt.expectedType, condition.Type)
}

if condition.Status != tt.expectedStatus {
t.Errorf("expected condition status %v, got %v", tt.expectedStatus, condition.Status)
}

if condition.Message != tt.expectedMessage {
t.Errorf("expected message %q, got %q", tt.expectedMessage, condition.Message)
}
})
}
}
2 changes: 1 addition & 1 deletion pkg/controller/kubelet-config/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ func wrapErrorWithCondition(err error, args ...interface{}) mcfgv1.KubeletConfig
if err != nil {
condition = apihelpers.NewKubeletConfigCondition(
mcfgv1.KubeletConfigFailure,
corev1.ConditionFalse,
corev1.ConditionTrue,
fmt.Sprintf("Error: %v", err),
)
} else {
Expand Down
71 changes: 71 additions & 0 deletions pkg/controller/kubelet-config/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package kubeletconfig

import (
"fmt"
"testing"

mcfgv1 "github.com/openshift/api/machineconfiguration/v1"
corev1 "k8s.io/api/core/v1"
)

func TestWrapErrorWithCondition(t *testing.T) {
tests := []struct {
name string
err error
args []interface{}
expectedType mcfgv1.KubeletConfigStatusConditionType
expectedStatus corev1.ConditionStatus
expectedMessage string
}{
{
name: "error without args produces Failure condition with status True",
err: fmt.Errorf("KubeletConfiguration: swapBehavior is not allowed to be set, but contains: LimitedSwap"),
args: nil,
expectedType: mcfgv1.KubeletConfigFailure,
expectedStatus: corev1.ConditionTrue,
expectedMessage: "Error: KubeletConfiguration: swapBehavior is not allowed to be set, but contains: LimitedSwap",
},
{
name: "error with formatted args produces Failure condition with status True",
err: fmt.Errorf("validation failed"),
args: []interface{}{"Failed to validate %s: %v", "kubelet config", "invalid field"},
expectedType: mcfgv1.KubeletConfigFailure,
expectedStatus: corev1.ConditionTrue,
expectedMessage: "Failed to validate kubelet config: invalid field",
},
{
name: "nil error produces Success condition with status True",
err: nil,
args: nil,
expectedType: mcfgv1.KubeletConfigSuccess,
expectedStatus: corev1.ConditionTrue,
expectedMessage: "Success",
},
{
name: "nil error with args still produces Success condition",
err: nil,
args: []interface{}{"Custom success message"},
expectedType: mcfgv1.KubeletConfigSuccess,
expectedStatus: corev1.ConditionTrue,
expectedMessage: "Custom success message",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
condition := wrapErrorWithCondition(tt.err, tt.args...)

if condition.Type != tt.expectedType {
t.Errorf("expected condition type %v, got %v", tt.expectedType, condition.Type)
}

if condition.Status != tt.expectedStatus {
t.Errorf("expected condition status %v, got %v", tt.expectedStatus, condition.Status)
}

if condition.Message != tt.expectedMessage {
t.Errorf("expected message %q, got %q", tt.expectedMessage, condition.Message)
}
})
}
}
2 changes: 1 addition & 1 deletion test/extended-priv/kubeletconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func (kc KubeletConfig) waitUntilSuccess(timeout string) {
func (kc KubeletConfig) waitUntilFailure(expectedMsg, timeout string) {
logger.Infof("wait for %s to report failure", kc.name)
o.EventuallyWithOffset(1, &kc, timeout, "2s").Should(o.SatisfyAll(
HaveConditionField("Failure", "status", "False"),
HaveConditionField("Failure", "status", "True"),
HaveConditionField("Failure", "message", o.ContainSubstring(expectedMsg)),
), "KubeletConfig '%s' should report Failure in status.conditions and report failure message %s. But it doesn't.", kc.GetName(), expectedMsg)
}
Expand Down