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

Add new KPA status for dependencies #8809

Merged
merged 9 commits into from
Jul 26, 2020
Merged

Add new KPA status for dependencies #8809

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2692df5
Add new Kpa status for dependencies
vagababov Jul 24, 2020
ad7fba5
t Merge branch 'master' into 20200724-kpa-status
vagababov Jul 25, 2020
7b79765
more files to fix
vagababov Jul 25, 2020
a8ba223
review
vagababov Jul 25, 2020
22d40be
restore tests
vagababov Jul 25, 2020
983c811
Merge branch 'master' into 20200724-kpa-status
vagababov Jul 26, 2020
9a2048c
rewrite as sks only cond
vagababov Jul 26, 2020
3dbfdd2
holdout
vagababov Jul 26, 2020
4b1ca7d
more fixes
vagababov Jul 26, 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
15 changes: 14 additions & 1 deletion pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
var podCondSet = apis.NewLivingConditionSet(
PodAutoscalerConditionActive,
PodAutoscalerConditionScaleTargetInitialized,
PodAutoscalerConditionDependenciesReady,
)

// GetConditionSet retrieves the condition set for this resource. Implements the KRShaped interface.
Expand Down Expand Up @@ -185,6 +186,18 @@ func (pas *PodAutoscalerStatus) MarkScaleTargetInitialized() {
podCondSet.Manage(pas).MarkTrue(PodAutoscalerConditionScaleTargetInitialized)
}

// MarkDependenciesReadymarks the PA condition denoting that all its
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// MarkDependenciesReadymarks the PA condition denoting that all its
// MarkDependenciesReady marks the PA condition denoting that all its

// dependencies are ready.
func (pas *PodAutoscalerStatus) MarkDependenciesReady() {
podCondSet.Manage(pas).MarkTrue(PodAutoscalerConditionDependenciesReady)
}

// MarkDependenciesNotReady marks the PA condation that at least one of the
// dependendcies is not ready.
func (pas *PodAutoscalerStatus) MarkDependenciesNotReady(reason, message string) {
Copy link
Member

Choose a reason for hiding this comment

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

I expected NotReady to be False, but perhaps it is sort of ambiguous. 🤷

podCondSet.Manage(pas).MarkUnknown(PodAutoscalerConditionDependenciesReady, reason, message)
}

// GetCondition gets the condition `t`.
func (pas *PodAutoscalerStatus) GetCondition(t apis.ConditionType) *apis.Condition {
return podCondSet.Manage(pas).GetCondition(t)
Expand Down Expand Up @@ -248,7 +261,7 @@ func (pas *PodAutoscalerStatus) CanFailActivation(now time.Time, idlePeriod time

// inStatusFor returns positive duration if the PodAutoscalerStatus's Active condition has stayed in
// the specified status for at least the specified duration. Otherwise it returns negative duration,
// including when the status is undetermined (Active condition is not found.)
// including when the status is undetermined.
func (pas *PodAutoscalerStatus) inStatusFor(status corev1.ConditionStatus, now time.Time, dur time.Duration) time.Duration {
cond := pas.GetCondition(PodAutoscalerConditionActive)
if cond == nil || cond.Status != status {
Expand Down
24 changes: 23 additions & 1 deletion pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ func TestInactiveFor(t *testing.T) {
})
}
}

func TestActiveFor(t *testing.T) {
now := time.Now()
cases := []struct {
Expand Down Expand Up @@ -977,21 +978,41 @@ func TestTypicalFlow(t *testing.T) {
r.InitializeConditions()
apistest.CheckConditionOngoing(r, PodAutoscalerConditionActive, t)
apistest.CheckConditionOngoing(r, PodAutoscalerConditionReady, t)
apistest.CheckConditionOngoing(r, PodAutoscalerConditionDependenciesReady, t)
apistest.CheckConditionOngoing(r, PodAutoscalerConditionScaleTargetInitialized, t)

r.MarkActive()
r.MarkDependenciesReady()
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionDependenciesReady, t)
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionActive, t)
apistest.CheckConditionOngoing(r, PodAutoscalerConditionReady, t)

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

// Check idempotency.
r.MarkActive()
r.MarkScaleTargetInitialized()
r.MarkDependenciesReady()
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionScaleTargetInitialized, t)
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionActive, t)
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionDependenciesReady, t)
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionReady, t)

