Skip to content

Commit d11fcad

Browse files
author
Evan Anderson
committed
Improve kpack conditions.
Conditions should always have 'reason' filled, per https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties ("This field may not be empty"). Additionally, 'message' should be filled in for Unknown or Error conditions so that users know how to resolve the issue Signed-off-by: Evan Anderson <evana@vmware.com>
1 parent 21404dd commit d11fcad

3 files changed

Lines changed: 72 additions & 18 deletions

File tree

‎pkg/apis/build/v1alpha2/image_lifecycle.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
const (
1313
BuilderNotFound = "BuilderNotFound"
1414
BuilderNotReady = "BuilderNotReady"
15+
BuilderReady = "BuilderReady"
1516
)
1617

1718
func (im *Image) BuilderNotFound() corev1alpha1.Conditions {

‎pkg/reconciler/image/image_test.go‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -796,8 +796,10 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) {
796796
ObservedGeneration: originalGeneration,
797797
Conditions: corev1alpha1.Conditions{
798798
{
799-
Type: corev1alpha1.ConditionReady,
800-
Status: corev1.ConditionUnknown,
799+
Type: corev1alpha1.ConditionReady,
800+
Status: corev1.ConditionFalse,
801+
Reason: buildapi.BuilderNotReady,
802+
Message: "Builder builder-name is not ready: something went wrong",
801803
},
802804
{
803805
Type: buildapi.ConditionBuilderReady,
@@ -2106,6 +2108,7 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) {
21062108
{
21072109
Type: buildapi.ConditionBuilderReady,
21082110
Status: corev1.ConditionTrue,
2111+
Reason: buildapi.BuilderReady,
21092112
},
21102113
},
21112114
},
@@ -2168,10 +2171,12 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) {
21682171
{
21692172
Type: corev1alpha1.ConditionSucceeded,
21702173
Status: corev1.ConditionTrue,
2174+
Reason: image.UpToDateReason,
21712175
},
21722176
{
21732177
Type: buildapi.ConditionBuilderReady,
21742178
Status: corev1.ConditionTrue,
2179+
Reason: buildapi.BuilderReady,
21752180
},
21762181
},
21772182
},
@@ -2210,10 +2215,12 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) {
22102215
{
22112216
Type: corev1alpha1.ConditionReady,
22122217
Status: corev1.ConditionTrue,
2218+
Reason: image.UpToDateReason,
22132219
},
22142220
{
22152221
Type: buildapi.ConditionBuilderReady,
22162222
Status: corev1.ConditionTrue,
2223+
Reason: buildapi.BuilderReady,
22172224
},
22182225
},
22192226
},
@@ -2265,7 +2272,7 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) {
22652272
})
22662273
})
22672274

2268-
it("reports unknown when last build was successful and builder is not ready", func() {
2275+
it("reports false when last build was successful and builder is not ready", func() {
22692276
imageWithBuilder.Status.BuildCounter = 1
22702277
imageWithBuilder.Status.LatestBuildRef = "image-name-build-1"
22712278
imageWithBuilder.Status.LatestImage = "some/image@some-old-sha"
@@ -2291,8 +2298,10 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) {
22912298
ObservedGeneration: originalGeneration,
22922299
Conditions: corev1alpha1.Conditions{
22932300
{
2294-
Type: corev1alpha1.ConditionReady,
2295-
Status: corev1.ConditionUnknown,
2301+
Type: corev1alpha1.ConditionReady,
2302+
Status: corev1.ConditionFalse,
2303+
Reason: buildapi.BuilderNotReady,
2304+
Message: "Builder builder-name is not ready",
22962305
},
22972306
{
22982307
Type: buildapi.ConditionBuilderReady,
@@ -2377,11 +2386,13 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) {
23772386
{
23782387
Type: corev1alpha1.ConditionReady,
23792388
Status: corev1.ConditionFalse,
2389+
Reason: image.BuildFailedReason,
23802390
Message: failureMessage,
23812391
},
23822392
{
23832393
Type: buildapi.ConditionBuilderReady,
23842394
Status: corev1.ConditionTrue,
2395+
Reason: buildapi.BuilderReady,
23852396
},
23862397
},
23872398
},
@@ -2594,11 +2605,13 @@ func conditionReadyUnknown() corev1alpha1.Conditions {
25942605
{
25952606
Type: corev1alpha1.ConditionReady,
25962607
Status: corev1.ConditionUnknown,
2608+
Reason: image.ResolverNotReadyReason,
25972609
Message: "SourceResolver image-name-source is not ready",
25982610
},
25992611
{
26002612
Type: buildapi.ConditionBuilderReady,
26012613
Status: corev1.ConditionTrue,
2614+
Reason: buildapi.BuilderReady,
26022615
},
26032616
}
26042617
}
@@ -2608,11 +2621,13 @@ func conditionBuildExecuting(buildName string) corev1alpha1.Conditions {
26082621
{
26092622
Type: corev1alpha1.ConditionReady,
26102623
Status: corev1.ConditionUnknown,
2624+
Reason: image.BuildRunningReason,
26112625
Message: fmt.Sprintf("%s is executing", buildName),
26122626
},
26132627
{
26142628
Type: buildapi.ConditionBuilderReady,
26152629
Status: corev1.ConditionTrue,
2630+
Reason: buildapi.BuilderReady,
26162631
},
26172632
}
26182633
}
@@ -2622,10 +2637,12 @@ func conditionReady() corev1alpha1.Conditions {
26222637
{
26232638
Type: corev1alpha1.ConditionReady,
26242639
Status: corev1.ConditionTrue,
2640+
Reason: image.UpToDateReason,
26252641
},
26262642
{
26272643
Type: buildapi.ConditionBuilderReady,
26282644
Status: corev1.ConditionTrue,
2645+
Reason: buildapi.BuilderReady,
26292646
},
26302647
}
26312648
}
@@ -2635,10 +2652,12 @@ func conditionNotReady() corev1alpha1.Conditions {
26352652
{
26362653
Type: corev1alpha1.ConditionReady,
26372654
Status: corev1.ConditionFalse,
2655+
Reason: image.BuildFailedReason,
26382656
},
26392657
{
26402658
Type: buildapi.ConditionBuilderReady,
26412659
Status: corev1.ConditionTrue,
2660+
Reason: buildapi.BuilderReady,
26422661
},
26432662
}
26442663
}

