Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
11 changes: 3 additions & 8 deletions pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const hasBeenActiveAnnotation = "HasBeenActive"

Comment thread
markusthoemmes marked this conversation as resolved.
Outdated
var podCondSet = apis.NewLivingConditionSet(
PodAutoscalerConditionActive,
PodAutoscalerConditionHasBeenActive,
)

// GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface.
Expand Down Expand Up @@ -176,18 +177,12 @@ func (pas *PodAutoscalerStatus) IsInactive() bool {

// HasBeenActive returns true if the pod autoscaler has reached its initial scale.
func (pas *PodAutoscalerStatus) HasBeenActive() bool {
if val, ok := pas.Annotations[hasBeenActiveAnnotation]; !ok || val != "true" {
return false
}
return true
return pas.GetCondition(PodAutoscalerConditionHasBeenActive).IsTrue()
}

// MarkHasBeenActive marks the PA's PodAutoscalerConditionInitiallyActive condition true.
func (pas *PodAutoscalerStatus) MarkHasBeenActive() {
if pas.Annotations == nil {
pas.Annotations = map[string]string{}
}
pas.Annotations[hasBeenActiveAnnotation] = "true"
podCondSet.Manage(pas).MarkTrue(PodAutoscalerConditionHasBeenActive)
}

// GetCondition gets the condition `t`.
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -980,6 +980,7 @@ func TestTypicalFlow(t *testing.T) {

// When we see traffic, mark ourselves active.
r.MarkActive()
r.MarkHasBeenActive()
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionActive, t)
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionReady, t)

