Skip to content

[CLEARWATER-SP1] Changes to enable contrailvrouter as a networking option in XenServer. #1780

Closed
wants to merge 3 commits into from

7 participants

@rranjeet
rranjeet commented Jun 4, 2014

Changes to enable contrailvrouter as a networking option in XenServer.

  • Changes to xcp-networkd and xapi daemons to accept contrailvrouter as a networking option in the /etc/xensource/network.conf file
  • New module ContrailVR to handle commands that are received from XAPI daemon and pass it on to the corresponding Contrail Vrouter control module.
  • Changes to script to change network backend.
  • Changes in VIF script to handle contrailvrouter to add, remove, online, offline VIFs
  • contrail-vrouter-ctl which is the control component for the Contrail Vrouter is not part of this checkin and will be installed as part of the Contrail VRouter RPM.

Note: Please discard first two commits since it is a commit and a revert.

rranjeet added some commits Jun 3, 2014
@rranjeet rranjeet Changes to support Contrail VRouter as a forwarding data path option
in Xen (similar to OpenVswitch).

The changes include
* Adding a new network option to the /etc/xensource/network.conf file
  (contrailvrouter)
* Changes to xcp-networkd, xapi daemon to support contrailvrouter
* Module to handle calls to the Contrail Vrouter from XAPI
* Changes to vif script to handle calls if the network option is
  contrailvrouter
* The control component of Contrail VRouter (contrail-vrouter-ctl)
  is not part of the change and will be installed as part of the
  Contrail Vrouter RPM for XenServer.
* Changes also include to add ContrailVRouter as an option in the
  netman library.
7f58e91
@rranjeet rranjeet Revert "Changes to support Contrail VRouter as a forwarding data path…
… option"

This reverts commit 7f58e91.
b7dde45
@rranjeet rranjeet Changes to enable contrailvrouter as a networking option in
XenServer.

* Changes to xcp-networkd and xapi daemons to accept
  contrailvrouter as a networking option in the
  /etc/xensource/network.conf file
* New module ContrailVR to handle commands that are
  received from XAPI daemon and pass it on to the
  corresponding Contrail Vrouter control module.
* Changes to script to change network backend.
* Changes in VIF script to handle contrailvrouter
  to add, remove, online, offline VIFs
d4e0847
@xen-git
Xapi Project member
xen-git commented Jun 4, 2014

Can one of the admins verify this patch for testing?

@akshayramani

test this please.

@simonjbeaumont
Xapi Project member

HI @rranjeet:

Note: Please discard first two commits since it is a commit and a revert.

If you rebase your branch and remove these commits (do a rebase -i master) and comment out the two commits you do not want. Then you can force push to your branch (rranjeet:clearwater-sp1-lcm) and this pull request will be automatically updated.

@simonjbeaumont
Xapi Project member

Note to team: It also looks like this pull request is not expected to build until xapi-project/xen-api-libs#160 is merged.

@simonjbeaumont simonjbeaumont changed the title from Clearwater sp1 lcm to [CLEARWATER-SP1] Changes to enable contrailvrouter as a networking option in XenServer. Jun 12, 2014
@robhoes
Xapi Project member
robhoes commented Jul 8, 2014

The InterfaceReconfigure.py scripts are not used anymore since XenServer 6.1. They are still present, but use only during an upgrade from pre-6.1 XenServers. Since those older XenServers wouldn't have VRouter setup, the changes to scripts/InterfaceReconfigure.py are not actually needed.

@robhoes robhoes commented on the diff Jul 9, 2014
ocaml/network/network_server.ml
@@ -627,6 +648,11 @@ module Bridge = struct
warn "Could not add default flows for port %s on bridge %s because no MAC address was specified"
name bridge
end
+ | Contrailvrouter ->
+ if List.length interfaces = 1 then begin
+ List.iter (fun name -> Interface.bring_up () dbg ~name) interfaces;
+ ignore (ContrailVR.create_port (List.hd interfaces) bridge)
+ end
@robhoes
Xapi Project member
robhoes added a note Jul 9, 2014

It seems that Contrail does not support bonding (the case where there are more than one interfaces), is that correct? A comment in the source that explains this would be useful.

@robhoes
Xapi Project member
robhoes added a note Jul 9, 2014

It may be best to raise Not_implemented here in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhoes robhoes commented on the diff Jul 9, 2014
ocaml/network/network_server.ml
@@ -460,6 +463,16 @@ module Bridge = struct
in
ignore (Ovs.create_bridge ?mac ~fail_mode ?external_id ?disable_in_band
vlan vlan_bug_workaround name)
+ | Contrailvrouter ->
+ let external_id =
+ if List.mem_assoc "network-uuids" other_config then
+ Some ("xs-network-uuids", List.assoc "network-uuids" other_config)
+ else
+ None
+ in
+ ignore (ContrailVR.create_bridge ?mac ?external_id name)
+
+
@robhoes
Xapi Project member
robhoes added a note Jul 9, 2014

Also VLANs are not handled here; are they not supported by the Contrail VRouter? Or is the intention to add this later? Again, it would be best to raise Not_implemented (if vlan <> None).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhoes robhoes commented on the diff Jul 9, 2014
ocaml/network/network_utils.ml
@@ -786,6 +787,56 @@ module Ovs = struct
List.iter (fun flow -> ignore (ofctl ~log:true ["add-flow"; bridge; flow])) flows
end
+module ContrailVR = struct
+ let vrctl ?(log=false) args =
+ call_script ~log_successful_output:log contrail_vrctl args
+
+ let bridge_to_interfaces name =
+ try
+ let ifaces = String.rtrim (vrctl ["list-ifaces"; name]) in
+ if ifaces <> "" then
+ String.split '\n' ifaces
+ else
+ []
+ with _ -> []
@robhoes
Xapi Project member
robhoes added a note Jul 9, 2014

This line seems to have a tab too many.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhoes robhoes commented on the diff Jul 9, 2014
scripts/vif
@@ -218,7 +238,82 @@ call_hook_script() {
fi
}
-NETWORK_MODE=$(cat @ETCDIR@/network.conf)
@robhoes
Xapi Project member
robhoes added a note Jul 9, 2014

The patch changes this line with a hardcoded @ETCDIR@.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhoes robhoes commented on the diff Jul 9, 2014
scripts/vif
- logger -t script-vif "${dev}: writing ${HOTPLUG_STATUS}=connected"
- xenstore-write "${HOTPLUG_STATUS}" "connected"
- call_hook_script $DOMID "${ACTION}"
- fi
+ case $NETWORK_MODE in
+ bridge|openvswitch)
+ if [ "${TYPE}" = "vif" ] ; then
+ handle_ethtool rx
+ handle_ethtool tx
+ handle_ethtool sg
+ handle_ethtool tso
+ handle_ethtool ufo
+ handle_ethtool gso
+
+ handle_mtu
+ add_to_bridge
@robhoes
Xapi Project member
robhoes added a note Jul 9, 2014

I'm a little suspicious about leaving out the handle_* functions in the contrail case. Also, the hook script below must be called in any case.

Also, to keep the contrail case more in line with the other two backends, it would be better to do any special casing in the add_to_bridge function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhoes robhoes commented on the diff Jul 9, 2014
scripts/vif
+
+ handle_mtu
+ add_to_bridge
+ handle_promiscuous
+
+ # only for the benefit of xenrt test case, see CA-61528
+ xenstore-write "${HOTPLUG}/vif" "${dev}"
+ xenstore-write "${HOTPLUG}/hotplug" "online"
+
+ logger -t script-vif "${dev}: writing ${HOTPLUG_STATUS}=connected"
+ xenstore-write "${HOTPLUG_STATUS}" "connected"
+ call_hook_script $DOMID "${ACTION}"
+ fi
+ ;;
+
+ contrailvrouter)
@robhoes
Xapi Project member
robhoes added a note Jul 9, 2014

No interface is actually added to a bridge here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhoes robhoes commented on the diff Jul 9, 2014
scripts/vif
;;
add)
- if [ "${TYPE}" = "tap" ] ; then
- add_to_bridge
- fi
+ case $NETWORK_MODE in
+ bridge|openvswitch)
+ if [ "${TYPE}" = "tap" ] ; then
+ add_to_bridge
+ fi
+ ;;
+
+ contrailvrouter)
+ # Try to determine whether this is an HVM or PV machine.
+ # HVMs use qemu tap interfaces to send and receive packets.
+ isHVM
@robhoes
Xapi Project member
robhoes added a note Jul 9, 2014

