Skip to content

Fix #6787: report ErrImagePull in the Integration ready condition - #6848

Open
harshilv17 wants to merge 2 commits into
apache:mainfrom
harshilv17:fix/6787
Open

harshilv17 wants to merge 2 commits into
apache:mainfrom
harshilv17:fix/6787

Conversation

@harshilv17

@harshilv17 harshilv17 commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #6787

The kubelet alternates a container's waiting reason between ErrImagePull (a pull attempt failed) and ImagePullBackOff (waiting before the next attempt). arePodsFailingStatuses only matched ImagePullBackOff. On an ErrImagePull pass, the monitor fell through to updateReadyCondition, which replaced the pull error in the Ready condition with 0/1 updated replicas.

Change: arePodsFailingStatuses matches ErrImagePull alongside ImagePullBackOff. The order of the monitor checks is unchanged, per review.

Known limit: checkReadyCondition still runs first. Once the Deployment reports ProgressDeadlineExceeded (the deployment trait's default progressDeadlineSeconds is 60), the Ready message becomes ReplicaSet "…" has timed out progressing. and the pull error is no longer shown. An earlier revision of this PR checked pending Pods before the controller status to cover that case. It was dropped because changing the check order is riskier.

Test: TestMonitorImagePullFailure covers both reasons. The ErrImagePull case fails on main. go test ./pkg/controller/integration/... passes, and golangci-lint on the package reports 0 issues.

Claude Code on behalf of Harshil Valecha

@harshilv17

Copy link
Copy Markdown
Contributor Author

@squakez pls review...

@squakez squakez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The monitoring order is a bit fragile, so, not sure if inverting the order of checks is a good thing or can introduce regressions. I wonder if we can stick to the same logic and just introduce a "ErrImagePull" condition only to reach the same result.

@github-actions

Copy link
Copy Markdown
Contributor

✔️ Unit test coverage report - coverage increased from 63.8% to 63.9% (+0.1%)

@harshilv17 harshilv17 changed the title Fix #6787: keep the image pull error in the Integration ready condition Fix #6787: report ErrImagePull in the Integration ready condition Sep 24, 2026
@harshilv17

Copy link
Copy Markdown
Contributor Author

@squakez Makes sense. I have reverted the order change, so it now only matches ErrImagePull alongside ImagePullBackOff. That covers the flip to 0/1 updated replicas between pull retries.

@harshilv17

Copy link
Copy Markdown
Contributor Author

Also one thing being that, once the Deployment hits ProgressDeadlineExceeded (60s by default from the deployment trait), checkReadyCondition still replaces the pull error with "timed out progressing".
Your views on this, or would tell if it's worth it...

@squakez

squakez commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

I think we need to review entirely the monitoring after the work deprecated in #6622 - let me open a new issue to track the work, and we can park this PR in the while. Thanks for working on it.

@harshilv17

Copy link
Copy Markdown
Contributor Author

@squakez Yeah, cool!
Will work on the new PR now, let's merge this PR up till then.

@squakez

squakez commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

@squakez Yeah, cool! Will work on the new PR now, let's merge this PR up till then.

No, we better hold on this one before committing some code that could introduce any regression. About #6850 it requires a bit of architecture knowledge and should not be started in 2.12.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A failed integration because missing registry secret does not show the error reason

2 participants