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-402 use LabelsMatch algo to match deployments in service details #156

Merged
merged 3 commits into from Apr 13, 2018

Conversation

@jotak
Copy link
Contributor

commented Apr 13, 2018

Some changes are necessary for that, such as passing []Deployment instead of DeploymentsList in the k8s temporary model

@jotak jotak added the do not merge label Apr 13, 2018
@jotak jotak requested a review from xeviknal Apr 13, 2018
@jotak

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2018

Please do not merge yet, I want to add unit tests

@@ -108,7 +103,7 @@ func (in *IstioClient) GetServiceDetails(namespace string, serviceName string) (
}()

go func() {
deployments, err := in.k8s.AppsV1beta1().Deployments(namespace).List(*getDeploymentFilterListOptions(serviceName))
deployments, err := in.k8s.AppsV1beta1().Deployments(namespace).List(emptyListOptions)

This comment has been minimized.

Copy link
@xeviknal

xeviknal Apr 13, 2018

Member

I am having doubts here. In this case, you move label matching from k8s-side to our side. So now we are listing whole deployments, that might be expensive. Why did you consider this way instead of pass service.selector as OptionList?

This comment has been minimized.

Copy link
@jotak

jotak Apr 13, 2018

Author Contributor

Because the other way doesn't work :)

As far as I understand, this is the exact same problem we had in ServiceList and the reason why you added the LabelsMatch algorithm (please explain to me if i'm wrong). Here we may get more results than necessary but then it's filtered in the exact same way we do for ServiceList so we have the same result.

Here deployments are queried with label app=istio-mixer. This is wrong, we cannot presume how the objects are labelled by third-party software. The solution with LabelsMatch is maybe not perfect but at least, doing so makes our behaviour consistent between service list & service details.

This comment has been minimized.

Copy link
@jotak

jotak Apr 13, 2018

Author Contributor

@xeviknal maybe we can directly pass service's labels (and not selector) to the deployment query. It still creates an assumption, but it's the same that we do in LabelsMatch, and it will reduce the amount of data retrieved... I'm trying that

This comment has been minimized.

Copy link
@xeviknal

xeviknal Apr 13, 2018

Member

Yes, it works (although you have to wait for service to be fetched). I tried before and it filter deployments properly by the service selector labels. But okay, I guess you prefer to share common filter logic but pay a little bit on performance downloading some extra deployments.

On the other hand, for sure, using this 'app=mixer' was a bad idea due to lack of knowledge :)

This comment has been minimized.

Copy link
@jotak

jotak Apr 13, 2018

Author Contributor

No, you're right, I didn't noticed we could pass as option the same filtering data that we're doing in LabelsMatch.
I've updated the PR.

@jotak jotak force-pushed the jotak:kiali-402 branch from 6604f12 to 368eafa Apr 13, 2018
@jotak jotak removed the do not merge label Apr 13, 2018
@jotak

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2018

@xeviknal I added some tests and updated with your comment

func GetLabeledListOptions(labelSelector string) *meta_v1.ListOptions {
return &meta_v1.ListOptions{
LabelSelector: labelSelector}
func selectorToString(selector map[string]string) string {

This comment has been minimized.

Copy link
@xeviknal

xeviknal Apr 13, 2018

Member

How much do you like this? Maybe I am being so picky, but when I discovered I though was useful (a lil bit ruby-ish)

querySelector := make([]string, 0, len(labelSelector))
for label, name := range labelSelector {
	querySelector = append(querySelector, label+"="+name)
}
return strings.Join(querySelector, ",")

This comment has been minimized.

Copy link
@jotak

jotak Apr 13, 2018

Author Contributor

done

Copy link
Member

left a comment

LGTM! Comments addressed and tests suite is in green!

@xeviknal xeviknal merged commit 3d8f22f into kiali:master Apr 13, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jotak jotak deleted the jotak:kiali-402 branch Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.