Move autoscaling config struct into new package #9434

taragu · 2020-09-16T15:26:00Z

~~Moving autoscaling config constants into pkg/autoscaler/config.~~
Move autoscaling config struct into new package

This is needed because currently in order to pass the autoscaler configmap into pkg/apis/autoscaling/annotation_validation.go, we can only pass in individual values in the configmap (see ValidateAnnotations(...)). The reason we cannot pass in the configmap directly is because there will be a cyclic dependency. This can get ugly as we would need to pass in another variable MaxScaleLimit into ValidateAnnotations() for issue #8628.

This PR fixes such cyclic dependency so we don't have to do so, and can pass the entire autoscaler configmap into ValidateAnnotations().

/lint

Part of #8628
/assign @vagababov @markusthoemmes @julz @yanweiguo

knative-prow-robot

@taragu: 0 warnings.

In response to this:

Moving autoscaling config constants into pkg/autoscaler/config.

This is needed because currently in order to pass the autoscaler configmap into pkg/apis/autoscaling/annotation_validation.go, we can only pass in individual values in the configmap (see ValidateAnnotations(...)). The reason we cannot pass in the configmap directly is because there will be a cyclic dependency. This can get ugly as we would need to pass in another variable MaxScaleLimit into ValidateAnnotations() for issue #8628.

This PR fixes such cyclic dependency so we don't have to do so, and can pass the entire autoscaler configmap into ValidateAnnotations().

/lint

Part of #8628
/assign @vagababov @markusthoemmes @julz @yanweiguo

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)

taragu · 2020-09-16T16:51:18Z

(The failed github action should be unrelated, but I don't have write access to the repo and can't rerun it.)

vagababov · 2020-09-16T17:12:06Z

pkg/apis/autoscaling/register.go

 const (
 	domain = ".knative.dev"

 	// InternalGroupName is the internal autoscaling group name. This is used for CRDs.
 	InternalGroupName = "autoscaling.internal.knative.dev"
-
-	// GroupName is the the public autoscaling group name. This is used for annotations, labels, etc.
-	GroupName = "autoscaling.knative.dev"


We should be keeping GroupName here, I think.
Rest, I guess we can move.

I don't think we can keep the GroupName here, because it's used by the other annotation keys, so we will have to import knative.dev/serving/pkg/apis/autoscaling into pkg/autoscaler/config/config.go and the cyclic dependency issue won't be solved.

import cycle not allowed package knative.dev/serving/cmd/activator imports knative.dev/serving/pkg/activator/handler imports knative.dev/serving/pkg/activator/util imports knative.dev/serving/pkg/apis/serving/v1 imports knative.dev/serving/pkg/apis/autoscaling imports knative.dev/serving/pkg/autoscaler/config imports knative.dev/serving/pkg/apis/autoscaling

would it be the end of the world if we just duplicated GroupName in both places?

keep in mind it's 2020, the world's probably ending anyway

vagababov · 2020-09-16T17:56:02Z

😠
Everywhere else we have group in register.go.
Shrug. I am fine.
/assign mattmoor
Maybe Matt has more ideas.

yanweiguo

/lgtm

vagababov · 2020-09-16T18:05:00Z

/lgtm

vagababov · 2020-09-16T18:05:25Z

Duplication works too. ;)

julz · 2020-09-17T13:03:23Z

hmm @taragu surely the DeepCopy is a public method on the struct so is (must be?) in whatever package the struct is in? Any chance you just need to add the right // +k8s:deepcopy-gen=false incantations to the file and to doc.go?

This reverts commit 6d71135.

vagababov · 2020-09-18T16:16:41Z

I probably missed something, but why "config/autoscalerconfig"?
Why not just put everything under config. It's basically empty now.

This reverts commit babe472.

taragu · 2020-09-18T16:26:39Z

@vagababov actually I moved too much in the last commit, and we are back to the cyclic dependency issue. Looks like we can only move the Config struct.

knative-test-reporter-robot · 2020-09-18T17:14:13Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-integration-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-integration-tests:

test/scale.TestScaleToN/scale-200
test/scale.TestScaleToN

taragu · 2020-09-18T18:01:40Z

/retest

taragu · 2020-09-21T14:38:53Z

This is ready for another look.

julz · 2020-09-21T14:42:36Z

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

@@ -26,7 +26,7 @@ import (

 	"knative.dev/pkg/apis"
 	"knative.dev/serving/pkg/apis/autoscaling"
-	autoscalerconfig "knative.dev/serving/pkg/autoscaler/config"
+	"knative.dev/serving/pkg/autoscaler/config/sharedconfig"


this may well be a terrible idea, but what if we called this package autoscalerconfig? Then we'd maybe only have to change the imports - since we're always importing autoscaler/config as this anyway - and the diff would be much smaller (also it seems like maybe a better name for this anyway). Unless there's a place we import both autoscaler/config/sharedconfig and autoscaler/config, but I don't think there can be because it'd be circular then anyway?

There are a few places that we need to import both, such as test/e2e/e2e.go. It's mostly ConfigName and NewConfigFromMap(...) that we are importing from autoscaler/config. I tried to move them to sharedconfig but then we were going back to the circular dependency we had to begin with.

can't you just import them with different names in those couple places? I guess at that point it becomes a bit icky anyway

Ah I see. So instead of autoscalerconfig "knative.dev/serving/pkg/autoscaler/config" we call it asconfig "knative.dev/serving/pkg/autoscaler/config", and rename autoscaler/config/sharedconfig to autoscaler/config/autoscalerconfig ?

yeah, dont honestly know if that's better or worse if we end up having to import both asconfig and autoscalerconfig in the same file tho. Not that autoscalerconfig + sharedconfig is a ton better so 🤷

certainly makes the diff look nicer this way 🤷‍♂️👍️.

…sconfig

codecov · 2020-09-21T15:14:30Z

Codecov Report

❗ No coverage uploaded for pull request base (master@9ceb85e). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #9434   +/-   ##
=========================================
  Coverage          ?   88.57%           
=========================================
  Files             ?      209           
  Lines             ?     9378           
  Branches          ?        0           
=========================================
  Hits              ?     8307           
  Misses            ?      817           
  Partials          ?      254

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ceb85e...bfcbb1b. Read the comment docs.

julz

Package names still make me sad, but I don't have a better solution and this looks pretty good 🤷. Looks good to me apart from couple nits

julz · 2020-09-21T15:14:17Z

pkg/reconciler/autoscaling/kpa/kpa_test.go

@@ -1229,7 +1230,7 @@ func deploy(namespace, name string, opts ...deploymentOption) *appsv1.Deployment
 	return s
 }