r.MarkDependenciesNotReady("omg", "not ready")
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionScaleTargetInitialized, t)
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionActive, t)
apistest.CheckConditionOngoing(r, PodAutoscalerConditionDependenciesReady, t)
apistest.CheckConditionOngoing(r, PodAutoscalerConditionReady, t)

// Restore.
r.MarkDependenciesReady()

// When we stop seeing traffic, mark ourselves inactive.
r.MarkInactive("TheReason", "the message")
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionScaleTargetInitialized, t)
Expand All @@ -1014,9 +1035,10 @@ func TestTypicalFlow(t *testing.T) {
if !r.IsActive() {
t.Error("Active was not set.")
}
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionScaleTargetInitialized, t)
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionActive, t)
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionDependenciesReady, t)
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionReady, t)
apistest.CheckConditionSucceeded(r, PodAutoscalerConditionScaleTargetInitialized, t)
}

func TestTargetBC(t *testing.T) {
Expand Down
3 changes: 3 additions & 0 deletions pkg/apis/autoscaling/v1alpha1/pa_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,9 @@ const (
PodAutoscalerConditionScaleTargetInitialized apis.ConditionType = "ScaleTargetInitialized"
// PodAutoscalerConditionActive is set when the PodAutoscaler's ScaleTargetRef is receiving traffic.
PodAutoscalerConditionActive apis.ConditionType = "Active"
// PodAutoscalerCondtionDependenciesReady is set when all the dependencies have are ready and
// the revision is ready to scale.
PodAutoscalerConditionDependenciesReady = "DependeciesReady"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
PodAutoscalerConditionDependenciesReady = "DependeciesReady"
PodAutoscalerConditionDependenciesReady = "DependenciesReady"

)

// PodAutoscalerStatus communicates the observed state of the PodAutoscaler (from the controller).
Expand Down
8 changes: 7 additions & 1 deletion pkg/reconciler/autoscaling/kpa/kpa.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,13 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pa *pav1alpha1.PodAutosc
}

// Having an SKS and its PrivateServiceName is a prerequisite for all upcoming steps.
if sks == nil || (sks != nil && sks.Status.PrivateServiceName == "") {
if sks == nil || sks.Status.PrivateServiceName == "" {
// Before we can reconcile decider and get real number of activators
// we start with default of 2.
if _, err = c.ReconcileSKS(ctx, pa, nv1alpha1.SKSOperationModeServe, 0 /*numActivators == all*/); err != nil {
return fmt.Errorf("error reconciling SKS: %w", err)
}
pa.Status.MarkDependenciesNotReady("DependenciesNotReady", "SKS is provisioning")
Copy link
Member

Choose a reason for hiding this comment

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

"DependenciesNotReady" is redundant with the new Type. I'd use this to identify the dependency itself, e.g. SKSNotReady or something

return computeStatus(ctx, pa, podCounts{want: scaleUnknown}, logger)
}

Expand Down Expand Up @@ -138,7 +139,12 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pa *pav1alpha1.PodAutosc
// If SKS is not ready — ensure we're not becoming ready.
// TODO: see if we can perhaps propagate the SKS state to computing active status.
if !sks.IsReady() {
logger.Debug("SKS is not ready, marking dependencies not ready")
ready = 0
pa.Status.MarkDependenciesNotReady("DependenciesNotReady", "SKS is provisioning")
Copy link
Member

Choose a reason for hiding this comment

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

same

} else {
logger.Debug("SKS is ready, marking dependencies not ready")
pa.Status.MarkDependenciesReady()
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this at a meta level for a moment... Are you sure you want a single condition for all dependencies?
In general, I'd probably advocate for a condition per dependency.

I comment on that here because one of the problems you have with an N-ary condition is: Who should mark it ready?

The logic we have for managing the actual "Ready" condition sort of solves this already, assuming you have a sub-condition per dependency.

}

logger.Infof("PA scale got=%d, want=%d, desiredPods=%d ebc=%d", ready, want,
Expand Down
Loading