Skip to content

proxy-status diff of what Pilot has not what it has sent to Envoy#6818

Merged
rshriram merged 2 commits intoistio:release-1.0from
liamawhite:proxydiff
Jul 3, 2018
Merged

proxy-status diff of what Pilot has not what it has sent to Envoy#6818
rshriram merged 2 commits intoistio:release-1.0from
liamawhite:proxydiff

Conversation

@liamawhite
Copy link
Copy Markdown
Member

After further discussions with users they are interested in what Pilot would send not what Pilot has sent. This is far more useful as it enables them to see if Pilot has propagated their Istio config changes to Envoy yet.

@istio-testing istio-testing requested review from ayj and nmittler July 3, 2018 14:28
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 3, 2018

Codecov Report

Merging #6818 into release-1.0 will decrease coverage by 1%.
The diff coverage is 57%.

Impacted file tree graph

@@             Coverage Diff              @@
##           release-1.0   #6818    +/-   ##
============================================
- Coverage           71%     71%   -<1%     
============================================
  Files              368     369     +1     
  Lines            32078   32146    +68     
============================================
+ Hits             22689   22717    +28     
- Misses            8459    8493    +34     
- Partials           930     936     +6
Impacted Files Coverage Δ
pilot/pkg/proxy/envoy/v2/debug.go 35% <100%> (ø) ⬆️
pilot/pkg/proxy/envoy/v2/cds.go 60% <36%> (+1%) ⬆️
pilot/pkg/proxy/envoy/v2/ads.go 78% <58%> (+2%) ⬆️
pilot/pkg/proxy/envoy/v2/rds.go 58% <58%> (ø)
pilot/pkg/proxy/envoy/v2/lds.go 54% <62%> (+1%) ⬆️
...olarwinds/internal/papertrail/papertrail_logger.go 59% <0%> (-21%) ⬇️
pilot/pkg/config/memory/monitor.go 82% <0%> (-9%) ⬇️
pilot/pkg/serviceregistry/kube/controller.go 69% <0%> (ø) ⬇️
mixer/adapter/cloudwatch/handler.go 87% <0%> (ø) ⬇️
mixer/adapter/denier/denier.go 100% <0%> (ø) ⬆️
... and 4 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 cacc188...4c35bd9. Read the comment docs.

@zachgersh
Copy link
Copy Markdown
Contributor

Is this something that is going to make it into 1.0?

@liamawhite
Copy link
Copy Markdown
Member Author

liamawhite commented Jul 3, 2018

@zachgersh unless anyone has any objections, yes! 🙂

Its only actually a small change, most of the functionality is already there.

func (s *DiscoveryServer) generateRawRoutes(con *XdsConnection) ([]*xdsapi.RouteConfiguration, error) {
rc := make([]*xdsapi.RouteConfiguration, 0)
// TODO: Follow this logic for other xDS resources as well
// And cache/retrieve this info on-demand, not for every request from every proxy
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.

Can we take this out? If someone needs this commented out code we can bring it back since its already in source control!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

These aren't my comments I just split out what we had into a separate file. We could ask whoever wrote them in the first place though I guess?

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.

it was @rshriram :D. @rshriram can we please delete these and bring them back when needed? Either that, or lets put a bug in for caching so we can track this via github.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sure go for it.

Copy link
Copy Markdown
Contributor

@nmittler nmittler left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@istio-testing
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liamawhite, nmittler

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

@rshriram rshriram merged commit 6c8b287 into istio:release-1.0 Jul 3, 2018
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