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

Jump to bottom

Add concurrency-utilization option for service create and service update #788

Merged
merged 1 commit into from
Apr 9, 2020
Merged

Add concurrency-utilization option for service create and service update #788

merged 1 commit into from
Apr 9, 2020

Conversation

MIBc
Copy link
Contributor

@MIBc MIBc commented Apr 8, 2020

Fixes: ##768

  • Support setting "autoscaling.knative.dev/targetUtilizationPercentage" annotation.

Release Note:

  • 🎁 New feature
    Add "--target-utilization" to manage "autoscaling.knative.dev/targetUtilizationPercentage" annotation.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 8, 2020
@knative-prow-robot knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 8, 2020
@knative-prow-robot
Copy link
Contributor

Hi @MIBc. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@MIBc MIBc force-pushed the target-utilization branch 2 times, most recently from f1ae618 to 9b4da8e Compare April 8, 2020 02:20
rhuss
rhuss requested changes Apr 8, 2020
View reviewed changes
Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thanks a lot ! Looks good on a first sight, but thinking again about the naming I probably would choose --concurrency-utilization to group all the concurrency related options together (i.e. all prefixed with --concurrency-...)

Also you need to resolve the conflict in the e2e test (because of the e2e refactorings)

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2020
@maximilien
Copy link
Contributor

/ok-to-test

You might first need to resolve conflict: test/e2e/service_options_test.go

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 9, 2020
@MIBc MIBc force-pushed the target-utilization branch from 9b4da8e to 53e7384 Compare April 9, 2020 03:29
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2020
@MIBc MIBc force-pushed the target-utilization branch from 53e7384 to 68d0006 Compare April 9, 2020 05:37
@MIBc MIBc requested a review from rhuss April 9, 2020 05:37
@MIBc
Copy link
Contributor Author

MIBc commented Apr 9, 2020

/retest

@MIBc MIBc force-pushed the target-utilization branch 2 times, most recently from 0f0c6d1 to c44433d Compare April 9, 2020 06:15
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 9, 2020
@rhuss
Copy link
Contributor

rhuss commented Apr 9, 2020

The test is failing because we have switched back to async delete by default and haven't adapted the e2e tests to add a --wait to the delete operations.

@duglin you asked for an evidence why sync deletion is important in e2e tests. The it failure here is one of it.

@MIBc MIBc force-pushed the target-utilization branch from c44433d to 18cd014 Compare April 9, 2020 08:46
mattmoor
mattmoor reviewed Apr 9, 2020
View reviewed changes
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

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

pkg/kn/commands/service/describe_test.go Outdated Show resolved Hide resolved
pkg/serving/config_changes_test.go Outdated Show resolved Hide resolved
@MIBc
Add "--target-utilization" to manage "autoscaling.knative.dev/targetU…
eb750ad 
…tilizationPercentage" annotation

* Support setting "autoscaling.knative.dev/targetUtilizationPercentage" annotation.
@MIBc MIBc force-pushed the target-utilization branch from 18cd014 to eb750ad Compare April 9, 2020 08:48
@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
pkg/kn/commands/revision/describe.go 63.8% 63.2% -0.6
pkg/kn/commands/service/configuration_edit_flags.go 84.7% 84.7% -0.0
pkg/serving/config_changes.go 79.6% 79.6% 0.1
pkg/serving/revision_template.go 58.1% 54.5% -3.5

rhuss
rhuss approved these changes Apr 9, 2020
View reviewed changes
Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

thanks !

@rhuss
Copy link
Contributor

rhuss commented Apr 9, 2020

/approve
/lgtm

@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Apr 9, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MIBc, rhuss

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:

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 knative-prow-robot merged commit deb9630 into knative:master Apr 9, 2020
@MIBc MIBc deleted the target-utilization branch April 9, 2020 15:27
@navidshaikh navidshaikh added the backport/candidate Consider this PR to be backported to the release branch label Apr 15, 2020
@navidshaikh navidshaikh changed the title Add target-utilization option for service create and service update Add concurrency-utilization option for service create and service update Apr 15, 2020
rhuss pushed a commit to rhuss/knative-client that referenced this pull request Apr 15, 2020
@MIBc @rhuss
Add "--target-utilization" to manage "autoscaling.knative.dev/targetU…
3e25a10 
…tilizationPercentage" annotation (knative#788)

* Support setting "autoscaling.knative.dev/targetUtilizationPercentage" annotation.

