Skip to content

503s: create DestinationRules before VirtualServices#6705

Merged
rshriram merged 6 commits intorelease-1.0from
bookinfo
Jul 5, 2018
Merged

503s: create DestinationRules before VirtualServices#6705
rshriram merged 6 commits intorelease-1.0from
bookinfo

Conversation

@andraxylia
Copy link
Copy Markdown
Contributor

Bookinfo changes to ensure when we create and delete VirtualServices via istioctl replace we do not delete the DestinationRules and cause traffic loss.

Associated istio.io change to come.

Copy link
Copy Markdown
Member

@rshriram rshriram left a comment

Choose a reason for hiding this comment

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

/lgtm
I think you need to update tests to refer to new files. Else tests won’t pass

@@ -53,78 +53,3 @@ spec:
host: details
subset: v1
---
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.

This file is now the same as route-rule-all-v1.yaml. Delete it?

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.

Good catch, removed.

labels:
version: v1
---
---
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.

delete this line

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 28, 2018

Codecov Report

Merging #6705 into release-1.0 will decrease coverage by 1%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##           release-1.0   #6705     +/-   ##
=============================================
- Coverage           71%     71%    -<1%     
=============================================
  Files              370     354     -16     
  Lines            31886   30657   -1229     
=============================================
- Hits             22363   21497    -866     
+ Misses            8586    8314    -272     
+ Partials           937     846     -91
Impacted Files Coverage Δ
pilot/pkg/networking/core/v1alpha3/configgen.go 0% <0%> (-100%) ⬇️
pilot/pkg/config/aggregate/config.go 0% <0%> (-79%) ⬇️
pilot/pkg/networking/core/v1alpha3/cluster.go 0% <0%> (-38%) ⬇️
pilot/pkg/networking/plugin/authz/rbac.go 73% <0%> (-6%) ⬇️
pilot/pkg/proxy/envoy/v2/debug.go 30% <0%> (-4%) ⬇️
mixer/adapter/signalfx/cumulative.go 67% <0%> (-4%) ⬇️
istioctl/pkg/writer/envoy/configdump/route.go 80% <0%> (-2%) ⬇️
istioctl/pkg/writer/envoy/configdump/listener.go 87% <0%> (-2%) ⬇️
pilot/pkg/serviceregistry/kube/controller.go 67% <0%> (-1%) ⬇️
mixer/adapter/fluentd/fluentd.go 74% <0%> (-1%) ⬇️
... and 67 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 bfe4526...00819c2. Read the comment docs.

@andraxylia
Copy link
Copy Markdown
Contributor Author

thanks @rshriram will do

@andraxylia
Copy link
Copy Markdown
Contributor Author

I notice in fact the mixer test still exercise v1 rule from /kube directory. We need to change them to exercise v1alpha3 rules from the routing directory, cc @douglas-reid .

@douglas-reid
Copy link
Copy Markdown
Contributor

if the tests are running with the old rules, then the flag that was being set for those tests is no longer being set.

This PR made the transition, iirc: #6378

@andraxylia
Copy link
Copy Markdown
Contributor Author

/retest prow/istio-presubmit.sh

@andraxylia
Copy link
Copy Markdown
Contributor Author

@frankbu can you please approve? All the files there are needed, the destination-rule-all-mtls.yaml contains a different policy.

@andraxylia
Copy link
Copy Markdown
Contributor Author

/retest all

@rshriram rshriram dismissed frankbu’s stale review July 2, 2018 23:38

comments have been addressed

@rshriram
Copy link
Copy Markdown
Member

rshriram commented Jul 2, 2018

/lgtm

@istio-testing istio-testing removed the lgtm label Jul 3, 2018
@istio-testing
Copy link
Copy Markdown
Collaborator

New changes are detected. LGTM label has been removed.

@andraxylia andraxylia changed the base branch from master to release-1.0 July 3, 2018 22:06
@istio-testing
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andraxylia, rshriram

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

@andraxylia
Copy link
Copy Markdown
Contributor Author

@costinm @rshriram @hklai needs admin to be merged.

@rshriram rshriram merged commit d676c49 into release-1.0 Jul 5, 2018
@andraxylia andraxylia deleted the bookinfo branch July 5, 2018 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants