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

Fix problem of Configuration becomes ready prematurely #6096

Merged
merged 3 commits into from
Nov 27, 2019
Merged

Fix problem of Configuration becomes ready prematurely #6096

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a07865
Fix problem of Configuration becomes ready prematurally
Nov 25, 2019
0140354
Revert "Fix problem of Configuration becomes ready prematurally"
Nov 27, 2019
e9ff533
A simplified fix: safeguard setting config to ready with lcr == lrr
Nov 27, 2019
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
17 changes: 13 additions & 4 deletions pkg/apis/serving/v1alpha1/configuration_lifecycle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,18 +79,27 @@ func (cs *ConfigurationStatus) InitializeConditions() {
func (cs *ConfigurationStatus) SetLatestCreatedRevisionName(name string) {
cs.LatestCreatedRevisionName = name
if cs.LatestReadyRevisionName != name {
confCondSet.Manage(cs).MarkUnknown(
ConfigurationConditionReady,
"",
"")
cs.MarkConfigurationUnknown()
}
}

func (cs *ConfigurationStatus) SetLatestReadyRevisionName(name string) {
cs.LatestReadyRevisionName = name
}

// MarkConfigurationReady sets the Ready state of a configuration to True
func (cs *ConfigurationStatus) MarkConfigurationReady() {
taragu marked this conversation as resolved.
Show resolved Hide resolved
confCondSet.Manage(cs).MarkTrue(ConfigurationConditionReady)
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a huge change. Why can't we just guard this in a check that LatestCreated == LatestReady?

}

// MarkConfigurationUnknown sets the Ready state of a configuration to Unknown
func (cs *ConfigurationStatus) MarkConfigurationUnknown() {
taragu marked this conversation as resolved.
Show resolved Hide resolved
confCondSet.Manage(cs).MarkUnknown(
ConfigurationConditionReady,
"",
"")
}

func (cs *ConfigurationStatus) MarkLatestCreatedFailed(name, message string) {
confCondSet.Manage(cs).MarkFalse(
ConfigurationConditionReady,
Expand Down
6 changes: 6 additions & 0 deletions pkg/apis/serving/v1alpha1/configuration_lifecycle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ func TestTypicalFlow(t *testing.T) {
apitestv1.CheckConditionOngoing(r.duck(), ConfigurationConditionReady, t)

r.SetLatestReadyRevisionName("foo")
r.MarkConfigurationReady()
apitestv1.CheckConditionSucceeded(r.duck(), ConfigurationConditionReady, t)

// Verify a second call to SetLatestCreatedRevisionName doesn't change the status from Ready
Expand All @@ -235,6 +236,7 @@ func TestTypicalFlow(t *testing.T) {
apitestv1.CheckConditionOngoing(r.duck(), ConfigurationConditionReady, t)

r.SetLatestReadyRevisionName("bar")
r.MarkConfigurationReady()
apitestv1.CheckConditionSucceeded(r.duck(), ConfigurationConditionReady, t)

r.MarkResourceNotConvertible(ConvertErrorf("build", "something something not allowed.").(*CannotConvertError))
Expand Down Expand Up @@ -272,6 +274,7 @@ func TestFailingFirstRevisionWithRecovery(t *testing.T) {

// When the new revision becomes ready, then Ready becomes true as well.
r.SetLatestReadyRevisionName("bar")
r.MarkConfigurationReady()
apitestv1.CheckConditionSucceeded(r.duck(), ConfigurationConditionReady, t)
}

Expand All @@ -284,6 +287,7 @@ func TestFailingSecondRevision(t *testing.T) {
apitestv1.CheckConditionOngoing(r.duck(), ConfigurationConditionReady, t)

r.SetLatestReadyRevisionName("foo")
r.MarkConfigurationReady()
apitestv1.CheckConditionSucceeded(r.duck(), ConfigurationConditionReady, t)

r.SetLatestCreatedRevisionName("bar")
Expand All @@ -307,6 +311,7 @@ func TestLatestRevisionDeletedThenFixed(t *testing.T) {
apitestv1.CheckConditionOngoing(r.duck(), ConfigurationConditionReady, t)

r.SetLatestReadyRevisionName("foo")
r.MarkConfigurationReady()
apitestv1.CheckConditionSucceeded(r.duck(), ConfigurationConditionReady, t)

// When the latest revision is deleted, the Configuration became Failed.
Expand All @@ -322,6 +327,7 @@ func TestLatestRevisionDeletedThenFixed(t *testing.T) {
apitestv1.CheckConditionOngoing(r.duck(), ConfigurationConditionReady, t)

r.SetLatestReadyRevisionName("bar")
r.MarkConfigurationReady()
apitestv1.CheckConditionSucceeded(r.duck(), ConfigurationConditionReady, t)
}

Expand Down
8 changes: 6 additions & 2 deletions pkg/reconciler/configuration/configuration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,14 +187,14 @@ func (c *Reconciler) reconcile(ctx context.Context, config *v1alpha1.Configurati
return fmt.Errorf("unrecognized condition status: %v on revision %q", rc.Status, revName)
}

if err = c.findAndSetLatestReadyRevision(config); err != nil {
if err = c.findAndSetLatestReadyRevision(config, rc != nil && rc.Status == corev1.ConditionTrue); err != nil {
return fmt.Errorf("failed to find and set latest ready revision: %w", err)
}
return nil
}

// findAndSetLatestReadyRevision finds the last ready revision and sets LatestReadyRevisionName to it.
func (c *Reconciler) findAndSetLatestReadyRevision(config *v1alpha1.Configuration) error {
func (c *Reconciler) findAndSetLatestReadyRevision(config *v1alpha1.Configuration, lcrReady bool) error {
sortedRevisions, err := c.getSortedCreatedRevisions(config)
if err != nil {
return err
Expand All @@ -203,6 +203,10 @@ func (c *Reconciler) findAndSetLatestReadyRevision(config *v1alpha1.Configuratio
if rev.Status.IsReady() {
old, new := config.Status.LatestReadyRevisionName, rev.Name
config.Status.SetLatestReadyRevisionName(rev.Name)
// Only mark config ready if latest created revision is ready
if lcrReady {
config.Status.MarkConfigurationReady()
}
if old != new {
c.Recorder.Eventf(config, corev1.EventTypeNormal, "LatestReadyUpdate",
"LatestReadyRevisionName updated to %q", rev.Name)
Expand Down
45 changes: 41 additions & 4 deletions pkg/reconciler/configuration/configuration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,8 @@ func TestReconcile(t *testing.T) {
// When we see the LatestCreatedRevision become Ready, then we
// update the latest ready revision.
WithLatestReady("matching-revision-done-00001"),
WithLatestCreated("matching-revision-done-00001")),
WithLatestCreated("matching-revision-done-00001"),
WithConfigurationReady()),
}},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "ConfigurationReady", "Configuration becomes ready"),
Expand All @@ -246,7 +247,8 @@ func TestReconcile(t *testing.T) {
Name: "reconcile revision matching generation (ready: true, idempotent)",
Objects: []runtime.Object{
cfg("matching-revision-done-idempotent", "foo", 5566,
WithObservedGen, WithLatestCreated("matching-revision"), WithLatestReady("matching-revision")),
WithObservedGen, WithLatestCreated("matching-revision"), WithLatestReady("matching-revision"),
WithConfigurationReady()),
rev("matching-revision-done-idempotent", "foo", 5566,
WithCreationTimestamp(now), MarkRevisionReady, WithRevName("matching-revision")),
},
Expand Down Expand Up @@ -358,6 +360,7 @@ func TestReconcile(t *testing.T) {
Object: cfg("revision-recovers", "foo", 1337,
WithLatestCreated("revision-recovers-00001"),
WithLatestReady("revision-recovers-00001"),
WithConfigurationReady(),
WithObservedGen,
// When a LatestReadyRevision recovers from failure,
// then we should go back to Ready.
Expand All @@ -374,10 +377,10 @@ func TestReconcile(t *testing.T) {
// when no fix is present
cfg("double-trouble", "foo", 1,
WithLatestCreated("double-trouble-00001"),
WithLatestReady("double-trouble-00001"), WithObservedGen),
WithLatestReady("double-trouble-00001"), WithConfigurationReady(), WithObservedGen),
cfg("first-trouble", "foo", 1,
WithLatestCreated("first-trouble-00001"),
WithLatestReady("first-trouble-00001"), WithObservedGen),
WithLatestReady("first-trouble-00001"), WithConfigurationReady(), WithObservedGen),

rev("first-trouble", "foo", 1,
WithRevName("first-trouble-00001"),
Expand Down Expand Up @@ -410,6 +413,7 @@ func TestReconcile(t *testing.T) {
Object: cfg("threerevs", "foo", 3,
WithLatestCreated("threerevs-00003"),
WithLatestReady("threerevs-00002"),
WithConfigurationUnknown(),
WithObservedGen, func(cfg *v1alpha1.Configuration) {
cfg.Spec.GetTemplate().Name = "threerevs-00003"
},
Expand All @@ -419,6 +423,39 @@ func TestReconcile(t *testing.T) {
Eventf(corev1.EventTypeNormal, "LatestReadyUpdate", "LatestReadyRevisionName updated to %q", "threerevs-00002"),
},
Key: "foo/threerevs",
}, {
Name: "revision not ready, the latest ready should be updated, but the configuration should still be ready==Unknown",
Objects: []runtime.Object{
cfg("revnotready", "foo", 3,
WithLatestCreated("revnotready-00002"),
WithLatestReady("revnotready-00001"), WithObservedGen, func(cfg *v1alpha1.Configuration) {
cfg.Spec.GetTemplate().Name = "revnotready-00003"
},
),
rev("revnotready", "foo", 1,
WithRevName("revnotready-00001"),
WithCreationTimestamp(now), MarkRevisionReady),
rev("revnotready", "foo", 2,
WithRevName("revnotready-00002"),
WithCreationTimestamp(now), MarkRevisionReady),
rev("revnotready", "foo", 3,
WithRevName("revnotready-00003"),
WithCreationTimestamp(now)),
},
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Object: cfg("revnotready", "foo", 3,
WithLatestCreated("revnotready-00003"),
WithLatestReady("revnotready-00002"),
WithConfigurationUnknown(),
WithObservedGen, func(cfg *v1alpha1.Configuration) {
cfg.Spec.GetTemplate().Name = "revnotready-00003"
},
),
}},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "LatestReadyUpdate", "LatestReadyRevisionName updated to %q", "revnotready-00002"),
},
Key: "foo/revnotready",
}}

table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler {
Expand Down
40 changes: 27 additions & 13 deletions pkg/reconciler/service/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,8 @@ func TestReconcile(t *testing.T) {
config("pinned3", "foo", WithReleaseRollout("pinned3-00001"),
WithGeneration(1), WithObservedGen,
WithLatestCreated("pinned3-00001"),
WithLatestReady("pinned3-00001")),
WithLatestReady("pinned3-00001"),
WithConfigurationReady()),
route("pinned3", "foo", WithReleaseRollout("pinned3-00001"),
WithURL, WithAddress, WithInitRouteConditions,
WithStatusTraffic(v1alpha1.TrafficTarget{
Expand Down Expand Up @@ -324,7 +325,8 @@ func TestReconcile(t *testing.T) {
Objects: []runtime.Object{
Service("release-nr", "foo", WithReleaseRollout("release-nr-00002"), WithInitSvcConditions),
config("release-nr", "foo", WithReleaseRollout("release-nr-00002"),
WithCreatedAndReady("release-nr-00002", "release-nr-00002")),
WithCreatedAndReady("release-nr-00002", "release-nr-00002"),
WithConfigurationReady()),
// NB: route points to the previous revision.
route("release-nr", "foo", WithReleaseRollout("release-nr-00002"), RouteReady,
WithURL, WithAddress, WithInitRouteConditions,
Expand Down Expand Up @@ -363,7 +365,8 @@ func TestReconcile(t *testing.T) {
Objects: []runtime.Object{
Service("release-nr", "foo", WithReleaseRollout("release-nr-00002", "release-nr-00003"), WithInitSvcConditions),
config("release-nr", "foo", WithReleaseRollout("release-nr-00002", "release-nr-00003"),
WithCreatedAndReady("release-nr-00003", "release-nr-00003")),
WithCreatedAndReady("release-nr-00003", "release-nr-00003"),
WithConfigurationReady()),
// NB: route points to the previous revision.
route("release-nr", "foo", WithReleaseRollout("release-nr-00002", "release-nr-00003"),
RouteReady, WithURL, WithAddress, WithInitRouteConditions,
Expand Down Expand Up @@ -405,7 +408,8 @@ func TestReconcile(t *testing.T) {
WithInitSvcConditions),
config("release-nr-ts", "foo",
WithReleaseRolloutAndPercentage(42, "release-nr-ts-00002", "release-nr-ts-00003"),
WithCreatedAndReady("release-nr-ts-00003", "release-nr-ts-00003")),
WithCreatedAndReady("release-nr-ts-00003", "release-nr-ts-00003"),
WithConfigurationReady()),
route("release-nr-ts", "foo",
WithReleaseRolloutAndPercentage(42, "release-nr-ts-00002", "release-nr-ts-00003"),
RouteReady, WithURL, WithAddress, WithInitRouteConditions,
Expand Down Expand Up @@ -457,7 +461,8 @@ func TestReconcile(t *testing.T) {
WithInitSvcConditions),
config("release-nr-ts2", "foo",
WithReleaseRolloutAndPercentage(58, "release-nr-ts2-00002", "release-nr-ts2-00003"),
WithCreatedAndReady("release-nr-ts2-00003", "release-nr-ts2-00003")),
WithCreatedAndReady("release-nr-ts2-00003", "release-nr-ts2-00003"),
WithConfigurationReady()),
route("release-nr-ts2", "foo",
WithReleaseRolloutAndPercentage(58, "release-nr-ts2-00002", "release-nr-ts2-00003"),
RouteReady, WithURL, WithAddress, WithInitRouteConditions,
Expand Down Expand Up @@ -526,7 +531,8 @@ func TestReconcile(t *testing.T) {
WithGeneration(1), WithObservedGen,
// These turn a Configuration to Ready=true
WithLatestCreated("release-ready-lr-00001"),
WithLatestReady("release-ready-lr-00001")),
WithLatestReady("release-ready-lr-00001"),
WithConfigurationReady()),
},
Key: "foo/release-ready-lr",
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Expand Down Expand Up @@ -595,7 +601,8 @@ func TestReconcile(t *testing.T) {
WithGeneration(2), WithObservedGen,
// These turn a Configuration to Ready=true
WithLatestCreated("release-ready-lr-00002"),
WithLatestReady("release-ready-lr-00002")),
WithLatestReady("release-ready-lr-00002"),
WithConfigurationReady()),
},
Key: "foo/release-ready-lr",
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Expand Down Expand Up @@ -671,7 +678,8 @@ func TestReconcile(t *testing.T) {
config("release-ready", "foo", WithRunLatestRollout,
WithGeneration(2), WithObservedGen,
// These turn a Configuration to Ready=true
WithLatestCreated("release-ready-00002"), WithLatestReady("release-ready-00002")),
WithLatestCreated("release-ready-00002"), WithLatestReady("release-ready-00002"),
WithConfigurationReady()),
},
Key: "foo/release-ready",
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Expand Down Expand Up @@ -1092,7 +1100,8 @@ func TestReconcile(t *testing.T) {
config("all-ready", "foo", WithRunLatestRollout,
WithGeneration(1), WithObservedGen,
// These turn a Configuration to Ready=true
WithLatestCreated("all-ready-00001"), WithLatestReady("all-ready-00001")),
WithLatestCreated("all-ready-00001"), WithLatestReady("all-ready-00001"),
WithConfigurationReady()),
},
Key: "foo/all-ready",
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Expand Down Expand Up @@ -1137,7 +1146,8 @@ func TestReconcile(t *testing.T) {
config("all-ready", "foo", WithRunLatestRollout,
WithGeneration(1), WithObservedGen, WithGeneration(2),
// These turn a Configuration to Ready=true
WithLatestCreated("all-ready-00001"), WithLatestReady("all-ready-00001")),
WithLatestCreated("all-ready-00001"), WithLatestReady("all-ready-00001"),
WithConfigurationReady()),
},
Key: "foo/all-ready",
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Expand Down Expand Up @@ -1180,7 +1190,8 @@ func TestReconcile(t *testing.T) {
config("config-only-ready", "foo", WithRunLatestRollout,
WithGeneration(2 /*will generate revision -00002*/), WithObservedGen,
// These turn a Configuration to Ready=true
WithLatestCreated("config-only-ready-00002"), WithLatestReady("config-only-ready-00002")),
WithLatestCreated("config-only-ready-00002"), WithLatestReady("config-only-ready-00002"),
WithConfigurationReady()),
},
Key: "foo/config-only-ready",
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Expand Down Expand Up @@ -1221,6 +1232,7 @@ func TestReconcile(t *testing.T) {
}), MarkTrafficAssigned, MarkIngressReady),
config("config-fails", "foo", WithRunLatestRollout, WithGeneration(2),
WithLatestReady("config-fails-00001"), WithLatestCreated("config-fails-00002"),
WithConfigurationReady(),
MarkLatestCreatedFailed("blah"), WithObservedGen),
},
Key: "foo/config-fails",
Expand Down Expand Up @@ -1280,7 +1292,8 @@ func TestReconcile(t *testing.T) {
RouteFailed("Propagate me, please", "")),
config("route-fails", "foo", WithRunLatestRollout, WithGeneration(1), WithObservedGen,
// These turn a Configuration to Ready=true
WithLatestCreated("route-fails-00001"), WithLatestReady("route-fails-00001")),
WithLatestCreated("route-fails-00001"), WithLatestReady("route-fails-00001"),
WithConfigurationReady()),
},
Key: "foo/route-fails",
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Expand Down Expand Up @@ -1352,7 +1365,8 @@ func TestReconcile(t *testing.T) {
}), MarkTrafficAssigned, MarkIngressReady),
config("new-owner", "foo", WithRunLatestRollout, WithGeneration(1), WithObservedGen,
// These turn a Configuration to Ready=true
WithLatestCreated("new-owner-00001"), WithLatestReady("new-owner-00001")),
WithLatestCreated("new-owner-00001"), WithLatestReady("new-owner-00001"),
WithConfigurationReady()),
},
Key: "foo/new-owner",
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Expand Down
14 changes: 14 additions & 0 deletions pkg/testing/v1alpha1/configuration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,20 @@ func WithLatestReady(name string) ConfigOption {
}
}

// WithConfigurationReady sets the ConfigurationConditionReady condition to True
func WithConfigurationReady() ConfigOption {
return func(cfg *v1alpha1.Configuration) {
cfg.Status.MarkConfigurationReady()
}
}

// WithConfigurationUnknown sets the ConfigurationConditionReady condition to Unknown
func WithConfigurationUnknown() ConfigOption {
return func(cfg *v1alpha1.Configuration) {
cfg.Status.MarkConfigurationUnknown()
}
}

// MarkRevisionCreationFailed calls .Status.MarkRevisionCreationFailed.
func MarkRevisionCreationFailed(msg string) ConfigOption {
return func(cfg *v1alpha1.Configuration) {
Expand Down