Normalize DPDK port parameter names #20
Conversation
User interface changes: - Renamed 'host' leaf to 'host-name'. 'host' is ambiguous; the value we are setting is the host *name*. - Renamed 'hotplug' leaf to 'qemu-hotplug-mode'. 'hotplug' is ambiguous; the value we are setting is the QEMU hotplug *mode*. - Renamed 'qemu-vm-mac' leaf to 'qemu-vm-mac-address'. The singleton port mac address leaf is named 'mac-address'. Leaf names should be consistent. Internal changes: - Renamed SWBackend protobuf definitions, changing the prefix to 'Dpdk' in some cases and removing it entirely in others. - Renamed protobuf fields to match the leaf name: 'mempool_name' instead of 'mempool', 'port_type' instead of 'type', etc. Other changes: - Disabled the ColdbootSetupFailureWhenPipelineConfigPushFailsForSomeNodes test case in dpdk_hal_test. This test will always fail for DPDK because we require that the forwarding pipeline configuration file be empty on startup, which causes DpdkSwitch::PushChassisConfig to return OK instead of an error. - Modified DpdkSwitch::RetrieveValue to return ERR_UNIMPLEMENTED instead of passing port parameters that DPDK does not support (kPortSpeed, etc.) to the chassis manager. - Corrected the temporary directory name used by DPDK unit tests from 'ipdk' to 'dpdk'. - Corrected grammar errors in a number of comments. Outstanding issues: - The Teardown tests in dpdk_hal_test are currently failing for reasons that are not immediately clear. Signed-off-by: Derek G Foster <derek.foster@intel.com>
| PARAM_SOCK_PORT = 2; | ||
| PARAM_HOTPLUG = 3; | ||
| PARAM_HOTPLUG_MODE = 3; | ||
| PARAM_VM_MAC = 4; |
There was a problem hiding this comment.
Or MAC_ADDRESS, to match the parameter name.
If we make the hotplug params ordinary port params, we can get rid of this enumeration entirely.
|
|
||
| node = tree->AddNode(GetPath("interfaces")( | ||
| "virtual-interface", name)("config")("qemu-vm-mac")()); | ||
| "virtual-interface", name)("config")("qemu-vm-mac-address")()); |
There was a problem hiding this comment.
These changes will require modifications in gnmi-ctl cmdline use to hotplug the port, so we need to change the documentation and also notify Validation
There was a problem hiding this comment.
We'll need to notify Validation about all three changes. That's one of the reasons I want to make the change now. It gives Validation more time to update their tests.
| << " qemu_vm_device_id=" << hotplug_attrs->qemu_vm_device_id | ||
| << " native_socket_path=" << hotplug_attrs->native_socket_path | ||
| << " qemu_hotplug = " << hotplug_attrs->qemu_hotplug; | ||
| << " qemu_hotplug_mode = " << hotplug_attrs->qemu_hotplug; |
There was a problem hiding this comment.
As per hotplug_attrs->qemu_hotplug = (hotplug_config.qemu_hotplug_mode != 0), hotplug_attrs->qemu_hotplug will be true in case of ADD/Del and on line 146 instead of printing mode ADD/DEL, we will be printing true
There was a problem hiding this comment.
Oops. I shouldn't have changed the name on the left-hand side.
Otherwise, you're correct, and that's just fine. The log message displays the value the SDK uses, not the value Stratum uses.
There was a problem hiding this comment.
Since stratum makes the decision of call appropriate hotplug API, bf_pal_hotplug_add for HOTPLUG_MODE_ADD and bf_pal_hotplug_del for HOTPLUG_MODE_ADD, maybe we shoudl consider removing qemu_hotplug parameter from hotplug_attributes_t structure
There was a problem hiding this comment.
Good point. Something to consider doing after this release.
| NO_HOTPLUG = 0; | ||
| HOTPLUG_ADD = 1; | ||
| HOTPLUG_DEL = 2; | ||
| enum QemuHotplugMode { |
There was a problem hiding this comment.
I'm not convinced that this parameter is idiomatically correct. It's a command, not a mode or state. A more conventional interface might be a read/write Boolean hotplug-enabled (true or false) plus a read-only hotplug-state (e.g INACTIVE, CONFIGURING, ACTIVE, and one or two error states).
Fix inaccurate change to logging message.
IPsec changes to address comments previously raised
User interface changes:
Renamed
hostleaf tohost-name.hostis ambiguous; the value we are setting is the host name.Renamed
hotplugleaf toqemu-hotplug-mode.hotplugis ambiguous; the value we are setting is the QEMU hotplug mode.Renamed
qemu-vm-macleaf toqemu-vm-mac-address. The singleton port mac address leaf is namedmac-address. Leaf names should be consistent.Internal changes:
Renamed
SWBackendprotobuf definitions, changing the prefix toDpdkin some cases and removing it entirely in others.Renamed protobuf fields to match the leaf name:
mempool_nameinstead ofmempool,port_typeinstead oftype, etc.Other changes:
Disabled the
ColdbootSetupFailureWhenPipelineConfigPushFailsForSomeNodestest case in dpdk_hal_test. This test will always fail for DPDK because we require that the forwarding pipeline configuration file be empty on startup, which causes DpdkSwitch::PushChassisConfig to return OK instead of an error.Modified DpdkSwitch::RetrieveValue to return ERR_UNIMPLEMENTED instead of passing port parameters that DPDK does not support (kPortSpeed, etc.) to the chassis manager.
Corrected the temporary directory name used by DPDK unit tests from
ipdktodpdk.Corrected grammar errors in a number of comments.
Outstanding issues:
Signed-off-by: Derek G Foster derek.foster@intel.com