Skip to content

Fix minscale on deploy #7214

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

Merged
knative-prow-robot merged 4 commits into from
Mar 13, 2020
Merged

Fix minscale on deploy #7214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3ce550
Test minscale stability when configuration is created
tanzeeb Mar 11, 2020
92afa46
Check both .spec.traffic and .status.traffic for resources to label
tanzeeb Mar 11, 2020
8313f82
Inline condition
tanzeeb Mar 12, 2020
df128b4
Inline owner reference check
tanzeeb Mar 12, 2020
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
78 changes: 65 additions & 13 deletions pkg/reconciler/labeler/labeler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func TestReconcile(t *testing.T) {
}, {
Name: "label pinned revision",
Objects: []runtime.Object{
simpleRoute("default", "pinned-revision", "the-revision"),
pinnedRoute("default", "pinned-revision", "the-revision"),
simpleConfig("default", "the-config"),
rev("default", "the-config"),
rev("default", "the-config", WithRevName("the-revision")),
Expand All @@ -103,6 +103,43 @@ func TestReconcile(t *testing.T) {
WithRevisionLabel("serving.knative.dev/route", "steady-state")),
},
Key: "default/steady-state",
}, {
Name: "no ready revision",
Objects: []runtime.Object{
simpleRunLatest("default", "no-ready-revision", "the-config", WithStatusTraffic()),
simpleConfig("default", "the-config", WithLatestReady("")),
rev("default", "the-config"),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchAddFinalizerAction("default", "no-ready-revision"),
patchAddLabel("default", rev("default", "the-config").Name,
"serving.knative.dev/route", "no-ready-revision"),
patchAddLabel("default", "the-config",
"serving.knative.dev/route", "no-ready-revision"),
},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", "no-ready-revision"),
},
Key: "default/no-ready-revision",
}, {
Name: "transitioning route",
Objects: []runtime.Object{
simpleRunLatest("default", "transitioning-route", "old", WithRouteFinalizer,
WithSpecTraffic(configTraffic("new"))),
simpleConfig("default", "old",
WithConfigLabel("serving.knative.dev/route", "transitioning-route")),
rev("default", "old",
WithRevisionLabel("serving.knative.dev/route", "transitioning-route")),
simpleConfig("default", "new"),
rev("default", "new"),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchAddLabel("default", rev("default", "new").Name,
"serving.knative.dev/route", "transitioning-route"),
patchAddLabel("default", "new",
"serving.knative.dev/route", "transitioning-route"),
},
Key: "default/transitioning-route",
}, {
Name: "failure adding label (revision)",
// Induce a failure during patching
Expand Down Expand Up @@ -278,23 +315,38 @@ func TestReconcile(t *testing.T) {
}))
}

