Skip to content

CNF-23402: Add context.Context to Execute and Ops interfaces #6187

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
sebrandon1 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

CNF-23402: Add context.Context to Execute and Ops interfaces #6187

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 .konflux/overlay/release.in.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,6 @@ variables:
manager_version: "lifecycle-agent.v5.0.0"
# min_kube_version should match ocp
# https://access.redhat.com/solutions/4870701
min_kube_version: "1.35.0"
min_kube_version: "1.32.0"
recert_image: PLACEHOLDER_RECERT_IMAGE
version: "5.0.0"
2 changes: 1 addition & 1 deletion controllers/ibu_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ func (r *ImageBasedUpgradeReconciler) Reconcile(ctx context.Context, req ctrl.Re
}

var isAfterPivot bool
isAfterPivot, err = r.RPMOstreeClient.IsStaterootBooted(common.GetDesiredStaterootName(ibu))
isAfterPivot, err = r.RPMOstreeClient.IsStaterootBooted(ctx, common.GetDesiredStaterootName(ibu))
if err != nil {
return
}
Expand Down
2 changes: 1 addition & 1 deletion controllers/ibu_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,7 @@ func TestImageBasedUpgradeReconciler_Reconcile(t *testing.T) {

ctrl := gomock.NewController(t)
mockClient := rpmostreeclient.NewMockIClient(ctrl)
mockClient.EXPECT().IsStaterootBooted("rhcos_").Return(false, nil)
mockClient.EXPECT().IsStaterootBooted(gomock.Any(), "rhcos_").Return(false, nil)

r := &ImageBasedUpgradeReconciler{
Client: fakeClient,
Expand Down
18 changes: 9 additions & 9 deletions controllers/idle_handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ func (r *ImageBasedUpgradeReconciler) cleanupStateroot(ctx context.Context) erro
}

r.Log.Info("Cleaning up unbooted stateroot resources")
if err := CleanupUnbootedStateroots(r.Log, r.Ops, r.OstreeClient, r.RPMOstreeClient); err != nil {
if err := CleanupUnbootedStateroots(ctx, r.Log, r.Ops, r.OstreeClient, r.RPMOstreeClient); err != nil {
return fmt.Errorf("failed to clean up host stateroot resources: %w", err)
}

Expand All @@ -231,8 +231,8 @@ func cleanupIBUFiles() error {
return nil
}

func CleanupUnbootedStateroots(log logr.Logger, ops ops.Ops, ostreeClient ostreeclient.IClient, rpmOstreeClient rpmostreeclient.IClient) error {
status, err := rpmOstreeClient.QueryStatus()
func CleanupUnbootedStateroots(ctx context.Context, log logr.Logger, ops ops.Ops, ostreeClient ostreeclient.IClient, rpmOstreeClient rpmostreeclient.IClient) error {
status, err := rpmOstreeClient.QueryStatus(ctx)
if err != nil {
return fmt.Errorf("failed to query status with rpmostree: %w", err)
}
Expand All @@ -254,14 +254,14 @@ func CleanupUnbootedStateroots(log logr.Logger, ops ops.Ops, ostreeClient ostree
if stateroot == bootedStateroot {
continue
}
if err := cleanupUnbootedStateroot(stateroot, ops, ostreeClient, rpmOstreeClient); err != nil {
if err := cleanupUnbootedStateroot(ctx, stateroot, ops, ostreeClient, rpmOstreeClient); err != nil {
log.Error(err, "failed to remove stateroot", "stateroot", stateroot)
failures += 1
}
}

// clear temporary files and reclaim disk space
if err := rpmOstreeClient.RpmOstreeCleanup(); err != nil {
if err := rpmOstreeClient.RpmOstreeCleanup(ctx); err != nil {
log.Error(err, "failed rpm-ostree cleanup -b")
failures += 1
}
Comment on lines +257 to 267
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 20, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Return immediately on context cancellation during cleanup.

Line 257 and Line 264 currently treat context.Canceled/context.DeadlineExceeded as normal cleanup failures and continue mutating state. That breaks cancellation semantics and can prolong shutdown/reconcile aborts.

Proposed fix 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
@@
 	for _, stateroot := range staterootsToRemove {
@@
 		if err := cleanupUnbootedStateroot(ctx, stateroot, ops, ostreeClient, rpmOstreeClient); err != nil {
+			if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
+				return err
+			}
 			log.Error(err, "failed to remove stateroot", "stateroot", stateroot)
 			failures += 1
 		}
 	}
@@
 	if err := rpmOstreeClient.RpmOstreeCleanup(ctx); err != nil {
+		if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
+			return err
+		}
 		log.Error(err, "failed rpm-ostree cleanup -b")
 		failures += 1
 	}

As per coding guidelines, “Focus on major issues impacting performance, readability, maintainability and security.”

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if err := cleanupUnbootedStateroot(ctx, stateroot, ops, ostreeClient, rpmOstreeClient); err != nil {
log.Error(err, "failed to remove stateroot", "stateroot", stateroot)
failures += 1
}
}
// clear temporary files and reclaim disk space
if err := rpmOstreeClient.RpmOstreeCleanup(); err != nil {
if err := rpmOstreeClient.RpmOstreeCleanup(ctx); err != nil {
log.Error(err, "failed rpm-ostree cleanup -b")
failures += 1
}
if err := cleanupUnbootedStateroot(ctx, stateroot, ops, ostreeClient, rpmOstreeClient); err != nil {
if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
return err
}
log.Error(err, "failed to remove stateroot", "stateroot", stateroot)
failures += 1
}
}
// clear temporary files and reclaim disk space
if err := rpmOstreeClient.RpmOstreeCleanup(ctx); err != nil {
if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
return err
}
log.Error(err, "failed rpm-ostree cleanup -b")
failures += 1
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@controllers/idle_handlers.go` around lines 257 - 267, The cleanup paths treat
context cancellation as routine failures; change both the
cleanupUnbootedStateroot call and the rpmOstreeClient.RpmOstreeCleanup call to
detect context cancellation (use errors.Is(err, context.Canceled) ||
errors.Is(err, context.DeadlineExceeded) or errors.Is with ctx.Err()) and
immediately return that error (or propagate/abort) instead of logging and
incrementing failures; update the logic around cleanupUnbootedStateroot,
rpmOstreeClient.RpmOstreeCleanup, ctx, log and failures so
context-cancellation/timeout results short-circuit the function without mutating
failures or continuing other cleanup steps.

Expand Down Expand Up @@ -291,8 +291,8 @@ func CleanupUnbootedStateroots(log logr.Logger, ops ops.Ops, ostreeClient ostree
return fmt.Errorf("failed to remove %d stateroots", failures)
}

func cleanupUnbootedStateroot(stateroot string, ops ops.Ops, ostreeClient ostreeclient.IClient, rpmOstreeClient rpmostreeclient.IClient) error {
status, err := rpmOstreeClient.QueryStatus()
func cleanupUnbootedStateroot(ctx context.Context, stateroot string, ops ops.Ops, ostreeClient ostreeclient.IClient, rpmOstreeClient rpmostreeclient.IClient) error {
status, err := rpmOstreeClient.QueryStatus(ctx)
if err != nil {
return fmt.Errorf("failed to query status with rpmostree during stateroot cleanup: %w", err)
}
Expand All @@ -310,15 +310,15 @@ func cleanupUnbootedStateroot(stateroot string, ops ops.Ops, ostreeClient ostree
indicesToUndeploy = append(indicesToUndeploy, i)
}
for _, idx := range indicesToUndeploy {
if err := ostreeClient.Undeploy(idx); err != nil {
if err := ostreeClient.Undeploy(ctx, idx); err != nil {
return fmt.Errorf("failed to undeploy %s with index %d: %w", stateroot, idx, err)
}
}
staterootPath := common.GetStaterootPath(stateroot)
if _, err := osStat(common.PathOutsideChroot(staterootPath)); err != nil {
return nil
}
if _, err := ops.RunBashInHostNamespace("unshare", "-m", "/bin/sh", "-c",
if _, err := ops.RunBashInHostNamespace(ctx, "unshare", "-m", "/bin/sh", "-c",
fmt.Sprintf("\"mount -o remount,rw /sysroot && rm -rf %s\"", staterootPath)); err != nil {
return fmt.Errorf("removing stateroot %s failed: %w", stateroot, err)
}
Expand Down
22 changes: 11 additions & 11 deletions controllers/idle_handlers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,13 @@ func TestImageBasedUpgradeReconciler_cleanupUnbootedStateroot(t *testing.T) {
executorMock := ops.NewMockExecute(ctrl)
mockOps := ops.NewMockOps(ctrl)

rpmostreeclientMock.EXPECT().QueryStatus().Return(&rpmostreeclient.Status{
rpmostreeclientMock.EXPECT().QueryStatus(gomock.Any()).Return(&rpmostreeclient.Status{
Deployments: tt.deployments}, nil)
for _, x := range tt.undeployIndices {
ostreeclientMock.EXPECT().Undeploy(x)
ostreeclientMock.EXPECT().Undeploy(gomock.Any(), x)
}
if tt.expectToRemove != "" {
mockOps.EXPECT().RunBashInHostNamespace("unshare", "-m", "/bin/sh", "-c",
mockOps.EXPECT().RunBashInHostNamespace(gomock.Any(), "unshare", "-m", "/bin/sh", "-c",
fmt.Sprintf("\"mount -o remount,rw /sysroot && rm -rf /ostree/deploy/%s\"",
tt.expectToRemove))
}
Expand All @@ -138,8 +138,8 @@ func TestImageBasedUpgradeReconciler_cleanupUnbootedStateroot(t *testing.T) {
return os.Stat(".")
}

if err := cleanupUnbootedStateroot(tt.input, r.Ops, r.OstreeClient, r.RPMOstreeClient); (err != nil) != tt.wantErr {
t.Errorf("ImageBasedUpgradeReconciler.cleanupUnbootedStateroot() error = %v, wantErr %v", err, tt.wantErr)
if err := cleanupUnbootedStateroot(context.Background(), tt.input, r.Ops, r.OstreeClient, r.RPMOstreeClient); (err != nil) != tt.wantErr {
t.Errorf("ImageBasedUpgradeReconciler.cleanupUnbootedStateroot(context.Background(), ) error = %v, wantErr %v", err, tt.wantErr)
}
})
}
Expand Down Expand Up @@ -276,19 +276,19 @@ func TestImageBasedUpgradeReconciler_cleanupUnbootedStateroots(t *testing.T) {
executorMock := ops.NewMockExecute(ctrl)
mockOps := ops.NewMockOps(ctrl)

rpmostreeclientMock.EXPECT().QueryStatus().Return(&rpmostreeclient.Status{
rpmostreeclientMock.EXPECT().QueryStatus(gomock.Any()).Return(&rpmostreeclient.Status{
Deployments: tt.deployments}, nil)
for _, x := range tt.undeployIndices {
ostreeclientMock.EXPECT().Undeploy(x)
ostreeclientMock.EXPECT().Undeploy(gomock.Any(), x)
}
for _, stateroot := range tt.staterootsToRemove {
rpmostreeclientMock.EXPECT().QueryStatus().Return(&rpmostreeclient.Status{
rpmostreeclientMock.EXPECT().QueryStatus(gomock.Any()).Return(&rpmostreeclient.Status{
Deployments: tt.deployments}, nil)
mockOps.EXPECT().RunBashInHostNamespace("unshare", "-m", "/bin/sh", "-c",
mockOps.EXPECT().RunBashInHostNamespace(gomock.Any(), "unshare", "-m", "/bin/sh", "-c",
fmt.Sprintf("\"mount -o remount,rw /sysroot && rm -rf /ostree/deploy/%s\"",
stateroot))
}
rpmostreeclientMock.EXPECT().RpmOstreeCleanup().Return(nil)
rpmostreeclientMock.EXPECT().RpmOstreeCleanup(gomock.Any()).Return(nil)
r := &ImageBasedUpgradeReconciler{
Log: logr.Discard(),
RPMOstreeClient: rpmostreeclientMock,
Expand All @@ -297,7 +297,7 @@ func TestImageBasedUpgradeReconciler_cleanupUnbootedStateroots(t *testing.T) {
Ops: mockOps,
}

if err := CleanupUnbootedStateroots(r.Log, r.Ops, r.OstreeClient, r.RPMOstreeClient); (err != nil) != tt.wantErr {
if err := CleanupUnbootedStateroots(context.Background(), r.Log, r.Ops, r.OstreeClient, r.RPMOstreeClient); (err != nil) != tt.wantErr {
t.Errorf("ImageBasedUpgradeReconciler.cleanupUnbootedStateroots() error = %v, wantErr %v", err, tt.wantErr)
}
})
Expand Down
19 changes: 10 additions & 9 deletions controllers/ipc_config_handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,13 +144,13 @@ func (h *IPCConfigStageHandler) Handle(ctx context.Context, ipc *ipcv1.IPConfig)
return doNotRequeue(), nil
}

targetStaterootBooted, err := isTargetStaterootBooted(ipc, h.RPMOstreeClient)
targetStaterootBooted, err := isTargetStaterootBooted(ctx, ipc, h.RPMOstreeClient)
if err != nil {
logger.Error(err, "Failed to determine whether target stateroot is booted")
return requeueWithError(fmt.Errorf("failed to check if target stateroot is booted: %w", err))
}

isUnbootedStaterootAvailable, err := isUnbootedStaterootAvailable(h.RPMOstreeClient)
isUnbootedStaterootAvailable, err := isUnbootedStaterootAvailable(ctx, h.RPMOstreeClient)
if err != nil {
logger.Error(err, "Failed to determine whether an unbooted stateroot is available")
return requeueWithError(fmt.Errorf("failed to check if unbooted stateroot is available: %w", err))
Expand Down Expand Up @@ -279,7 +279,7 @@ func (h *IPCConfigTwoPhaseHandler) PrePivot(
return requeueWithError(fmt.Errorf("failed to export ipconfig for uncontrolled rollback: %w", err))
}

if err := h.RunLcaCliIPConfigPrePivot(logger); err != nil {
if err := h.RunLcaCliIPConfigPrePivot(ctx, logger); err != nil {
controllerutils.SetIPConfigStatusFailed(
ipc,
fmt.Sprintf("ip-config pre-pivot failed. error: %s", err.Error()),
Expand All @@ -305,7 +305,7 @@ func (h *IPCConfigTwoPhaseHandler) PostPivot(
logger.Info("Starting post-pivot phase")

if ipc.Status.RollbackAvailabilityExpiration.IsZero() {
unbootedStateroot, err := h.RPMOstreeClient.GetUnbootedStaterootName()
unbootedStateroot, err := h.RPMOstreeClient.GetUnbootedStaterootName(ctx)
if err != nil {
return requeueWithError(fmt.Errorf("unable to determine unbooted stateroot path for rollback: %w", err))
}
Expand All @@ -320,7 +320,7 @@ func (h *IPCConfigTwoPhaseHandler) PostPivot(
}
}

if err := h.RebootClient.DisableInitMonitor(); err != nil {
if err := h.RebootClient.DisableInitMonitor(ctx); err != nil {
controllerutils.SetIPConfigStatusFailed(
ipc,
fmt.Sprintf("failed to disable init monitor: %s", err.Error()),
Expand Down Expand Up @@ -799,6 +799,7 @@ func getRecertImage(ipc *ipcv1.IPConfig) string {

// RunLcaCliIPConfigPrePivot schedules an lca-cli ip-config pre-pivot via systemd-run.
func (h *IPCConfigTwoPhaseHandler) RunLcaCliIPConfigPrePivot(
ctx context.Context,
logger logr.Logger,
) error {
logger.Info("Scheduling lca-cli ip-config pre-pivot via systemd-run")
Expand All @@ -812,7 +813,7 @@ func (h *IPCConfigTwoPhaseHandler) RunLcaCliIPConfigPrePivot(
controllerutils.LcaCliBinaryName, "ip-config", "pre-pivot",
}

if _, err := h.ChrootOps.RunSystemdAction(args...); err != nil {
if _, err := h.ChrootOps.RunSystemdAction(ctx, args...); err != nil {
return fmt.Errorf("lca-cli ip-config pre-pivot failed: %w", err)
}

Expand Down Expand Up @@ -1014,8 +1015,8 @@ func (h *IPCConfigStageHandler) validateSNO(ctx context.Context) error {
return nil
}

func (h *IPCConfigStageHandler) validateStaticNetworking() error {
output, err := h.ChrootOps.RunInHostNamespace("nmstatectl", "show", "--json", "-q")
func (h *IPCConfigStageHandler) validateStaticNetworking(ctx context.Context) error {
output, err := h.ChrootOps.RunInHostNamespace(ctx, "nmstatectl", "show", "--json", "-q")
if err != nil {
return fmt.Errorf("failed to run nmstatectl show --json for static networking validation: %w", err)
}
Expand Down Expand Up @@ -1074,7 +1075,7 @@ func (h *IPCConfigStageHandler) validateConfigStart(ctx context.Context, ipc *ip
return fmt.Errorf("validation of SNO failed: %w", err)
}

if err := h.validateStaticNetworking(); err != nil {
if err := h.validateStaticNetworking(ctx); err != nil {
return fmt.Errorf("validation of static networking failed: %w", err)
}

Expand Down
Loading