Skip to content

Override locality load balancer settings per host#18406

Merged
istio-testing merged 27 commits intoistio:masterfrom
aattuluri:Override-locality-load-balance-settings-per-host
Nov 6, 2019
Merged

Override locality load balancer settings per host#18406
istio-testing merged 27 commits intoistio:masterfrom
aattuluri:Override-locality-load-balance-settings-per-host

Conversation

@aattuluri
Copy link
Copy Markdown
Member

@aattuluri aattuluri commented Oct 28, 2019 •
edited
Loading

Fixes #17802

@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Oct 28, 2019
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test labels Oct 28, 2019
@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Oct 28, 2019

Hi @aattuluri. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

howardjohn
howardjohn reviewed Oct 28, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

Nice! Can you change tests/integration/pilot/locality to use a destination rule? Basically should just need to remove the setupConfig function and then modify the DR to add the locality settings there. There is already a DR to add the outlier detection so can just add to that one

@howardjohn
Copy link
Copy Markdown
Member

howardjohn commented Oct 28, 2019

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Oct 28, 2019
@aattuluri aattuluri requested a review from a team as a code owner October 28, 2019 23:18
howardjohn
howardjohn reviewed Oct 28, 2019
View reviewed changes
@jasonwzm
Copy link
Copy Markdown
Member

jasonwzm commented Oct 29, 2019

@aattuluri you will need to use the generated CRDs you updated in https://github.com/istio/api/blob/master/kubernetes/customresourcedefinitions.gen.yaml. Please rebase and use the make update-crds command to update that.

@aattuluri aattuluri requested a review from a team as a code owner October 29, 2019 13:21
@aattuluri
Copy link
Copy Markdown
Member Author

aattuluri commented Oct 29, 2019

@aattuluri you will need to use the generated CRDs you updated in https://github.com/istio/api/blob/master/kubernetes/customresourcedefinitions.gen.yaml. Please rebase and use the make update-crds command to update that.

Thank you.

@aattuluri aattuluri changed the title Override locality load balancer settings per host (WIP) Override locality load balancer settings per host Oct 29, 2019
@jasonwzm
Copy link
Copy Markdown
Member

jasonwzm commented Oct 29, 2019

@aattuluri it looks like the testdata is still not valid.

@aattuluri
Copy link
Copy Markdown
Member Author

aattuluri commented Nov 5, 2019

since they failed repeatedly on this PR but not others, I think its cause by this change.

That being said, I don't see why. May be some subtle issue or as a result of other changes pull in from api update?

Ok, probably I should try downgrading to some previous version of api on my branch and check?

@jasonwzm
Copy link
Copy Markdown
Member

jasonwzm commented Nov 5, 2019

If you compare the API between your original change and the master, there is little changed on the API definitions themselves and the generated go types. istio/api@9a5ec23...master

@jasonwzm
Copy link
Copy Markdown
Member

jasonwzm commented Nov 5, 2019

We added MarshalJSON methods to the APIs in the latest API. I am guessing that is messing around with our structpath implementation. I will look into that tomorrow. In the meantime, please try to use commit istio/api@9fe6a7d. (gencheck probably will fail, but let's see if the other tests are okay.)

@aattuluri
Copy link
Copy Markdown
Member Author

aattuluri commented Nov 5, 2019

/retest

@jasonwzm
Copy link
Copy Markdown
Member

jasonwzm commented Nov 5, 2019

@aattuluri the crd yaml in the api commit cannot be parsed by our test framework. that is why the tests are failing now. you can keep last crd-all.gen.yaml as unchanged.

@jasonwzm
Copy link
Copy Markdown
Member

jasonwzm commented Nov 5, 2019

some values in our synthetic service entry tests are invalid. im trying to spot those and hopefully can fix the test failures we were seeing yesterday.

@aattuluri
Copy link
Copy Markdown
Member Author

aattuluri commented Nov 5, 2019

@aattuluri the crd yaml in the api commit cannot be parsed by our test framework. that is why the tests are failing now. you can keep last crd-all.gen.yaml as unchanged.

Yep, reverted that.

@aattuluri
Copy link
Copy Markdown
Member Author

aattuluri commented Nov 5, 2019 •
edited
Loading

some values in our synthetic service entry tests are invalid. im trying to spot those and hopefully can fix the test failures we were seeing yesterday.

That will be great, thanks!

@aattuluri aattuluri requested a review from a team November 6, 2019 00:24
@aattuluri aattuluri requested a review from a team as a code owner November 6, 2019 00:24
@hzxuzhonghu
Copy link
Copy Markdown
Member

hzxuzhonghu commented Nov 6, 2019

/retest

1 similar comment
@ghost
Copy link
Copy Markdown

ghost commented Nov 6, 2019

/retest

@jasonwzm jasonwzm requested a review from ayj November 6, 2019 04:37
@jasonwzm
Copy link
Copy Markdown
Member

jasonwzm commented Nov 6, 2019

Since I made commits to the PR, I will not be approving on behalf of config team. @ayj Can you please take a look? Particularly the fixes for the json naming in conformance tests and synthetic service entry integration tests, which I mentioned to you today.

Unfortunately, this PR has grown to a huge one because the latest api commit had a few changes.

@jasonwzm
Copy link
Copy Markdown
Member

jasonwzm commented Nov 6, 2019

@kyessenov Kuat, can you please review the changes of json naming in mixer tests?

@jasonwzm jasonwzm requested a review from kyessenov November 6, 2019 17:33
@istio-testing istio-testing merged commit 4ecf35b into istio:master Nov 6, 2019
@howardjohn
Copy link
Copy Markdown
Member

howardjohn commented Nov 6, 2019

@aattuluri thanks for implementing this and everything else that got dragged along with it!

@ghost
Copy link
Copy Markdown

ghost commented Nov 7, 2019

@howardjohn Sure thing. Thanks for being proactive and helping on this one.

@sushicw sushicw mentioned this pull request Nov 7, 2019
Merged
@ghost ghost mentioned this pull request Nov 8, 2019
Closed
5 tasks
@yingzhuivy yingzhuivy mentioned this pull request Nov 12, 2019
Closed
sdake pushed a commit to sdake/istio that referenced this pull request Dec 1, 2019
@aattuluri
Override locality load balancer settings per host (istio#18406)
886ac59 
* Fix version

* Update istio api version to the latest.

* Update test data to test the feature

* Fix indentation and config.

* Fix indentation round 2

* Indentation fix round 3.

* Update crds for tests.

* remove yaml separator

* Add loadbalancer mandatory attribute for test data.

* Incorporate code review comments.

* Updated to reflect existing behavior

* Update import

* Restore to original order.

* Fix unit tests.

* Fix lint errors.

* Check in make gen output

* Update istio api and redo make gen

* Testing with rolled back api version

* Keeping the old crd gen file.

* fix json field names in tests and update to latest api

* fix go sum

* fix json naming in conformance tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@howardjohn howardjohn howardjohn approved these changes

@kyessenov kyessenov kyessenov approved these changes

@hzxuzhonghu hzxuzhonghu hzxuzhonghu approved these changes

@geeknoid geeknoid Awaiting requested review from geeknoid

@linsun linsun Awaiting requested review from linsun linsun is a code owner

@rshriram rshriram Awaiting requested review from rshriram

+1 more reviewer

@ayj ayj ayj approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/networking cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support Locality Load Balancer settings override per host

10 participants

@aattuluri @istio-testing @howardjohn @jasonwzm @rshriram @hzxuzhonghu @ayj @kyessenov @googlebot @istio-policy-bot