Why is this needed for contrail and not for the other backends?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhoes robhoes commented on the diff Jul 9, 2014
scripts/vif
+ if [ "${TYPE}" = "tap" ] ; then
+ add_to_bridge
+ fi
+ ;;
+
+ contrailvrouter)
+ # Try to determine whether this is an HVM or PV machine.
+ # HVMs use qemu tap interfaces to send and receive packets.
+ isHVM
+ if [ $? -gt 0 -a "${TYPE}" == "vif" ]; then
+ logger -t scripts-vif "HVM ${dev} unused"
+ else
+ add_vm_interface ${dev}
+ fi
+
+ ${IP} link set ${dev} up
@robhoes
Xapi Project member
robhoes added a note Jul 9, 2014

This is done by setup_vif_rules and should not be here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhoes robhoes commented on the diff Jul 9, 2014
scripts/vif
- # Trigger xapi to clean up:
- xenstore-rm "${HOTPLUG}/hotplug"
- fi
- logger -t scripts-vif "${dev} has been removed"
- remove_from_bridge
+ logger -t script-vif "${dev}: removing ${HOTPLUG_STATUS}"
+ xenstore-rm "${HOTPLUG_STATUS}"
+ logger -t script-vif "${dev}: removing ${HOTPLUG}/hotplug"
+ # Trigger xapi to clean up:
+ xenstore-rm "${HOTPLUG}/hotplug"
+ fi
+ logger -t scripts-vif "${dev} has been removed"
+ remove_from_bridge
+ ;;
+
+ contrailvrouter)
@robhoes
Xapi Project member
robhoes added a note Jul 9, 2014

As above, I don't think the case distinction is in the right place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhoes
Xapi Project member
robhoes commented Jul 9, 2014

Oh, and just like for the xen-api-libs PR, it would be good to rebase this onto the xs64bit branch.

@robhoes robhoes commented on the diff Jul 10, 2014
scripts/vif
@@ -83,6 +86,23 @@ set_vif_external_id()
echo "-- set interface vif${DOMID}.${DEVID} external-ids:\"${key}\"=\"${value}\""
}
+#
+# Return true if this is an hardware vitualized VM.
+#
+isHVM()
+{
+ local vmpath=$(xenstore-read /local/domain/${DOMID}/vm 2>/dev/null)
+ local vmid=$(basename ${vmpath})
+ if [ -z "${vmid}" ]; then
+ return 0
+ fi
+ local pvboot=$(xe vm-param-get uuid=${vmid} param-name=PV-bootloader 2>/dev/null)
@robhoes
Xapi Project member
robhoes added a note Jul 10, 2014

For performance reasons, this hotplug script should not contain any xe calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhoes robhoes commented on the diff Jul 10, 2014
scripts/vif
+ local address=$(xenstore-read "${PRIVATE}/bridge-MAC")
+ if [ $? -ne 0 -o -z "${address}" ]; then
+ logger -t scripts-vif "Failed to read ${PRIVATE}/bridge-MAC from xenstore"
+ xenstore-write "${HOTPLUG_ERROR}" "Failed to read ${PRIVATE}/bridge-MAC from xenstore"
+ exit 1
+ fi
+
+ logger -t scripts-vif "Adding ${dev} with address ${address}"
+
+ local vif_uuid=$(xenstore-read ${PRIVATE}/vif-uuid 2>/dev/null)
+ if [ -z "${vif_uuid}" ]; then
+ logger -t scripts-vif "Failed to read vif-uuid"
+ exit 1
+ fi
+
+ local config=$(xe vif-param-get uuid=${vif_uuid} param-name=other-config 2>/dev/null)
@robhoes
Xapi Project member
robhoes added a note Jul 10, 2014

As above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robhoes robhoes commented on the diff Jul 10, 2014
scripts/vif
+ logger -t scripts-vif "add ${dev} succeeded"
+ else
+ logger -t scripts-vif "add ${dev} failure"
+ fi
+
+}
+
+del_vm_interface()
+{
+ local vif_uuid=$(xenstore-read ${PRIVATE}/vif-uuid 2>/dev/null)
+ if [ -z "${vif_uuid}" ]; then
+ logger -t scripts-vif "Failed to read vif-uuid"
+ exit 1
+ fi
+
+ local config=$(xe vif-param-get uuid=${vif_uuid} param-name=other-config 2>/dev/null)
@robhoes
Xapi Project member
robhoes added a note Jul 10, 2014

As above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@johnelse johnelse added the For hotfix label Nov 27, 2014
@xen-git
Xapi Project member
xen-git commented Feb 26, 2015

Can one of the admins verify this patch for testing?

@simonjbeaumont
Xapi Project member

Due to inactivity we're going to close this. Please reopen if required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.