Skip to content

AGENT-1366: Report InternalReleaseImageController errors in IRI status #5803

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
bfournie wants to merge 1 commit into
base: main
Choose a base branch
from
+141 −8
Open

AGENT-1366: Report InternalReleaseImageController errors in IRI status #5803

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
76 changes: 68 additions & 8 deletions pkg/controller/internalreleaseimage/internalreleaseimage_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apimachinery/pkg/util/wait"
clientset "k8s.io/client-go/kubernetes"
Expand Down Expand Up @@ -298,7 +299,7 @@ func (ctrl *Controller) enqueue(iri *mcfgv1alpha1.InternalReleaseImage) {
// syncInternalReleaseImage will sync the InternalReleaseImage with the given key.
// This function is not meant to be invoked concurrently with the same key.
// nolint: gocyclo
func (ctrl *Controller) syncInternalReleaseImage(key string) error {
func (ctrl *Controller) syncInternalReleaseImage(key string) (syncErr error) {
startTime := time.Now()
klog.V(4).Infof("Started syncing InternalReleaseImage %q (%v)", key, startTime)
defer func() {
Expand Down Expand Up @@ -331,9 +332,22 @@ func (ctrl *Controller) syncInternalReleaseImage(key string) error {
return nil
}

// Update status condition on function exit based on sync result
defer func() {
if statusErr := ctrl.updateInternalReleaseImageStatus(iri, syncErr); statusErr != nil {
if syncErr != nil {
// Already have a sync error, just log the status update failure
klog.Warningf("Error updating InternalReleaseImage status: %v", statusErr)
} else {
// Sync succeeded but status update failed, propagate the error
syncErr = fmt.Errorf("failed to update InternalReleaseImage status: %w", statusErr)
}
}
}()

cconfig, err := ctrl.ccLister.Get(ctrlcommon.ControllerConfigName)
if err != nil {
return fmt.Errorf("could not get ControllerConfig %w", err)
return fmt.Errorf("could not get ControllerConfig: %w", err)
}

iriSecret, err := ctrl.secretLister.Secrets(ctrlcommon.MCONamespace).Get(ctrlcommon.InternalReleaseImageTLSSecretName)
Expand All @@ -347,31 +361,31 @@ func (ctrl *Controller) syncInternalReleaseImage(key string) error {
mc, err := ctrl.mcLister.Get(r.GetMachineConfigName())
isNotFound := errors.IsNotFound(err)
if err != nil && !isNotFound {
return err // syncStatus, could not find MachineConfig
return fmt.Errorf("could not get MachineConfig: %w", err)
}
if isNotFound {
mc, err = r.CreateEmptyMachineConfig()
if err != nil {
return err // syncStatusOnly, could not create MachineConfig
return fmt.Errorf("could not create MachineConfig: %w", err)
}
}

err = r.RenderAndSetIgnition(mc)
if err != nil {
return err // syncStatus, could not generate IRI configs
return fmt.Errorf("could not generate IRI configs: %w", err)
}
err = ctrl.createOrUpdateMachineConfig(isNotFound, mc)
if err != nil {
return err // syncStatus, could not Create/Update MachineConfig
return fmt.Errorf("could not create/update MachineConfig: %w", err)
}
if err := ctrl.addFinalizerToInternalReleaseImage(iri, mc); err != nil {
return err // syncStatus , could not add finalizers
return fmt.Errorf("could not add finalizer: %w", err)
}
}

// Initialize status if empty
if err := ctrl.initializeInternalReleaseImageStatus(iri); err != nil {
return err
return fmt.Errorf("could not initialize status: %w", err)
}

return nil
Expand Down Expand Up @@ -431,6 +445,52 @@ func (ctrl *Controller) initializeInternalReleaseImageStatus(iri *mcfgv1alpha1.I
return nil
}

// updateInternalReleaseImageStatus updates the InternalReleaseImage status conditions
// based on the provided error. If err is nil, it sets Degraded=False, otherwise Degraded=True.
func (ctrl *Controller) updateInternalReleaseImageStatus(iri *mcfgv1alpha1.InternalReleaseImage, err error) error {
return retry.RetryOnConflict(updateBackoff, func() error {
// Get the latest version of the IRI directly from the API server to avoid conflicts
latestIRI, getErr := ctrl.client.MachineconfigurationV1alpha1().InternalReleaseImages().Get(context.TODO(), iri.Name, metav1.GetOptions{})
if getErr != nil {
return getErr
}
newIRI := latestIRI.DeepCopy()

// Prepare the condition based on error state
var condition metav1.Condition
if err != nil {
// Set Degraded=True when there's an error
condition = metav1.Condition{
Type: string(mcfgv1alpha1.InternalReleaseImageStatusConditionTypeDegraded),
Status: metav1.ConditionTrue,
Reason: "SyncError",
Message: fmt.Sprintf("Error syncing InternalReleaseImage: %v", err),
ObservedGeneration: newIRI.Generation,
}
} else {
// Set Degraded=False when sync is successful
condition = metav1.Condition{
Type: string(mcfgv1alpha1.InternalReleaseImageStatusConditionTypeDegraded),
Status: metav1.ConditionFalse,
Reason: "AsExpected",
Message: "InternalReleaseImage controller sync successful",
ObservedGeneration: newIRI.Generation,
}
}

// Update the condition and check if it actually changed
changed := meta.SetStatusCondition(&newIRI.Status.Conditions, condition)
if !changed {
// No changes needed, skip the API call
return nil
}

// Update the status subresource only if the condition changed
_, updateErr := ctrl.client.MachineconfigurationV1alpha1().InternalReleaseImages().UpdateStatus(context.TODO(), newIRI, metav1.UpdateOptions{})
return updateErr
})
}

func (ctrl *Controller) createOrUpdateMachineConfig(isNotFound bool, mc *mcfgv1.MachineConfig) error {
return retry.RetryOnConflict(updateBackoff, func() error {
var err error
Expand Down
73 changes: 73 additions & 0 deletions pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,21 @@ func TestInternalReleaseImageCreate(t *testing.T) {
assert.Nil(t, actualWorkerMC)
},
},
{
name: "status condition Degraded=False on successful sync",
initialObjects: objs(
iri().finalizer(masterName(), workerName()),
clusterVersion(), cconfig(), iriCertSecret(),
machineconfigmaster(), machineconfigworker()),
verify: func(t *testing.T, actualIRI *mcfgv1alpha1.InternalReleaseImage, actualMasterMC *mcfgv1.MachineConfig, actualWorkerMC *mcfgv1.MachineConfig) {
assert.NotNil(t, actualIRI)
assert.Len(t, actualIRI.Status.Conditions, 1)
assert.Equal(t, string(mcfgv1alpha1.InternalReleaseImageStatusConditionTypeDegraded), actualIRI.Status.Conditions[0].Type)
assert.Equal(t, metav1.ConditionFalse, actualIRI.Status.Conditions[0].Status)
assert.Equal(t, "AsExpected", actualIRI.Status.Conditions[0].Reason)
assert.Equal(t, "InternalReleaseImage controller sync successful", actualIRI.Status.Conditions[0].Message)
},
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
Expand Down Expand Up @@ -160,6 +175,64 @@ func TestInternalReleaseImageCreate(t *testing.T) {
}
}

func TestInternalReleaseImageStatusOnError(t *testing.T) {
cases := []struct {
name string
initialObjects func() []runtime.Object
verify func(t *testing.T, actualIRI *mcfgv1alpha1.InternalReleaseImage)
}{
{
name: "status condition Degraded=True when ControllerConfig is missing",
initialObjects: objs(
iri(),
clusterVersion(), iriCertSecret()),
verify: func(t *testing.T, actualIRI *mcfgv1alpha1.InternalReleaseImage) {
assert.NotNil(t, actualIRI)
assert.Len(t, actualIRI.Status.Conditions, 1)
assert.Equal(t, string(mcfgv1alpha1.InternalReleaseImageStatusConditionTypeDegraded), actualIRI.Status.Conditions[0].Type)
assert.Equal(t, metav1.ConditionTrue, actualIRI.Status.Conditions[0].Status)
assert.Equal(t, "SyncError", actualIRI.Status.Conditions[0].Reason)
assert.Contains(t, actualIRI.Status.Conditions[0].Message, "could not get ControllerConfig")
},
},
{
name: "status condition Degraded=True when Secret is missing",
initialObjects: objs(
iri(),
clusterVersion(), cconfig()),
verify: func(t *testing.T, actualIRI *mcfgv1alpha1.InternalReleaseImage) {
assert.NotNil(t, actualIRI)
assert.Len(t, actualIRI.Status.Conditions, 1)
assert.Equal(t, string(mcfgv1alpha1.InternalReleaseImageStatusConditionTypeDegraded), actualIRI.Status.Conditions[0].Type)
assert.Equal(t, metav1.ConditionTrue, actualIRI.Status.Conditions[0].Status)
assert.Equal(t, "SyncError", actualIRI.Status.Conditions[0].Reason)
assert.Contains(t, actualIRI.Status.Conditions[0].Message, "could not get Secret")
},
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
objs := tc.initialObjects()
f := newFixture(t, objs)
// Run the controller and expect an error
f.runController(ctrlcommon.InternalReleaseImageInstanceName, true)

if tc.verify != nil {
actualIRI, err := f.client.MachineconfigurationV1alpha1().InternalReleaseImages().Get(context.TODO(), ctrlcommon.InternalReleaseImageInstanceName, v1.GetOptions{})
if err != nil {
if !errors.IsNotFound(err) {
t.Errorf("Error getting IRI: %v", err)
} else {
actualIRI = nil
}
}
tc.verify(t, actualIRI)
}
})
}
}

// The fixture used to setup and run the controller.
type fixture struct {
t *testing.T
Expand Down