Skip to content

v1 to v3 rule conversion tool bug fixes and subset merging#5126

Closed
esnible wants to merge 38 commits intomasterfrom
issue3621
Closed

v1 to v3 rule conversion tool bug fixes and subset merging#5126
esnible wants to merge 38 commits intomasterfrom
issue3621

Conversation

@esnible
Copy link
Copy Markdown
Contributor

@esnible esnible commented Apr 23, 2018

No description provided.

Copy link
Copy Markdown
Contributor

@ijsnellf ijsnellf left a comment

Choose a reason for hiding this comment

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

Are you making sure we don't end up with duplicate DestinationRules? convert.DestinationPolicies will convert DestinationPolicies to DestinationRules, so you might end up with duplicates.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 23, 2018

Codecov Report

Merging #5126 into master will increase coverage by 1%.
The diff coverage is 50%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #5126    +/-   ##
=======================================
+ Coverage      73%     73%    +1%     
=======================================
  Files         316     316            
  Lines       26409   26515   +106     
=======================================
+ Hits        19222   19323   +101     
- Misses       6410    6421    +11     
+ Partials      777     771     -6
Impacted Files Coverage Δ
istioctl/cmd/istioctl/convert/cmd.go 44% <50%> (+4%) ⬆️
pilot/pkg/serviceregistry/eureka/controller.go 82% <0%> (-9%) ⬇️
mixer/adapter/solarwinds/metrics_handler.go 75% <0%> (-8%) ⬇️
mixer/adapter/servicecontrol/handler.go 39% <0%> (-3%) ⬇️
mixer/adapter/fluentd/fluentd.go 75% <0%> (-1%) ⬇️
mixer/adapter/rbac/rbacStore.go 84% <0%> (ø) ⬇️
mixer/adapter/circonus/circonus.go 71% <0%> (ø) ⬇️
mixer/adapter/memquota/rollingWindow.go 100% <0%> (ø) ⬆️
mixer/adapter/prometheus/prometheus.go 100% <0%> (ø) ⬆️
mixer/adapter/memquota/dedup.go 100% <0%> (ø) ⬆️
... and 8 more

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 7b1aaff...3f51cb7. Read the comment docs.

@esnible
Copy link
Copy Markdown
Contributor Author

esnible commented Apr 23, 2018

/assign @liamawhite

xiaolanz and others added 2 commits April 23, 2018 09:03
Automatic merge from submit-queue.

Add a linter check to make sure types.go are generated.

addresses #4418
* Remove orig_ manifests

* Remove istio-mixer-validator and istio-mixer-with-health-check manifests

* Remove unwanted manifests before archiving

* Remove istio-sidecar-injector.yaml from install/README.md

* Remove *one-namespace*.yaml from install/README.md

* Make helm-generated manifests overwrite updateVersion_orig.sh manifests
@esnible esnible changed the title Support for Subset and Dest Policy fix WIP: Support for Subset and Dest Policy fix Apr 23, 2018
douglas-reid and others added 10 commits April 23, 2018 12:55
Automatic merge from submit-queue.

Adding CI workflow for checking vendor diff

This aims to help ensure that a PR contains the correct vendor change,
by running `dep ensure` and seeing if git detects any changes.
Automatic merge from submit-queue.

Introduce galley/pkg/server

galley/pkg/server implements logic performs both CRD synchronization, along with resource synchronization operations. The resource synchronizers are started/stopped as CRDs (of interest) are added/deleted.
Automatic merge from submit-queue.

[vendor change] Add metrics command to istioctl experimental cli

This PR adds a new command for retrieving service-level metrics
for services within an Istio service mesh. In combination with
the `watch` command, this tool may be used to display a rudimentary
service dashboard from the commandline.

This command requires the deployment of a prometheus instance for
monitoring the mesh. It discovers a prometheus pod, establishes a
port-forward to that pod, and executes a series of queries to extract
the metrics for display.

Currently, this command pulls all metrics from the current time, 
calculating rates and latencies over a time window of 1 minute. In 
the future, it will be possible to add support for flexible time
windows.

Example usage (bookinfo example):

