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

Refactor ShellCommandHelper and add OvsHelper #934

Merged
merged 9 commits into from
Nov 30, 2021

Conversation

anurag6
Copy link
Collaborator

@anurag6 anurag6 commented Nov 29, 2021

The PR adds a module that helps create the OVS n/w setup needed to query DAQ interface/simulate an access switch.

@codecov
Copy link

codecov bot commented Nov 29, 2021

Codecov Report

Merging #934 (5de5e92) into master (4432a4f) will decrease coverage by 0.27%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #934      +/-   ##
==========================================
- Coverage   82.70%   82.42%   -0.28%     
==========================================
  Files          46       46              
  Lines        5862     5862              
==========================================
- Hits         4848     4832      -16     
- Misses       1014     1030      +16     
Flag Coverage Δ
ata 62.63% <ø> (-0.68%) ⬇️
aux 68.23% <ø> (-0.02%) ⬇️
base 66.35% <ø> (-0.16%) ⬇️
dhcp 67.50% <ø> (ø)
many 67.44% <ø> (ø)
mud 72.00% <ø> (-0.20%) ⬇️
switch 67.77% <ø> (-0.02%) ⬇️
topo 66.49% <ø> (-0.02%) ⬇️
unit 32.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
daq/varz_state_collector.py 46.98% <0.00%> (-3.62%) ⬇️
daq/traffic_analyzer.py 86.50% <0.00%> (-3.18%) ⬇️
daq/stream_monitor.py 87.60% <0.00%> (-2.48%) ⬇️
daq/tcpdump_helper.py 78.57% <0.00%> (-2.39%) ⬇️
daq/host.py 90.92% <0.00%> (-0.44%) ⬇️

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 4432a4f...5de5e92. Read the comment docs.

@anurag6 anurag6 requested review from grafnu and henry54809 November 29, 2021 23:30
@@ -12,6 +12,6 @@ bin/setup_remote forch

echo Starting $image_name build...

docker build . -f docker/modules/Dockerfile.daq --build-arg GIT_URL -t daqf/$image_name
docker build . -f docker/modules/Dockerfile.daq -t daqf/$image_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

why remove this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought that arg wasn't getting consumed anywhere. Is that a mistake?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I forget exactly how it works, but it sets an environment variable which is consumed somewhere down the chain of things. Docs seem to imply that it also requires an ENV setting in the Dockerfile, so not sure if it works without that or if it's broken.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair enough. I reverted that particular bit. Thought I was cleaning up something. Better not to break things.



class OvsHelper:
"""Class to build OVS, VxLANs and other network components"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think "build OVS" is what you mean here -- that implies actually compiling OVS (which is a thing). Do you mean something like construct an OVS bridge?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I meant OVS network components but updated it.

def delete_ovs_bridge(self, name):
"""Delete ovs bridge"""
self._logger.info('Deleting OVS bridge %s', name)
self._run_shell_no_raise('sudo ovs-vsctl add-br %s' % name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be for del br

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup. Updated.

vlan_str = ",".join(str(vlan) for vlan in vlans)
self._run_shell('sudo ovs-vsctl set port %s trunks=%s' % (interface, vlan_str))

def create_faux_device(self, index):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we going to refactor cmd/faux to python?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably a good idea long term, but I am not working on it atm. I just need the faux as a testing tool and simply running it as a shell script works for me.

@anurag6 anurag6 merged commit 052db4b into faucetsdn:master Nov 30, 2021
@anurag6 anurag6 deleted the daq_client branch November 30, 2021 19:53
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.

3 participants