Skip to content

Conversation

@JungukCho
Copy link
Contributor

@JungukCho JungukCho commented Jul 23, 2021

Reason for Change:

translatePolicy function returns duplicated ipsets for namedPort if duplicated ipsets exist, which causes unnecessary call in network policy controller. While the duplicated ipset is filtered in network policy controller, it is the best to avoid the call.

Issue Fixed:

Requirements:

Notes:

@JungukCho JungukCho changed the title Remove duplicate namedports in slices to avoid unnecessary CreateSet … [NPM] Remove duplicate namedports in slices to avoid unnecessary CreateSet … Jul 23, 2021
Copy link
Contributor

@vakalapa vakalapa left a comment

Choose a reason for hiding this comment

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

Lgtm, not sure how we missed this earlier. Worst case scenario we will wait a couple of ipset create calls i think. good to catch

@JungukCho JungukCho changed the title [NPM] Remove duplicate namedports in slices to avoid unnecessary CreateSet … [NPM] Remove duplicate namedports in slices to avoid unnecessary CreateSet calls in network policy controller Jul 23, 2021
@JungukCho
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@matmerr matmerr left a comment

Choose a reason for hiding this comment

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

approving with the caveat that this change is a bandaid fix, the parent issue of adding duplicates requires a much larger refactor and is actively being discussed how to fix in translatepolicy

@JungukCho
Copy link
Contributor Author

approving with the caveat that this change is a bandaid fix, the parent issue of adding duplicates requires a much larger refactor and is actively being discussed how to fix in translatepolicy

Correct. I am working on refactoring translatepolicy.go with determinisitic UTs together in #941.

@JungukCho JungukCho merged commit 161a5c7 into Azure:master Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants