Skip to content

Conversation

@JungukCho
Copy link
Contributor

@JungukCho JungukCho commented Jul 16, 2021

Reason for Change:

TestAllowMultiplePodSelectors has a flake UT result due to the ordering of ipset in Specs since the order of all ipset values in Specs is not guaranteed in MultiplePodSelectors case.
Thus, in this UT, before checking the iptables entries, sorting Specs is necessary before comparing them.

You can break this UT with this script without this PR.

max=1000
idx=0
while [[ $idx -ne $max ]]
do
/usr/local/go/bin/go test -timeout 30s -v --count=1 -run ^TestAllowMultiplePodSelectors$ github.com/Azure/azure-container-networking/npm
if [[ $? -ne 0 ]]
then
	exit
fi

idx=$((idx+1))
echo $idx
done

Issue Fixed:

Requirements:

Notes:
While we have one UT to be sorted in current UTs, we need to generalize this approach in all tests to avoid undeterministic behavior in the added UTs in the future. In addition, I feel like we need to clean up translatePolicy_test.go since it is unnecessarily too lengthy.

@JungukCho JungukCho added bug npm Related to NPM. labels Jul 16, 2021
vakalapa
vakalapa previously approved these changes Jul 16, 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

@JungukCho
Copy link
Contributor Author

JungukCho commented Jul 19, 2021

There is another issue in this PR (sorted one can cause incorrect results as well). I will update the issue in details and codes according to the issue.

The issue of previous commit is to sort all strings in specs of ipentries. So, if translation function returns incorrect values, they are sorted and possible to be match with expected values.
For example, if translation function returns "azure-npm-a src azure-npm-b dst" with incorrect logics, it will be equal to expected value "azure-npm-a dst azure-npm-b src" after sorting returned and expected values. This is incorrect behavior.
So, it needs to tie a direction with ipset, and then compare the values against expected values.

Note: before merging it, I need to check "expectedLists" values to see whether they have the same issues or not.

@rbtr rbtr dismissed vakalapa’s stale review July 19, 2021 17:20

"There is another issue in this PR (sorted one can cause incorrect results as well). I will update the issue in details and codes according to the issue."

@JungukCho
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@vakalapa
Copy link
Contributor

vakalapa commented Aug 2, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@JungukCho
Copy link
Contributor Author

Close now. I am working on fixing this issue in another PR (refactoring translatePolicy.go) which fundamentally fixes this issue while there are reusable commits here. I will revisit them in the other PR.

@JungukCho JungukCho closed this Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug npm Related to NPM.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants