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.

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

KIALI-1698 Limit VS badging to Service Nodes #542

Merged
merged 1 commit into from Oct 9, 2018

Conversation

jshaughn
Copy link
Collaborator

@jshaughn jshaughn commented Oct 4, 2018

  • this also improves the logic for CB badging in the appender although given
    the behavior of our checking call the on-screen behavior is unchanged (meaning
    that the service node will not get badged if the traffic policy is defined
    only in subsets)

@jshaughn
Copy link
Collaborator Author

jshaughn commented Oct 4, 2018

@lucasponce I added you as a reviewer just so you would see this, because it's related to your KIALI-1680 jira. If you have any comments please add, even if this is already merged.

Copy link
Collaborator

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

I think we should wait for more conversations on this before we merge - particularly about the CB stuff.

}
}
default:
if n.Version == "" || n.Version == graph.UnknownVersion {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why skip if version is ""? If this is an app node, would it be possible for a CB to be on the node? i.e. doesn't CheckDestinationRuleCircuitBreaker accept a "" version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jmazzitelli , I agree. In fact I've re-thought and formalized the scenarios. Please see the related Jira comment. If you are OK with the scenarios you can then ensure the PR actualizes the design.

@kiali kiali deleted a comment from rhqci Oct 5, 2018
@jshaughn jshaughn force-pushed the kiali-1698 branch 2 times, most recently from 00b0c80 to 10a798b Compare October 5, 2018 21:14
@kiali kiali deleted a comment from rhqci Oct 5, 2018
@kiali kiali deleted a comment from rhqci Oct 5, 2018
Here are the rules applied:
- VS Badging is done only on service nodes in the graph.
- CB Badging is done on a Service node with a CB destination rule or a subset with a CB destination rule
  - note: CB dest rule = traffic policy of outlierDetection or connectionPool
- CB Badging is done on a destination App node handling a CB-service.
- CB Badging is done on a destination VersionedApp node handling a CB-service with CB destination rule subset relevant to the version.
- CB Badging is not done on a App Box containing a CB-badged node. CB Badging is never done on an App Box. It is too noisy as the relevant member node will already be badged.
@jmazzitelli
Copy link
Collaborator

jmazzitelli commented Oct 8, 2018

I create bookinfo-ratings-delay.yaml, I see these - looks correct. note we only see the badge on the service (triangle) nodes:

image

image

image

image

image

image

@jshaughn
Copy link
Collaborator Author

jshaughn commented Oct 8, 2018

@jmazzitelli All of those screenshots seem correct to me. We only apply VS badges to service nodes to indicate which service definitions are virtual services in Istio. This is in line with the details pages in Kiali, where VS and DR are shown on the service detail pages.

@rhqci
Copy link

rhqci commented Oct 8, 2018

✔️ Jenkins CI: kiali-core-pr-e2e-test #148

KIALI_NAME=kialicorepr542-istio-system

@kiali kiali deleted a comment from rhqci Oct 8, 2018
Copy link
Collaborator

@jmazzitelli jmazzitelli left a comment

Choose a reason for hiding this comment

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

I think this is working as per the design documented in the JIRA.
@lucasponce you should take a look.

Copy link
Contributor

@lucasponce lucasponce left a comment

Choose a reason for hiding this comment

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

LGTM
The graph screenshots looks inline with the concepts as VS/DR are defined at service level.

@jshaughn jshaughn merged commit dabd4be into kiali:master Oct 9, 2018
@jshaughn jshaughn deleted the kiali-1698 branch October 9, 2018 12:20
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.

None yet

4 participants