Skip to content

Conversation

@murali-reddy
Copy link
Member

Currently only endpoint IP used so any change in port of the endpoint leaves stale ipvs server config. With this change (ip, port) tuple is used to track active endpoitns of service and to identify stale IPVS config of server of a service

also service info that is built does not carry targetport info, so any change to just targetPort in service spec was not trigerring sync

Fixes #841

…in use. Currently only endpoint IP

used so any change in port of the endpoint leaves stale ipvs server config

Fixes #841
@murali-reddy murali-reddy requested review from andrewsykim and removed request for andrewsykim February 18, 2020 19:15
@murali-reddy
Copy link
Member Author

@aauren Please review when you get a chance

namespace string
clusterIP net.IP
port int
targetPort string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why a string type? Shouldn't it have the same type as port?

Copy link
Member Author

Choose a reason for hiding this comment

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

TargetPort in service spec is intstr type.

Copy link
Collaborator

@aauren aauren left a comment

Choose a reason for hiding this comment

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

I haven't gotten a chance to test this out in a real environment, but the changes here look reasonable from a code perspective.

Are there any unit tests that need to be augmented to test the new port tracking?

@murali-reddy
Copy link
Member Author

@aauren thanks for the review

Are there any unit tests that need to be augmented to test the new port tracking?

No exisiting unit test covers this scenario. I will add unit tests in a seperate PR for this issue and for #828

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.

LVS doesn't follow nodeport service update accordingly

4 participants