Feature/usi OVS switch #521
Conversation
Codecov Report
@@ Coverage Diff @@
## master #521 +/- ##
==========================================
- Coverage 80.36% 79.81% -0.56%
==========================================
Files 21 21
Lines 3621 3621
==========================================
- Hits 2910 2890 -20
- Misses 711 731 +20
Continue to review full report at Codecov.
|
|
Renamed things. PTAL |
|
Can you please be more specific? File & line? "DAQ" doesn't help very
much...
…On Thu, Jul 2, 2020 at 9:08 AM henry54809 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In usi/src/main/proto/usi.proto
<#521 (comment)>:
> @@ -54,6 +54,15 @@ enum POEStatus {
DENY = 3;
}
+message SwitchInput {
+ // Device Port
+ int32 device_port = 1;
+ oneof input {
+ SwitchInfo switch_info = 2;
rpc call is constructed in DAQ in the other PR.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#521 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPD7VNSDKZDEU5GSM4ZLRZSWJPANCNFSM4OO4IKNQ>
.
|
|
If they aren't at all similar then this abstraction is broken and not the
right one. Either way, bleeding the faux in the abstraction is a massive
code smell (stink even), so I want to get to the bottom of it. I'm not at
all worried about performance.
…On Thu, Jul 2, 2020 at 9:05 AM henry54809 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In usi/src/main/java/daq/usi/UsiImpl.java
<#521 (comment)>:
> @@ -19,7 +21,11 @@ public UsiImpl() {
switchControllers = new HashMap<>();
}
- private SwitchController getSwitchController(SwitchInfo switchInfo) {
+ private ISwitchController getSwitchController(SwitchInput switchInput) {
+ if (!switchInput.hasSwitchInfo()) {
The 2 cases (with a physical switch and ovs) aren't at all similar which
was why I had to make so many changes for ovs case to work. Only is faux
interface is needed to be passed for the port connect and disconnect. As
faux interface is a string(and not a device port), it doesn't match the
existing signatures for all of the service functions. I decided to just
pass the faux interface separately from the existing switchInfo and pass
that information during object instantiation(the class is completely static
so it hurt performance). The faux interface should be coming from DAQ; usi
shouldn't take up the responsibility of parsing the config file.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#521 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPD5VC7ZDLMN37DB54GDRZSV4RANCNFSM4OO4IKNQ>
.
|
|
grpc call in DAQ in my other PR: the proto has changed in this PR so the implementation won't match proto 100%.
|
|
I don't understand this assertion "since DAQ needs to explicitly pass
faux_interface for OVS switch" -- why does it need to do this? Maybe
highlight the other end -- where is the bit of code that absolutely *needs*
this value?
Also, the reference you pointed me at has no indication of faux_interface
so it's still not helping answer the question.
…On Thu, Jul 2, 2020 at 10:00 AM henry54809 ***@***.***> wrote:
If they aren't at all similar then this abstraction is broken and not the
right one. Either way, bleeding the faux in the abstraction is a massive
code smell (stink even), so I want to get to the bottom of it. I'm not at
all worried about performance.
… <#m_8332880915667001470_>
On Thu, Jul 2, 2020 at 9:05 AM henry54809 *@*.*> wrote: @.** commented on
this pull request. ------------------------------ In
usi/src/main/java/daq/usi/UsiImpl.java <#521 (comment)
<#521 (comment)>>: > @@
-19,7 +21,11 @@ public UsiImpl() { switchControllers = new HashMap<>(); } -
private SwitchController getSwitchController(SwitchInfo switchInfo) { +
private ISwitchController getSwitchController(SwitchInput switchInput) { +
if (!switchInput.hasSwitchInfo()) { The 2 cases (with a physical switch and
ovs) aren't at all similar which was why I had to make so many changes for
ovs case to work. Only is faux interface is needed to be passed for the
port connect and disconnect. As faux interface is a string(and not a device
port), it doesn't match the existing signatures for all of the service
functions. I decided to just pass the faux interface separately from the
existing switchInfo and pass that information during object
instantiation(the class is completely static so it hurt performance). The
faux interface should be coming from DAQ; usi shouldn't take up the
responsibility of parsing the config file. — You are receiving this because
your review was requested. Reply to this email directly, view it on GitHub <#521
(comment)
<#521 (comment)>>, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AAIEPD5VC7ZDLMN37DB54GDRZSV4RANCNFSM4OO4IKNQ
.
grpc call in DAQ in my other PR: the proto has changed in this PR so the
implementation won't match proto 100%.
https://github.com/faucetsdn/daq/pull/520/files#r449149359
My thinking was, since DAQ needs to explicitly pass faux_interface for OVS
switch, the abstraction for faux switch was not going to work. Another
thing I could do, which I was hesitant of, was reusing device_port to send
the faux_interface instead of the device_port. That will keep the original
SwitchInfo proto and original abstraction.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#521 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPD3LQEQ5O25B2AZA6FLRZS4J7ANCNFSM4OO4IKNQ>
.
|
|
PTAL |
| protected boolean commandPending = false; | ||
|
|
||
| public BaseSwitchController(String remoteIpAddress, int telnetPort, String username, | ||
| String password) { |
There was a problem hiding this comment.
check your style settings -- this should be hanging indent 4. Did it pass stickler/lint?
There was a problem hiding this comment.
It passes stickler.mostly likely it means that rule is not configured.
There was a problem hiding this comment.
Actually intellij won't even let me move this line to a less indented position. Is this a java syntax issue?
| if (!switchInput.hasSwitchInfo()) { | ||
| return new OpenVSwitch(switchInput.getFauxInterface()); | ||
| private SwitchController getSwitchController(SwitchInfo switchInfo) { | ||
| if (switchInfo.getModel().equals(OVS_SWITCH)) { |
There was a problem hiding this comment.
Still don't understand why OVS is special cased -- why isn't it just a generic drop-in replacement? (If we discussed, can you please summarize the reason)
There was a problem hiding this comment.
OVS doesn’t need to be stored, doesn’t implement the runnable interface, so it needs to be handled first.
| @@ -74,12 +66,15 @@ message SwitchInfo { | |||
| // Telnet Port | |||
| int32 telnet_port = 2; | |||
|
|
|||
| // Device Port | |||
There was a problem hiding this comment.
for above -- telnet_port should be changed to cmd_port... ?
|
Is there a reason OVS can't be handled the same way as the others? It
doesn't *need* to store anything, I'm sure it can implement the runnable
interface as a no-op. I don't get why it needs to be handled "first"
because there's only one at a time. A generic implementation w/ no special
case will be better overall even if it means a little bit extra in the
implementing class. You're answering the question of if it *can* be
special cased, rather than if it *needs* to be special cased.
…On Mon, Jul 6, 2020 at 7:04 PM henry54809 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In usi/src/main/java/daq/usi/UsiImpl.java
<#521 (comment)>:
>
public UsiImpl() {
super();
switchControllers = new HashMap<>();
}
- private ISwitchController getSwitchController(SwitchInput switchInput) {
- if (!switchInput.hasSwitchInfo()) {
- return new OpenVSwitch(switchInput.getFauxInterface());
+ private SwitchController getSwitchController(SwitchInfo switchInfo) {
+ if (switchInfo.getModel().equals(OVS_SWITCH)) {
OVS doesn’t need to be stored, doesn’t implement the runnable interface,
so it needs to be handled first.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#521 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPD4VDVDGCUD6YDN3RVTR2J7C5ANCNFSM4OO4IKNQ>
.
|
|
I initially tried what you're proposing but I didn't like the end result. The OVS class has mostly empty override methods which makes the inheritance model a pretty bad fit in my opinion. This is why I created the current switchcontroller interface to mitigate this even though it adds the specific ovs model check. PTAL of telnet_port renaming
|
|
I'm not saying that OpenVSwitch should *inherit *from anything, just that
it's creation shouldn't be special cased (if statement rather than case
statement). switchControllers should map to a SwitchController not
BaseSwitchController, then you can put the OVS_SWITCH check into the case
statement, not a special-case if. All the constructors should just take a
"SwitchInfo" argument rather than explicitly enumerating all the options,
and then also put the Thread.start() inside of the classes that need it, or
create a separate .start() method on them if necessary if it doesn't want
to live inside the constructor, then you can also use computerIfAbsent.
Something like below -- but I'd also extract the case statement to a
function rather than inline (then you can ditch the intermediate variable
and break statements and just do return inline, and then don't abbreviate
*sc*.
private SwitchController getSwitchController(SwitchInfo switchInfo) {
String repr = String.join(",", switchInfo.getModel().toString(),
switchInfo.getIpAddr(),
String.valueOf(switchInfo.getCmdTelnetPort()), switchInfo.getUsername(),
switchInfo.getPassword());
SwitchController sc = switchControllers.computeIfAbsent(repr, key -> {
final SwitchController newController;
switch (switchInfo.getModel()) {
case ALLIED_TELESIS_X230: {
newController = new AlliedTelesisX230(switchInfo);
break;
}
case CISCO_9300: {
newController = new Cisco9300(switchInfo);
break;
}
case OVS_SWITCH: {
newController = new OpenVSwitch(switchInfo);
break;
}
default:
throw IllegalArgumentException("Unrecognized switch model " +
switchInfo.getModel());
}
newController.start();
return newController;
});
return sc;
}
…On Mon, Jul 6, 2020 at 10:37 PM henry54809 ***@***.***> wrote:
I initially tried what you're proposing but I didn't like the end result.
The OVS class has mostly empty override methods which makes the inheritance
model a pretty bad fit in my opinion. This is why I created the current
switchcontroller interface to mitigate this even though it adds the
specific ovs model check. PTAL of telnet_port renaming
Is there a reason OVS can't be handled the same way as the others? It
doesn't *need* to store anything, I'm sure it can implement the runnable
interface as a no-op. I don't get why it needs to be handled "first"
because there's only one at a time. A generic implementation w/ no special
case will be better overall even if it means a little bit extra in the
implementing class. You're answering the question of if it *can* be
special cased, rather than if it *needs* to be special cased.
… <#m_3058283750408108678_>
On Mon, Jul 6, 2020 at 7:04 PM henry54809 *@*.*> wrote: @.** commented on
this pull request. ------------------------------ In
usi/src/main/java/daq/usi/UsiImpl.java <#521 (comment)
<#521 (comment)>>: >
public UsiImpl() { super(); switchControllers = new HashMap<>(); } -
private ISwitchController getSwitchController(SwitchInput switchInput) { -
if (!switchInput.hasSwitchInfo()) { - return new
OpenVSwitch(switchInput.getFauxInterface()); + private SwitchController
getSwitchController(SwitchInfo switchInfo) { + if
(switchInfo.getModel().equals(OVS_SWITCH)) { OVS doesn’t need to be stored,
doesn’t implement the runnable interface, so it needs to be handled first.
— You are receiving this because your review was requested. Reply to this
email directly, view it on GitHub <#521 (comment)
<#521 (comment)>>, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AAIEPD4VDVDGCUD6YDN3RVTR2J7C5ANCNFSM4OO4IKNQ
.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#521 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPD45PGSKV2LJSLTZU33R2KYAXANCNFSM4OO4IKNQ>
.
|
|
Works for me... maybe something is still misconfigured about your IntelliJ
settings?
…On Mon, Jul 6, 2020 at 10:28 PM henry54809 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In usi/src/main/java/daq/usi/BaseSwitchController.java
<#521 (comment)>:
> +
+ // Define Common Variables Required for All Switch Interrogators
+ protected SwitchTelnetClientSocket telnetClientSocket;
+ protected Thread telnetClientSocketThread;
+ protected String remoteIpAddress;
+ protected int telnetPort;
+ protected boolean debug;
+ protected String username;
+ protected String password;
+ protected boolean userAuthorised = false;
+ protected boolean userEnabled = false;
+ protected String hostname = null;
+ protected boolean commandPending = false;
+
+ public BaseSwitchController(String remoteIpAddress, int telnetPort, String username,
+ String password) {
Actually intellij won't even let me move this line to a less indented
position. Is this a java syntax issue?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#521 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPD7PGMQTES6BEAJHC43R2KXAFANCNFSM4OO4IKNQ>
.
|
usi/src/main/proto/usi.proto
Outdated
| @@ -63,8 +63,8 @@ message SwitchInfo { | |||
| // IP address of external switch. | |||
| string ip_addr = 1; | |||
|
|
|||
| // Telnet Port | |||
| int32 telnet_port = 2; | |||
| // telnet port for sending switch commands. | |||
There was a problem hiding this comment.
drop the "telnet" -- just cmd_port -- no need to specify the protocol here.
There was a problem hiding this comment.
Puneet and I both think cmd_port is too ambiguous. cmd_port can be construed as the openflow port or the switch control port. Too many things are named ports in the current configuration where some ports are for physical ports and others are for tcp ports.
|
If you think cmd_port is too ambiguous then adding telnet isn't the right
solution.
…On Tue, Jul 7, 2020 at 7:53 AM henry54809 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In usi/src/main/proto/usi.proto
<#521 (comment)>:
> @@ -63,8 +63,8 @@ message SwitchInfo {
// IP address of external switch.
string ip_addr = 1;
- // Telnet Port
- int32 telnet_port = 2;
+ // telnet port for sending switch commands.
Puneet and I both think cmd_port is too ambiguous. cmd_port can be
construed as the openflow port or the switch control port. Too many things
are named ports in the current configuration where some ports are for
physical ports and others are for tcp ports.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#521 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPD6EOVQRIOIHTINTU4TR2MZFJANCNFSM4OO4IKNQ>
.
|
|
I still don't understand why protocol should not be stated in the config. There are multiple ways of accessing the switch, telnet, ssh or serial. What if we need to support ssh in the future? Any generic cmd_port (or any other name) will still cause confusion.
|
|
As discussed on GVC, we don't need the parameter at all for the time being,
so we'll just leave it out and rely on defaults. We can revisit this option
later if necessary.
…On Tue, Jul 7, 2020 at 8:08 AM henry54809 ***@***.***> wrote:
I still don't understand why protocol should not be stated in the config.
There are multiple ways of accessing the switch, telnet, ssh or serial.
What if we need to support ssh in the future? Any generic cmd_port (or any
other name) will still cause confusion.
If you think cmd_port is too ambiguous then adding telnet isn't the right
solution.
… <#m_6276675179600774982_>
On Tue, Jul 7, 2020 at 7:53 AM henry54809 *@*.*> wrote: @.** commented on
this pull request. ------------------------------ In
usi/src/main/proto/usi.proto <#521 (comment)
<#521 (comment)>>: > @@
-63,8 +63,8 @@ message SwitchInfo { // IP address of external switch.
string ip_addr = 1; - // Telnet Port - int32 telnet_port = 2; + // telnet
port for sending switch commands. Puneet and I both think cmd_port is too
ambiguous. cmd_port can be construed as the openflow port or the switch
control port. Too many things are named ports in the current configuration
where some ports are for physical ports and others are for tcp ports. — You
are receiving this because your review was requested. Reply to this email
directly, view it on GitHub <#521 (comment)
<#521 (comment)>>, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AAIEPD6EOVQRIOIHTINTU4TR2MZFJANCNFSM4OO4IKNQ
.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#521 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPD4OFIGJFSMAUUMRRILR2M27HANCNFSM4OO4IKNQ>
.
|
|
removed telnet port configuration. PTAL. |
| BaseSwitchController sc = switchControllers.get(repr); | ||
| if (sc == null) { | ||
| SwitchController sc = switchControllers.computeIfAbsent(repr, key -> { | ||
| final SwitchController newController; |
There was a problem hiding this comment.
extract switch statement to function (createNewController or something like that) - lengthy embedded functions aren't very readable...
| switchInfo.getPassword()); | ||
| BaseSwitchController sc = switchControllers.get(repr); | ||
| if (sc == null) { | ||
| SwitchController sc = switchControllers.computeIfAbsent(repr, key -> { |
There was a problem hiding this comment.
don't abbreviate sc, or just remove entirely (can just return directly)
| throw new IllegalArgumentException("Unrecognized switch model " | ||
| + switchInfo.getModel()); | ||
| } | ||
| newController.start(); |
There was a problem hiding this comment.
nit -- start should not be in create. They're typically two separate phases... first you create but don't really "do" anything, then there's a start after. This allows, for example, changing options or other things before actually activating it. It's fine as is, just something to ponder. Conceptually, the "create" function is the same as the class constructor, only difference is it redirects off of the switch type (aka the factory pattern).
No description provided.