```
$ istioctl experimental metrics productpage reviews ratings details
productpage:
  Total RPS:     7.872870
  Error RPS:     0.000000
  P50 Latency:   40ms
  P90 Latency:   80ms
  P99 Latency:   98ms
reviews:
  Total RPS:     7.909235
  Error RPS:     0.000000
  P50 Latency:   4ms
  P90 Latency:   9ms
  P99 Latency:   21ms
ratings:
  Total RPS:     5.309187
  Error RPS:     0.000000
  P50 Latency:   2ms
  P90 Latency:   4ms
  P99 Latency:   4ms
details:
  Total RPS:     7.872870
  Error RPS:     0.000000
  P50 Latency:   3ms
  P90 Latency:   38ms
  P99 Latency:   48ms
``` 

This tool is intended primarily to aid with debugging, as discovering
what is happening with a mesh and/or a particular service can be somewhat
cumbersome.

Reviewers: please let me know if there is a more appropriate place for 
such a tool and if there is more/different information that you think
is relevant to display for a service.

Vendor PR: istio/old_vendor-istio_repo#58
Automatic merge from submit-queue.

unset IFS, minor fix for perf setup
* need git pull --tags to get latest_release movement, use DUR variable for duration

* Add grafana ingress

Doesn’t work because of mixer/telemetry split yet but almost

Also had to disable mtls for grafana - this should be the default

* Add annotation for no mtls in helm template

* From 0.8 prometheus is already in the yaml

See #5111
Automatic merge from submit-queue.

Assert requried circle CI envs in ci2gubernator

There has been cases where tests on circle failed when calling ci2gubernator because `CIRCLE_PR_NUMBER` unbound. This PR asserts the existence of the circle ci envs required by ci2gubernator and resort to no op if any of those is not defined.
@esnible esnible changed the title WIP: Support for Subset and Dest Policy fix Support for Subset and Dest Policy fix Apr 24, 2018
Automatic merge from submit-queue.

Add Mixer perf tests that includes the RPC path.

The perf tests included two sets of tests (proper v.s. with _R2 suffix).
The tests with _R2 suffix was for testing runtime2 implementation.

Now that there is only one runtime, repurposing some of the tests to
include the gRpc layer as well.
@esnible esnible changed the title Support for Subset and Dest Policy fix v1 to v3 rule conversion tool bug fixes Apr 24, 2018
@esnible esnible changed the title v1 to v3 rule conversion tool bug fixes v1 to v3 rule conversion tool bug fixes and subset merging Apr 24, 2018
Copy link
Copy Markdown
Member

@liamawhite liamawhite left a comment

Choose a reason for hiding this comment

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

The code looks fine to me. I would like the opinion of an SME on this though.

@liamawhite
Copy link
Copy Markdown
Member

/assign @ijsnellf

Steven Dake and others added 10 commits April 24, 2018 11:00
Automatic merge from submit-queue.

Consume labeled multicluster secrets on startup

This patch when run against istio.yaml or istio-auth.yaml
runs in the new config mode using only labels rather than
configmaps.  The configmap functionality can be removed in
0.9.
…5152)

* Add/Update Mixer e2e tests to cover more attributes sent from Envoy.

* Fix indent.
* assorted bug fixes

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* lint

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@istio-testing
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: hklai

Assign the PR to them by writing /assign @hklai in a comment when ready.

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

The pull request process is described here

Details 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

@googlebot
Copy link
Copy Markdown
Collaborator

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@googlebot googlebot added cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. and removed cla: yes labels Apr 24, 2018
gyliu513 and others added 10 commits April 24, 2018 10:29
Automatic merge from submit-queue.

Updated zipkin to 2.7 for istio.

This is a follow up PR for #4726

/cc @ldemailly
Automatic merge from submit-queue.

Move mixer filter to per_filter_config

Move the per route mixer filter config from the metadata field to per_filter_config and turn it into a ServiceConfig proto.
@googlebot
Copy link
Copy Markdown
Collaborator

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@esnible
Copy link
Copy Markdown
Contributor Author

esnible commented Apr 24, 2018

Closing in favor of #5178 after a bad rebase and merge.

@esnible esnible closed this Apr 24, 2018
@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Apr 24, 2018

@esnible: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/e2e-bookInfoTests-v1alpha3.sh 3f51cb7 link /test e2e-bookInfo-envoyv2-v1alpha3
Details

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. I understand the commands that are listed here.

@rshriram rshriram deleted the issue3621 branch April 25, 2018 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA.

Projects

None yet

Development

Successfully merging this pull request may close these issues.