-func TestGlobalResyncOnUpdateAutoscalerConfigMap(t *testing.T) {
+func TestGlobalResyncOnUpdateasconfigMap(t *testing.T) {


Suggested change

func TestGlobalResyncOnUpdateasconfigMap(t *testing.T) {

func TestGlobalResyncOnUpdateAutoscalerConfigMap(t *testing.T) {

julz · 2020-09-21T15:14:47Z

pkg/reconciler/autoscaling/kpa/kpa_test.go

-	autoscalerConfig.InitialScale = 0
-	autoscalerConfig.EnableScaleToZero = true
-	return autoscalerConfig
+	asconfig, _ := asconfig.NewConfigFromMap(defaultConfigMapData())


maybe worth avoiding shadowing the package name with the local variable here

julz

/lgtm

knative-metrics-robot · 2020-09-21T15:32:57Z

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/autoscaling/kpa/scaler.go	93.4%	94.3%	0.8

vagababov

I really don't like the stutter in the package naming, but it's ok for now.
when I think of a good name, I'll rename it 😄
/lgtm
/approve

knative-prow-robot · 2020-09-21T16:16:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: taragu, vagababov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~hack/OWNERS~~ [vagababov]
~~pkg/apis/OWNERS~~ [vagababov]
~~pkg/autoscaler/OWNERS~~ [vagababov]
~~pkg/reconciler/OWNERS~~ [vagababov]
~~test/OWNERS~~ [vagababov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow-robot assigned julz, markusthoemmes, vagababov and yanweiguo Sep 16, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 16, 2020

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/API API objects and controllers labels Sep 16, 2020

knative-prow-robot reviewed Sep 16, 2020

View reviewed changes

knative-prow-robot added area/autoscale area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Sep 16, 2020

knative-prow-robot requested review from chaodaiG, julz, markusthoemmes, mdemirhan, tcnghia and vagababov September 16, 2020 15:26

mattmoor reviewed Sep 16, 2020

View reviewed changes

taragu force-pushed the max-maxscale-move-asconsts branch 5 times, most recently from 9c6ab81 to e964caa Compare September 16, 2020 15:52

vagababov reviewed Sep 16, 2020

View reviewed changes

knative-prow-robot assigned mattmoor Sep 16, 2020

yanweiguo reviewed Sep 16, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2020

Tara Gu added 2 commits September 17, 2020 13:01

Revert "Move autoscaling config constants into pkg/autoscaler/config"

94b9fe5

This reverts commit 6d71135.

Move autoscaler Config struct to a separate package

b3658aa

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 17, 2020

Rename sharedconfig -> autoscalerconfig

babe472

taragu changed the title ~~Move autoscaling config constants into pkg/autoscaler/config~~ Move autoscaling config struct into new package Sep 18, 2020

Revert "Rename sharedconfig -> autoscalerconfig"

986792c

This reverts commit babe472.

julz reviewed Sep 21, 2020

View reviewed changes

Tara Gu added 2 commits September 21, 2020 11:02

Renaming existing autoscaler/config import from autoscalerconfig to a…

b125d26

…sconfig

Rename sharedconfig -> autoscalerconfig without moving anything else

c1f523c

julz reviewed Sep 21, 2020

View reviewed changes

Review

0f004f0

julz reviewed Sep 21, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2020

Review cont

bfcbb1b

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2020

vagababov reviewed Sep 21, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2020

knative-prow-robot merged commit 5b761f3 into knative:master Sep 21, 2020

julz mentioned this pull request Oct 13, 2020

Remove no-op deepcopy #9795

Merged

	func TestGlobalResyncOnUpdateasconfigMap(t *testing.T) {
	func TestGlobalResyncOnUpdateAutoscalerConfigMap(t *testing.T) {

Move autoscaling config struct into new package #9434

Move autoscaling config struct into new package #9434

Uh oh!

Conversation

taragu commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

taragu commented Sep 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vagababov commented Sep 16, 2020

Uh oh!

yanweiguo left a comment

Choose a reason for hiding this comment

Uh oh!

vagababov commented Sep 16, 2020

Uh oh!

vagababov commented Sep 16, 2020

Uh oh!

julz commented Sep 17, 2020

Uh oh!

vagababov commented Sep 18, 2020

Uh oh!

taragu commented Sep 18, 2020

Uh oh!

knative-test-reporter-robot commented Sep 18, 2020

Uh oh!

taragu commented Sep 18, 2020

Uh oh!

taragu commented Sep 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

julz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julz left a comment

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Sep 21, 2020

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Sep 21, 2020

Uh oh!

Uh oh!

taragu commented Sep 16, 2020 •

edited

Loading

codecov bot commented Sep 21, 2020 •

edited

Loading

julz left a comment •

edited

Loading