func routeWithTraffic(namespace, name string, traffic v1.TrafficTarget, opts ...RouteOption) *v1.Route {
return Route(namespace, name, append(opts, WithStatusTraffic(traffic))...)
func configTraffic(name string) v1.TrafficTarget {
return v1.TrafficTarget{
ConfigurationName: name,
Percent: ptr.Int64(100),
LatestRevision: ptr.Bool(true),
}
}

func simpleRunLatest(namespace, name, config string, opts ...RouteOption) *v1.Route {
return routeWithTraffic(namespace, name, v1.TrafficTarget{
RevisionName: config + "-dbnfd",
func revTraffic(name string, latest bool) v1.TrafficTarget {
return v1.TrafficTarget{
RevisionName: name,
Percent: ptr.Int64(100),
LatestRevision: ptr.Bool(true),
}, opts...)
LatestRevision: ptr.Bool(latest),
}
}

func simpleRoute(namespace, name, revision string, opts ...RouteOption) *v1.Route {
return routeWithTraffic(namespace, name, v1.TrafficTarget{
RevisionName: revision,
Percent: ptr.Int64(100),
}, opts...)
func routeWithTraffic(namespace, name string, spec, status v1.TrafficTarget, opts ...RouteOption) *v1.Route {
return Route(namespace, name,
append([]RouteOption{WithSpecTraffic(spec), WithStatusTraffic(status)}, opts...)...)
}

func simpleRunLatest(namespace, name, config string, opts ...RouteOption) *v1.Route {
return routeWithTraffic(namespace, name,
configTraffic(config),
revTraffic(config+"-dbnfd", true),
opts...)
}

func pinnedRoute(namespace, name, revision string, opts ...RouteOption) *v1.Route {
traffic := revTraffic(revision, false)

return routeWithTraffic(namespace, name, traffic, traffic, opts...)
}

func simpleConfig(namespace, name string, opts ...ConfigOption) *v1.Configuration {
Expand Down
45 changes: 27 additions & 18 deletions pkg/reconciler/labeler/labels.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,28 +45,37 @@ func (c *Reconciler) syncLabels(ctx context.Context, r *v1.Route) error {
revisions := sets.NewString()
configs := sets.NewString()

// Walk the revisions in Route's .status.traffic and build a list
// of Configurations to label from their OwnerReferences.
for _, tt := range r.Status.Traffic {
rev, err := c.revisionLister.Revisions(r.Namespace).Get(tt.RevisionName)
if err != nil {
return err
// Walk the Route's .status.traffic and .spec.traffic and build a list
// of revisions and configurations to label
for _, tt := range append(r.Status.Traffic, r.Spec.Traffic...) {
revName := tt.RevisionName
configName := tt.ConfigurationName

if revName != "" {
rev, err := c.revisionLister.Revisions(r.Namespace).Get(revName)
if err != nil {
return err
}

revisions.Insert(revName)

// If the owner reference is a configuration, treat it like a configuration target
if owner := metav1.GetControllerOf(rev); owner != nil && owner.Kind == "Configuration" {
configName = owner.Name
}
}
revisions.Insert(tt.RevisionName)

// If the owner reference is a configuration, add it to the list of configurations
owner := metav1.GetControllerOf(rev)
if owner != nil && owner.Kind == "Configuration" {
configs.Insert(owner.Name)
if configName != "" {
config, err := c.configurationLister.Configurations(r.Namespace).Get(configName)
if err != nil {
return err
}

configs.Insert(configName)

// If we are tracking the latest revision, add the latest created revision as well
// If the target is for the latest revision, add the latest created revision to the list
// so that there is a smooth transition when the new revision becomes ready.
if tt.LatestRevision != nil && *tt.LatestRevision {
config, err := c.configurationLister.Configurations(r.Namespace).Get(owner.Name)
if err != nil {
return err
}

if config.Status.LatestCreatedRevisionName != "" && tt.LatestRevision != nil && *tt.LatestRevision {
revisions.Insert(config.Status.LatestCreatedRevisionName)
}
}
Expand Down
56 changes: 22 additions & 34 deletions test/e2e/minscale_readiness_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ func TestMinScale(t *testing.T) {
test.CleanupOnInterrupt(func() { test.TearDown(clients, names) })
defer test.TearDown(clients, names)

t.Log("Creating route")
if _, err := v1a1test.CreateRoute(t, clients, names); err != nil {
t.Fatalf("Failed to create Route: %v", err)
}

t.Log("Creating configuration")
cfg, err := v1a1test.CreateConfiguration(t, clients, names, withMinScale(minScale))
if err != nil {
Expand All @@ -63,44 +68,21 @@ func TestMinScale(t *testing.T) {
revName := latestRevisionName(t, clients, names.Config, "")
serviceName := privateServiceName(t, clients, revName)

// Before becoming ready, observe minScale
t.Log("Waiting for revision to scale to minScale before becoming ready")
if err := waitForDesiredScale(clients, serviceName, gte(minScale)); err != nil {
t.Fatalf("The revision %q did not scale >= %d before becoming ready: %v", revName, minScale, err)
}

// Revision becomes ready
t.Log("Waiting for revision to become ready")
if err := v1a1test.WaitForRevisionState(
clients.ServingAlphaClient, revName, v1a1test.IsRevisionReady, "RevisionIsReady",
); err != nil {
t.Fatalf("The Revision %q did not become ready: %v", revName, err)
}

// Without a route, ignore minScale
t.Log("Waiting for revision to scale below minScale after becoming ready")
if err := waitForDesiredScale(clients, serviceName, lt(minScale)); err != nil {
t.Fatalf("The revision %q did not scale < minScale after becoming ready: %v", revName, err)
}

// Create route
t.Log("Creating route")
if _, err := v1a1test.CreateRoute(t, clients, names); err != nil {
t.Fatalf("Failed to create Route: %v", err)
}

// Route becomes ready
t.Log("Waiting for route to become ready")
if err := v1a1test.WaitForRouteState(
clients.ServingAlphaClient, names.Route, v1a1test.IsRouteReady, "RouteIsReady",
); err != nil {
t.Fatalf("The Route %q is not ready: %v", names.Route, err)
}

// With a route, observe minScale
t.Log("Waiting for revision to scale to minScale after creating route")
if err := waitForDesiredScale(clients, serviceName, gte(minScale)); err != nil {
t.Fatalf("The revision %q did not scale >= %d after creating route: %v", revName, minScale, err)
t.Log("Holding revision at minScale after becoming ready")
if !ensureDesiredScale(clients, serviceName, gte(minScale)) {
t.Fatalf("The revision %q did not stay at scale >= %d after becoming ready", revName, minScale)
}

t.Log("Updating configuration")
Expand All @@ -111,31 +93,37 @@ func TestMinScale(t *testing.T) {
newRevName := latestRevisionName(t, clients, names.Config, revName)
newServiceName := privateServiceName(t, clients, newRevName)

// After update, observe minScale in new revision
t.Log("Waiting for latest revision to scale to minScale after update")
t.Log("Waiting for new revision to scale to minScale after update")
if err := waitForDesiredScale(clients, newServiceName, gte(minScale)); err != nil {
t.Fatalf("The revision %q did not scale >= %d after creating route: %v", newRevName, minScale, err)
}

// Revision becomes ready
t.Log("Waiting for revision to become ready")
t.Log("Waiting for new revision to become ready")
if err := v1a1test.WaitForRevisionState(
clients.ServingAlphaClient, newRevName, v1a1test.IsRevisionReady, "RevisionIsReady",
); err != nil {
t.Fatalf("The Revision %q did not become ready: %v", newRevName, err)
}

// After update, ensure new revision holds minScale
t.Log("Hold minScale after update")
t.Log("Holding new revision at minScale after becoming ready")
if !ensureDesiredScale(clients, newServiceName, gte(minScale)) {
t.Fatalf("The revision %q did not stay at scale >= %d after creating route", newRevName, minScale)
t.Fatalf("The new revision %q did not stay at scale >= %d after becoming ready", newRevName, minScale)
}

// After update, ensure old revision ignores minScale
t.Log("Waiting for old revision to scale below minScale after being replaced")
if err := waitForDesiredScale(clients, serviceName, lt(minScale)); err != nil {
t.Fatalf("The revision %q did not scale < minScale after being replaced: %v", revName, err)
}

t.Log("Deleting route")
if err := clients.ServingAlphaClient.Routes.Delete(names.Route, &metav1.DeleteOptions{}); err != nil {
t.Fatalf("Failed to delete route %q: %v", names.Route, err)
}

t.Log("Waiting for new revision to scale below minScale when there is no route")
if err := waitForDesiredScale(clients, newServiceName, lt(minScale)); err != nil {
t.Fatalf("The revision %q did not scale < minScale after being replaced: %v", newRevName, err)
}
}

func gte(m int) func(int) bool {
Expand Down