Skip to content
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

Jump to bottom

Followup on 1221 - further condition improvements #1226

Merged
merged 1 commit into from
Jul 18, 2023
Merged

Followup on 1221 - further condition improvements #1226

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
13 changes: 7 additions & 6 deletions pkg/reconciler/image/image_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2387,7 +2387,7 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) {
Type: corev1alpha1.ConditionReady,
Status: corev1.ConditionFalse,
Reason: image.BuildFailedReason,
Message: failureMessage,
Message: fmt.Sprintf("Build %s failed: %s", failedBuild.Name, failureMessage),
},
{
Type: buildapi.ConditionBuilderReady,
Expand All @@ -2409,7 +2409,7 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) {
it("deletes a failed build if more than the limit", func() {
imageWithBuilder.Spec.FailedBuildHistoryLimit = limit(4)
imageWithBuilder.Status.LatestBuildRef = "image-name-build-5"
imageWithBuilder.Status.Conditions = conditionNotReady()
imageWithBuilder.Status.Conditions = conditionNotReady(imageWithBuilder.Status.LatestBuildRef)
imageWithBuilder.Status.BuildCounter = 5
sourceResolver := resolvedSourceResolver(imageWithBuilder)

Expand Down Expand Up @@ -2647,12 +2647,13 @@ func conditionReady() corev1alpha1.Conditions {
}
}

func conditionNotReady() corev1alpha1.Conditions {
func conditionNotReady(failedBuild string) corev1alpha1.Conditions {
return corev1alpha1.Conditions{
{
Type: corev1alpha1.ConditionReady,
Status: corev1.ConditionFalse,
Reason: image.BuildFailedReason,
Type: corev1alpha1.ConditionReady,
Status: corev1.ConditionFalse,
Reason: image.BuildFailedReason,
Message: fmt.Sprintf("Build %s failed: ", failedBuild),
},
{
Type: buildapi.ConditionBuilderReady,
Expand Down
70 changes: 27 additions & 43 deletions pkg/reconciler/image/reconcile_build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
const (
BuildRunningReason = "BuildRunning"
ResolverNotReadyReason = "ResolverNotReady"
UnknownStateReason = "UnknownState"
BuildFailedReason = "BuildFailed"
UpToDateReason = "UpToDate"
)
Expand Down Expand Up @@ -76,53 +77,36 @@ func (c *Reconciler) reconcileBuild(ctx context.Context, image *buildapi.Image,
}

func noScheduledBuild(buildNeeded corev1.ConditionStatus, builder buildapi.BuilderResource, build *buildapi.Build, sourceResolver *buildapi.SourceResolver) corev1alpha1.Conditions {
if !builder.Ready() {
return corev1alpha1.Conditions{
{
Type: corev1alpha1.ConditionReady,
Status: corev1.ConditionFalse,
Reason: buildapi.BuilderNotReady,
Message: builderError(builder),
LastTransitionTime: corev1alpha1.VolatileTime{Inner: metav1.Now()},
},
builderCondition(builder),
}
}
if buildNeeded == corev1.ConditionUnknown {
message := "Build status unknown"
if !sourceResolver.Ready() {
message = fmt.Sprintf("SourceResolver %s is not ready", sourceResolver.GetName())
}
return corev1alpha1.Conditions{
{
Type: corev1alpha1.ConditionReady,
Status: corev1.ConditionUnknown,
Reason: ResolverNotReadyReason,
Message: message,
LastTransitionTime: corev1alpha1.VolatileTime{Inner: metav1.Now()},
},
builderCondition(builder),
}
ready := corev1alpha1.Condition{
Type: corev1alpha1.ConditionReady,
Status: corev1.ConditionUnknown,
LastTransitionTime: corev1alpha1.VolatileTime{Inner: metav1.Now()},
}

buildReason := UpToDateReason
buildMessage := "Last build succeeded"

if !build.Status.GetCondition(corev1alpha1.ConditionSucceeded).IsTrue() {
buildReason = BuildFailedReason
buildMessage = "Last build did not succeed"
switch {
case !builder.Ready():
ready.Status = corev1.ConditionFalse
ready.Reason = buildapi.BuilderNotReady
ready.Message = builderError(builder)
case buildNeeded == corev1.ConditionUnknown && !sourceResolver.Ready():
ready.Status = corev1.ConditionUnknown
ready.Reason = ResolverNotReadyReason
ready.Message = fmt.Sprintf("SourceResolver %s is not ready", sourceResolver.GetName())
case buildNeeded == corev1.ConditionUnknown && sourceResolver.Ready():
ready.Status = corev1.ConditionUnknown
ready.Reason = UnknownStateReason
ready.Message = "Build status unknown"
Comment on lines +95 to +98
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excitingly, this code isn't covered, as I changed the reason and no tests failed.

(I've been carrying this state around, but I'm not sure what it's supposed to represent...)

Copy link
Contributor

Choose a reason for hiding this comment

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

@matthewmcnew is this related to that libgit2 bug where the source resolver hangs forever? I can't think of any scenarios that would result in status unknown when both builder and source resolver are ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was https://github.com/pivotal/kpack/blob/7857820a20481e34b338c345ee4640ada53bb195/pkg/reconciler/image/reconcile_build.go#L74-L88, which would have had a message of "" and status Unknown with no Reason.

This is now explicit enough that at least it's obvious that we don't know how we got into this state. 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the source, it appears that today the only way to get to this state is when SourceResolver or Builder are not ready. Of course, a future refactor could change that, so I'd like to have some sort of default "not okay" status.

case build.Status.GetCondition(corev1alpha1.ConditionSucceeded).IsTrue():
ready.Status = corev1.ConditionTrue
ready.Reason = UpToDateReason
ready.Message = defaultMessageIfNil(build.Status.GetCondition(corev1alpha1.ConditionSucceeded), "Last build succeeded")
default:
ready.Status = unknownStatusIfNil(build.Status.GetCondition(corev1alpha1.ConditionSucceeded))
ready.Reason = BuildFailedReason
ready.Message = fmt.Sprintf("Build %s failed: %s", build.Name, defaultMessageIfNil(build.Status.GetCondition(corev1alpha1.ConditionSucceeded), "unknown error"))
}

return corev1alpha1.Conditions{
{
Type: corev1alpha1.ConditionReady,
Status: unknownStatusIfNil(build.Status.GetCondition(corev1alpha1.ConditionSucceeded)),
Reason: buildReason,
Message: defaultMessageIfNil(build.Status.GetCondition(corev1alpha1.ConditionSucceeded), buildMessage),
LastTransitionTime: corev1alpha1.VolatileTime{Inner: metav1.Now()},
},
builderCondition(builder),
}
return corev1alpha1.Conditions{ready, builderCondition(builder)}

}

Expand Down