Expand Down
3 changes: 3 additions & 0 deletions pkg/apis/autoscaling/v1alpha1/pa_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ const (
// PodAutoscalerConditionReady is set when the revision is starting to materialize
// runtime resources, and becomes true when those resources are ready.
PodAutoscalerConditionReady = apis.ConditionReady
// PodAutoscalerConditionHasBeenActive is set when the PodAutoscaler's ScaleTargetRef was ready to
// serve traffic once.
PodAutoscalerConditionHasBeenActive apis.ConditionType = "HasBeenActive"

@dprotaso dprotaso Jul 7, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

HasBeenActive sounds a bit odd to me - how about Deployed

@dprotaso dprotaso Jul 7, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ScaleTargetDeployed (maybe just TargetDeployed) might be more specific

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I almost feel like we should just set Ready 😂

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.

Not necessarily true. With initialScale=0 — nothing will ever be deployed

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.

Hmm ScaleTargetDeployed might imply new calculated autoscaler scale targets. What about InitialTargetDeployed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TargetInitialized ?

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.

Or maybe ReachedInitialScale, since that’s what actually gates the condition

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ScaleTargetInitialized would be my favorite mix from the above. I would like to keep InitialScale out of this, it doesn't necessarily gate this, for example if minScale is > initialScale.

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.

Hm, in my head when minScale > initialScale, initialScale is implicitly set to minScale, but I see how that could be confusing.

ScaleTargetInitialized seems alright, just a bit odd that it talks about a "Scale Target" concept that isn't really defined anywhere else, and doesn't mention the quite important fact that it's the scale target for the first scale we're talking about. I'm also not totally sure what "Initialized" means in this context.. "-Reached" seems clearer about what has to happen. Not the end of the world, though, if everyone else is happy Im happy.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it works because it's part of the PodAutoscaler spec

// ScaleTargetRef defines the /scale-able resource that this PodAutoscaler
// is responsible for quickly right-sizing.
ScaleTargetRef corev1.ObjectReference `json:"scaleTargetRef"`

// PodAutoscalerConditionActive is set when the PodAutoscaler's ScaleTargetRef is receiving traffic.
PodAutoscalerConditionActive apis.ConditionType = "Active"
)
Expand Down
1 change: 1 addition & 0 deletions pkg/reconciler/autoscaling/hpa/hpa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,7 @@ func pa(namespace, name string, options ...PodAutoscalerOption) *asv1a1.PodAutos
ProtocolType: networking.ProtocolHTTP1,
},
}
pa.Status.InitializeConditions()
for _, opt := range options {
opt(pa)
}
Expand Down
1 change: 1 addition & 0 deletions pkg/reconciler/autoscaling/kpa/kpa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ func kpa(ns, n string, opts ...PodAutoscalerOption) *asv1a1.PodAutoscaler {
kpa.Generation = 1
kpa.Annotations["autoscaling.knative.dev/class"] = "kpa.autoscaling.knative.dev"
kpa.Annotations["autoscaling.knative.dev/metric"] = "concurrency"
kpa.Status.InitializeConditions()
for _, opt := range opts {
opt(kpa)
}
Expand Down
1 change: 1 addition & 0 deletions pkg/reconciler/revision/revision_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ func testReadyPA(rev *v1.Revision) *av1alpha1.PodAutoscaler {
pa := resources.MakePA(rev)
pa.Status.InitializeConditions()
pa.Status.MarkActive()
pa.Status.MarkHasBeenActive()
pa.Status.ServiceName = serviceName(rev.Name)
return pa
}
Expand Down
8 changes: 4 additions & 4 deletions pkg/reconciler/revision/table_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ func TestReconcile(t *testing.T) {
Objects: []runtime.Object{
rev("foo", "pa-ready",
withK8sServiceName("old-stuff"), WithLogURL, AllUnknownConditions),
pa("foo", "pa-ready", WithTraffic, WithPAStatusService("new-stuff"), WithReachabilityUnknown),
pa("foo", "pa-ready", WithTraffic, WithHasBeenActive, WithPAStatusService("new-stuff"), WithReachabilityUnknown),
deploy(t, "foo", "pa-ready"),
image("foo", "pa-ready"),
},
Expand Down Expand Up @@ -343,7 +343,7 @@ func TestReconcile(t *testing.T) {
withK8sServiceName("ill-follow-the-sun"), WithLogURL, MarkRevisionReady,
WithRevisionLabel(serving.RouteLabelKey, "foo")),
pa("foo", "fix-mutated-pa", WithProtocolType(networking.ProtocolH2C),
WithTraffic, WithPAStatusService("fix-mutated-pa")),
WithTraffic, WithHasBeenActive, WithPAStatusService("fix-mutated-pa")),
deploy(t, "foo", "fix-mutated-pa"),
image("foo", "fix-mutated-pa"),
},
Expand All @@ -357,7 +357,7 @@ func TestReconcile(t *testing.T) {
withDefaultContainerStatuses(), withObservedGeneration(1)),
}},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: pa("foo", "fix-mutated-pa", WithTraffic,
Object: pa("foo", "fix-mutated-pa", WithTraffic, WithHasBeenActive,
WithPAStatusService("fix-mutated-pa"), WithReachabilityReachable),
}},
WantEvents: []string{
Expand Down Expand Up @@ -511,7 +511,7 @@ func TestReconcile(t *testing.T) {
// This signal should make our Reconcile mark the Revision as Ready.
Objects: []runtime.Object{
rev("foo", "steady-ready", withK8sServiceName("very-steady"), WithLogURL),
pa("foo", "steady-ready", WithTraffic, WithPAStatusService("steadier-even")),
pa("foo", "steady-ready", WithTraffic, WithHasBeenActive, WithPAStatusService("steadier-even")),
deploy(t, "foo", "steady-ready"),
image("foo", "steady-ready"),
},
Expand Down
5 changes: 5 additions & 0 deletions pkg/testing/functional.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ func WithTraffic(pa *asv1a1.PodAutoscaler) {
pa.Status.MarkActive()
}

// WithHasBeenActive updates the PA to reflect it having been active.
func WithHasBeenActive(pa *asv1a1.PodAutoscaler) {
pa.Status.MarkHasBeenActive()
}

// WithPAStatusService annotates PA Status with the provided service name.
func WithPAStatusService(svc string) PodAutoscalerOption {
return func(pa *asv1a1.PodAutoscaler) {
Expand Down