‎pkg/reconciler/image/reconcile_build.go‎

Lines changed: 47 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,12 @@ import (
1313
corev1alpha1 "github.com/pivotal/kpack/pkg/apis/core/v1alpha1"
1414
)
1515

16-
const BuildRunningReason = "BuildRunning"
16+
const (
17+
BuildRunningReason = "BuildRunning"
18+
ResolverNotReadyReason = "ResolverNotReady"
19+
BuildFailedReason = "BuildFailed"
20+
UpToDateReason = "UpToDate"
21+
)
1722

1823
func (c *Reconciler) reconcileBuild(ctx context.Context, image *buildapi.Image, latestBuild *buildapi.Build, sourceResolver *buildapi.SourceResolver, builder buildapi.BuilderResource, buildCacheName string) (buildapi.ImageStatus, error) {
1924
currentBuildNumber, err := buildCounter(latestBuild)
@@ -55,7 +60,7 @@ func (c *Reconciler) reconcileBuild(ctx context.Context, image *buildapi.Image,
5560
case corev1.ConditionFalse:
5661
return buildapi.ImageStatus{
5762
Status: corev1alpha1.Status{
58-
Conditions: noScheduledBuild(result.ConditionStatus, builder, latestBuild, sourceResolver),
63+
Conditions: noScheduledBuild(result, builder, latestBuild, sourceResolver),
5964
},
6065
LatestBuildRef: latestBuild.BuildRef(),
6166
LatestBuildReason: latestBuild.BuildReason(),
@@ -70,28 +75,50 @@ func (c *Reconciler) reconcileBuild(ctx context.Context, image *buildapi.Image,
7075
}
7176
}
7277

73-
func noScheduledBuild(buildNeeded corev1.ConditionStatus, builder buildapi.BuilderResource, build *buildapi.Build, sourceResolver *buildapi.SourceResolver) corev1alpha1.Conditions {
74-
if buildNeeded == corev1.ConditionUnknown {
75-
message := ""
78+
func noScheduledBuild(buildNeeded buildRequiredResult, builder buildapi.BuilderResource, build *buildapi.Build, sourceResolver *buildapi.SourceResolver) corev1alpha1.Conditions {
79+
if !builder.Ready() {
80+
return corev1alpha1.Conditions{
81+
{
82+
Type: corev1alpha1.ConditionReady,
83+
Status: corev1.ConditionFalse,
84+
Reason: buildapi.BuilderNotReady,
85+
Message: builderError(builder),
86+
LastTransitionTime: corev1alpha1.VolatileTime{Inner: metav1.Now()},
87+
},
88+
builderCondition(builder),
89+
}
90+
}
91+
if buildNeeded.ConditionStatus == corev1.ConditionUnknown {
92+
message := "Build status unknown"
7693
if !sourceResolver.Ready() {
7794
message = fmt.Sprintf("SourceResolver %s is not ready", sourceResolver.GetName())
7895
}
7996
return corev1alpha1.Conditions{
8097
{
8198
Type: corev1alpha1.ConditionReady,
8299
Status: corev1.ConditionUnknown,
100+
Reason: ResolverNotReadyReason,
83101
Message: message,
84102
LastTransitionTime: corev1alpha1.VolatileTime{Inner: metav1.Now()},
85103
},
86104
builderCondition(builder),
87105
}
88106
}
89107

108+
buildReason := UpToDateReason
109+
buildMessage := "Last build succeeded"
110+
111+
if !build.Status.GetCondition(corev1alpha1.ConditionSucceeded).IsTrue() {
112+
buildReason = BuildFailedReason
113+
buildMessage = "Last build did not succeed"
114+
}
115+
90116
return corev1alpha1.Conditions{
91117
{
92118
Type: corev1alpha1.ConditionReady,
93119
Status: unknownStatusIfNil(build.Status.GetCondition(corev1alpha1.ConditionSucceeded)),
94-
Message: emptyMessageIfNil(build.Status.GetCondition(corev1alpha1.ConditionSucceeded)),
120+
Reason: buildReason,
121+
Message: defaultMessageIfNil(build.Status.GetCondition(corev1alpha1.ConditionSucceeded), buildMessage),
95122
LastTransitionTime: corev1alpha1.VolatileTime{Inner: metav1.Now()},
96123
},
97124
builderCondition(builder),
@@ -106,9 +133,12 @@ func unknownStatusIfNil(condition *corev1alpha1.Condition) corev1.ConditionStatu
106133
return condition.Status
107134
}
108135

109-
func emptyMessageIfNil(condition *corev1alpha1.Condition) string {
136+
// Copies the message from the specified condition, or fills in a default message if nil.
137+
// We should always have a message for non-successful conditions, as that conveys
138+
// information to the user about what is expected.
139+
func defaultMessageIfNil(condition *corev1alpha1.Condition, defaultMessage string) string {
110140
if condition == nil {
111-
return ""
141+
return defaultMessage
112142
}
113143
return condition.Message
114144
}
@@ -126,6 +156,7 @@ func builderCondition(builder buildapi.BuilderResource) corev1alpha1.Condition {
126156
return corev1alpha1.Condition{
127157
Type: buildapi.ConditionBuilderReady,
128158
Status: corev1.ConditionTrue,
159+
Reason: buildapi.BuilderReady,
129160
LastTransitionTime: corev1alpha1.VolatileTime{Inner: metav1.Now()},
130161
}
131162
}
@@ -145,12 +176,14 @@ func scheduledBuildCondition(build *buildapi.Build) corev1alpha1.Conditions {
145176
{
146177
Type: corev1alpha1.ConditionReady,
147178
Status: corev1.ConditionUnknown,
148-
LastTransitionTime: corev1alpha1.VolatileTime{Inner: metav1.Now()},
179+
Reason: BuildRunningReason,
149180
Message: fmt.Sprintf("%s is executing", build.Name),
181+
LastTransitionTime: corev1alpha1.VolatileTime{Inner: metav1.Now()},
150182
},
151183
{
152184
Type: buildapi.ConditionBuilderReady,
153185
Status: corev1.ConditionTrue,
186+
Reason: buildapi.BuilderReady,
154187
LastTransitionTime: corev1alpha1.VolatileTime{Inner: metav1.Now()},
155188
},
156189
}
@@ -168,10 +201,11 @@ func buildCounter(build *buildapi.Build) (int64, error) {
168201
func buildRunningCondition(build *buildapi.Build, builder buildapi.BuilderResource) corev1alpha1.Conditions {
169202
return corev1alpha1.Conditions{
170203
{
171-
Type: corev1alpha1.ConditionReady,
172-
Status: corev1.ConditionUnknown,
173-
Reason: BuildRunningReason,
174-
Message: emptyMessageIfNil(build.Status.GetCondition(corev1alpha1.ConditionSucceeded)),
204+
Type: corev1alpha1.ConditionReady,
205+
Status: corev1.ConditionUnknown,
206+
Reason: BuildRunningReason,
207+
Message: defaultMessageIfNil(build.Status.GetCondition(corev1alpha1.ConditionSucceeded),
208+
fmt.Sprintf("%s is executing", build.Name)),
175209
LastTransitionTime: corev1alpha1.VolatileTime{Inner: metav1.Now()},
176210
},
177211
builderCondition(builder),

0 commit comments

Comments
 (0)