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

Notify Chewie of port status events. #2623

Merged
merged 6 commits into from
Dec 7, 2018
Merged

Conversation

Bairdo
Copy link
Contributor

@Bairdo Bairdo commented Nov 8, 2018

Also added code to explicitly remove the ACLs from the port_acl table, although this isn't actually necessary as the caller clears the table for anything that matches the port number.

https://travis-ci.org/Bairdo/faucet/builds/452148059

@codecov
Copy link

codecov bot commented Nov 8, 2018

Codecov Report

Merging #2623 into master will decrease coverage by 0.09%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2623     +/-   ##
=========================================
- Coverage   92.15%   92.05%   -0.1%     
=========================================
  Files          42       42             
  Lines        6077     6083      +6     
=========================================
  Hits         5600     5600             
- Misses        477      483      +6
Impacted Files Coverage Δ
faucet/faucet_dot1x.py 35.71% <16.66%> (-1.52%) ⬇️
faucet/valve.py 94.33% <25%> (-0.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce10cfc...0e66825. Read the comment docs.

@Bairdo
Copy link
Contributor Author

Bairdo commented Nov 13, 2018

I've rerun the failing test on my travis-ci 3 times, and it has not failed again, so possibly a timing issue??
latest run https://travis-ci.org/Bairdo/faucet/jobs/452148067

@gizmoguy
Copy link
Member

gizmoguy commented Nov 27, 2018

I reran on our travis instance three times and always see the same failure:

======================================================================
FAIL: mininet_tests.Faucet8021XPortStatusTest.test_untagged (subunit.RemotedTestCase)
mininet_tests.Faucet8021XPortStatusTest.test_untagged
----------------------------------------------------------------------
testtools.testresult.real._StringException: Traceback (most recent call last):
  File "/faucet-src/tests/integration/mininet_tests.py", line 626, in test_untagged
    self.scrape_prometheus_var('port_dot1x_success', labels={'port': 1}, default=0))
  File "/usr/lib/python3.5/unittest/case.py", line 820, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python3.5/unittest/case.py", line 813, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: 1 != 0

@gizmoguy
Copy link
Member

gizmoguy commented Dec 3, 2018

Same failure again 😞

@Bairdo
Copy link
Contributor Author

Bairdo commented Dec 3, 2018

Yea, I've down merged master in on another branch and got it failing on my instance of travis now.

50 retries doesn't help either

@Bairdo
Copy link
Contributor Author

Bairdo commented Dec 3, 2018

downmerge issue is probably the prometheus rename. "**_total"

@Bairdo
Copy link
Contributor Author

Bairdo commented Dec 3, 2018

still fails

@byllyfish
Copy link
Contributor

It's the labels. Use self.port_labels() to construct the label; it now prepends a 'b' to stuff.

'port_dot1x_success_total{dp_id="0xf0bcfbf4",dp_name="faucet-1",port="b1",port_description="b1"} 1.0',

@gizmoguy
Copy link
Member

gizmoguy commented Dec 3, 2018

Okay looks like this is mostly passing now, unfortunately #2647 broke builds for faucet@master. We're just working on a fix now and when that works I'll let you know to rebase which should make your tests pass again.

@Bairdo
Copy link
Contributor Author

Bairdo commented Dec 3, 2018

Thanks @byllyfish.

@gizmoguy
Copy link
Member

gizmoguy commented Dec 4, 2018

Okay turns out we had a few problems caused by faucetsdn/docker-test-base#9, I have reverted this change and fixed the clib failures we were seeing in #2652.

Rebase on master and this should work now, sorry about all the fuss!

@codecov
Copy link

codecov bot commented Dec 4, 2018

Codecov Report

Merging #2623 into master will decrease coverage by 0.09%.
The diff coverage is 28.57%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2623     +/-   ##
========================================
- Coverage   92.09%     92%   -0.1%     
========================================
  Files          42      42             
  Lines        6082    6088      +6     
========================================
  Hits         5601    5601             
- Misses        481     487      +6
Impacted Files Coverage Δ
faucet/valve.py 94.33% <25%> (-0.22%) ⬇️
faucet/faucet_dot1x.py 34.31% <33.33%> (-1.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ace4460...4e06ac7. Read the comment docs.

@Bairdo
Copy link
Contributor Author

Bairdo commented Dec 4, 2018

FaucetDockerHostTest failed, which I've seen fail at other times, is there something in particular that causes it to fail?

@gizmoguy
Copy link
Member

gizmoguy commented Dec 5, 2018

Hmmmm okay. I thought I had fixed this bug.

Basically the clib tests want to use docker-in-docker which can be unreliable when not done correctly. I will keep investigating at my end and try get to the bottom of this and let you know when it's safe to rebase again.

Sorry again for the disruption.

@gizmoguy
Copy link
Member

gizmoguy commented Dec 5, 2018

hi @Bairdo I finally got this fixed properly in #2657 try a rebase against faucet@master now and that should get you passing again

@gizmoguy
Copy link
Member

gizmoguy commented Dec 5, 2018

Sorry again, you managed to grab faucet@master during our release process which means the version number wasn't set correctly so your tests failed again. Give it one more rebase and you'll be good to go finally

Copy link
Member

@gizmoguy gizmoguy left a comment

Choose a reason for hiding this comment

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

We have a bit of a messy system for handling interface numbers now in case the end user is not using ports 1-4 in their hardware test bed.

For now you can use portN instead of the integers to set which port you are referring to.

tests/integration/mininet_tests.py Outdated Show resolved Hide resolved
tests/integration/mininet_tests.py Outdated Show resolved Hide resolved
tests/integration/mininet_tests.py Outdated Show resolved Hide resolved
@Bairdo
Copy link
Contributor Author

Bairdo commented Dec 6, 2018

I've just updated the docs to mention we support TLS (as of v0.0.10), and PEAP (there is a PR in chewie). I can back it out if it should be in another pr.

@gizmoguy
Copy link
Member

gizmoguy commented Dec 6, 2018

Yeah put the documentation change in a separate PR.

@anarkiwi
Copy link
Member

anarkiwi commented Dec 6, 2018

Does this PR now pass, on a hardware test bed using not ports 1-4?

@Bairdo
Copy link
Contributor Author

Bairdo commented Dec 6, 2018

all 1x tests pass on c9300 not using ports 1,2,3,4

@anarkiwi anarkiwi merged commit aa6784a into faucetsdn:master Dec 7, 2018
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.

4 participants