Signed-off-by: Roland Huß <[email protected]>
# Conflicts:
#	test/e2e/service_options_test.go
@rhuss rhuss mentioned this pull request Apr 15, 2020
Merged
knative-prow-robot pushed a commit that referenced this pull request Apr 15, 2020
@rhuss @maximilien
@dsimansk @navidshaikh @MIBc @itsmurugappan
Pr/release 0.13.2 backports (#806)
b67f302 
* (refactor) address the e2e extract / refactor of issue #763 (#765)

* (refactor) address the e2e extract / refactor of issue #763

* various updates to address reviewers feedback

* renamed lib/test/integration to lib/test and package to test

Signed-off-by: Roland Huß <[email protected]>
# Conflicts:
#	CHANGELOG.adoc
#	test/e2e/service_export_import_apply_test.go
#	test/e2e/trigger_test.go

* fix(plugin): Fix plugin lookup with file ext on Windows (#774)

* fix(plugin): Fix plugin lookup with file ext on Windows

* chore: Update changelog

* fix: Reflect review feedback

* fix: Reflect review feedback and add future todo

Signed-off-by: Roland Huß <[email protected]>
# Conflicts:
#	CHANGELOG.adoc

* fix(issue #762): correct error message when updating service (#778)

* fix(issue #762): correct error message when updating service

* correct message when updating service and passing many names
* fix issue with TestServiceUpdateWithMultipleImages running create vs update

* * added TestServiceDescribeWithMultipleNames
* added TestServiceCreateWithMultipleNames
* fix error message for service delete since many names can be passed

* Use vendored deps while running e2e locally (#783)

Also set GO111MODULE=on unconditionally

* Update sink binding create usage string (#785)

* Add "--target-utilization" to manage "autoscaling.knative.dev/targetUtilizationPercentage" annotation (#788)

* Support setting "autoscaling.knative.dev/targetUtilizationPercentage" annotation.

Signed-off-by: Roland Huß <[email protected]>
# Conflicts:
#	test/e2e/service_options_test.go

* Remove the delete propagation flag (#770)

* Remove the delete propagation flag

In it's current state it now takes me about 25 seconds for the `kn delete`
to complete. Before #682 it used to be
almost immediate. This is because we now pass in the
`DeletePropagationBackground` flag. I believe this is a mistake, not only
because of the 20+ seconds of additional time to delete things, but IMO
the CLI should talk to the server in the same way regardless of the --wait
flag. That flag should just be a CLI thing to indicate if the user wants the CLI
to wait for the server to complete but not HOW the server should do the delete.

Signed-off-by: Doug Davis <[email protected]>

* try just tweaking the --no-wait flag

Signed-off-by: Doug Davis <[email protected]>

* Fix error when output is set to name (#775)

* fix error when output is set to name

* add e2e test

* change to flags/listprint.go

Signed-off-by: Roland Huß <[email protected]>
# Conflicts:
#	test/e2e/basic_workflow_test.go

* Show all revisions when run `service describe -v` (#790)

* The `kn service describe -v` command shows repetitive revisions, because
  the revision would be covered by next one.

* Fix resource listing with -oname flag (#799)

* Fix resource listing with -oname flag

* add e2e tests

Signed-off-by: Roland Huß <[email protected]>
# Conflicts:
#	test/e2e/ping_test.go
#	test/e2e/revision_test.go
#	test/e2e/route_test.go
#	test/e2e/source_apiserver_test.go
#	test/e2e/source_binding_test.go
#	test/e2e/trigger_test.go

* Make wait, no-wait and async flags per bool var CLI convention (#802)

* Make wait, no-wait and async flags per bool var CLI convention

 Fixes #800

 - Deprecated bool vars can be supported for CLI convention
 - Bind --async flag value to --no-wait
 - Only one flag among [wait, no-wait, async] can be provided, else raise an error

* Simplify conditionals

* Add unit tests for deprecated flag async

* Fix a typo

* e2e: Foreground delete for revisions and services in e2e (#794)

* e2e: Foreground delete for revisions and services in e2e

 to avoid any race conditions and flakes

* Use --wait instead of --no-wait=false

Signed-off-by: Roland Huß <[email protected]>
# Conflicts:
#	test/e2e/basic_workflow_test.go
#	test/e2e/revision_test.go

* e2e: Run tekton e2e against pipeline v0.11.1 (#803)

* Use buildah task from master branch and paramterize FORMAT

* Configure pipeline v0.11.1

* DNM: Run tekton e2e in this PR

* Revert "DNM: Run tekton e2e in this PR"

This reverts commit 903f5be.

* Update CHANGELOG for v0.13.2 (#804)

* Pin serving to v0.13.2 and update version command (#797)

* Pin serving v0.13.2 dep to v0.13.2

* Update version command

 now points to serving v0.13.2 and eventing v0.13.6

* Copy go.sum as generated in CI

Signed-off-by: Roland Huß <[email protected]>
# Conflicts:
#	go.mod
#	go.sum
#	vendor/modules.txt

* add missing vendored files

* fixed error reporting for traffics tests

* Updated test

* fix formatting

* e2e for service export (#739)

* e2e for service export

* e2e for service export

* e2e for service export

* e2e for service export

* e2e for service export

Signed-off-by: Roland Huß <[email protected]>
# Conflicts:
#	test/e2e/service_export_import_apply_test.go

Co-authored-by: dr.max <[email protected]>
Co-authored-by: David Simansky <[email protected]>
Co-authored-by: Navid Shaikh <[email protected]>
Co-authored-by: Lv Jiawei <[email protected]>
Co-authored-by: Doug Davis <[email protected]>
Co-authored-by: Ying Chun Guo <[email protected]>
Co-authored-by: Murugappan Chetty <[email protected]>
@navidshaikh navidshaikh added backport/pr A backport PR which is target to a release branch. and removed backport/candidate Consider this PR to be backported to the release branch labels Apr 20, 2020
@rhuss rhuss added backported-to/0.13 and removed backport/pr A backport PR which is target to a release branch. labels Apr 20, 2020
coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020
@adrcunha @knative-prow-robot
Don't fail auto-releases if no tags exist in the repo (knative#788)
de4949e 
This happens for new repos with no releases.

Bonus 1: code cleanup.
Bonus 2: restrict critical failures to the build function.
@navidshaikh navidshaikh mentioned this pull request Jul 1, 2020
Closed
@navidshaikh navidshaikh modified the milestone: v0.14.0 Jul 1, 2020
@MIBc MIBc mentioned this pull request Aug 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattmoor mattmoor mattmoor left review comments

@rhuss rhuss rhuss approved these changes

@evankanderson evankanderson Awaiting requested review from evankanderson

@maximilien maximilien Awaiting requested review from maximilien

Assignees

@rhuss rhuss

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@MIBc @knative-prow-robot @maximilien @rhuss @knative-metrics-robot @mattmoor @navidshaikh @googlebot