diff mbox

[ovs-dev,V2] ipfix: Export user specified virtual observation ID

Message ID 1466771157-48897-1-git-send-email-wenyuz@vmware.com
State Accepted
Headers show

Commit Message

Wenyu Zhang June 24, 2016, 12:25 p.m. UTC
In virtual network, users want more info about the virtual point to observe the traffic.
It should be a string to provide clear info, not a simple interger ID.

Introduce "other-config: virtual_obs_id" in IPFIX, which is a string configured by user.
Introduce an enterprise IPFIX entity "virtualObsID"(898) to export the value. The entity is a
variable-length string.

Signed-off-by: Wenyu Zhang <wenyuz@vmware.com>
---
 ofproto/ipfix-enterprise-entities.def |  1 +
 ofproto/ofproto-dpif-ipfix.c          | 96 +++++++++++++++++++++++++++++++----
 ofproto/ofproto.h                     |  2 +
 vswitchd/bridge.c                     | 11 ++++
 vswitchd/vswitch.xml                  | 34 +++++++++++++
 5 files changed, 134 insertions(+), 10 deletions(-)

Comments

Ben Pfaff June 24, 2016, 8:42 p.m. UTC | #1
On Fri, Jun 24, 2016 at 05:25:57AM -0700, Wenyu Zhang wrote:
> In virtual network, users want more info about the virtual point to observe the traffic.
> It should be a string to provide clear info, not a simple interger ID.
> 
> Introduce "other-config: virtual_obs_id" in IPFIX, which is a string configured by user.
> Introduce an enterprise IPFIX entity "virtualObsID"(898) to export the value. The entity is a
> variable-length string.
> 
> Signed-off-by: Wenyu Zhang <wenyuz@vmware.com>

Hi Wenyu.

I reverted this commit because it dumps core in test 1048 "ofproto-dpif
- Flow IPFIX sanity check" with the following backtrace.  It crashes
the same way whether I use your original commit or my modified one.

#0  hmap_first_with_hash (hmap=<optimized out>, hmap=<optimized out>, 
    hash=<optimized out>) at ../lib/hmap.h:328
#1  smap_find__ (smap=0x94, key=key@entry=0x817f7ab "virtual_obs_id", 
    key_len=14, hash=2537071222) at ../lib/smap.c:366
#2  0x0812b9d7 in smap_get_node (smap=0x9738a276, 
    key=0x817f7ab "virtual_obs_id") at ../lib/smap.c:198
#3  0x0812ba30 in smap_get (smap=0x94, key=0x817f7ab "virtual_obs_id")
    at ../lib/smap.c:189
#4  0x08055a60 in bridge_configure_ipfix (br=<optimized out>)
    at ../vswitchd/bridge.c:1237
#5  bridge_reconfigure (ovs_cfg=0x94) at ../vswitchd/bridge.c:666
#6  0x080568d3 in bridge_run () at ../vswitchd/bridge.c:2972
#7  0x0804c9dd in main (argc=10, argv=0xffd8b934)
    at ../vswitchd/ovs-vswitchd.c:112

When you're ready, please post a fixed version.  Please start from the
version that I committed.

Before you post a patch, run the unit tests.
William Tu June 24, 2016, 8:54 p.m. UTC | #2
Hi Wenyu,

I was debugging a little bit and the issue is a NULL pointer deference
of be_cfg at   virtual_obs_id = smap_get(&be_cfg->other_config,
"virtual_obs_id");

Maybe adding if (valid_be_cfg) check before the deference? I will
leave you to fix it. Also I hope you can add a test case to this case.

Regards,
William


On Fri, Jun 24, 2016 at 1:42 PM, Ben Pfaff <blp@ovn.org> wrote:
> On Fri, Jun 24, 2016 at 05:25:57AM -0700, Wenyu Zhang wrote:
>> In virtual network, users want more info about the virtual point to observe the traffic.
>> It should be a string to provide clear info, not a simple interger ID.
>>
>> Introduce "other-config: virtual_obs_id" in IPFIX, which is a string configured by user.
>> Introduce an enterprise IPFIX entity "virtualObsID"(898) to export the value. The entity is a
>> variable-length string.
>>
>> Signed-off-by: Wenyu Zhang <wenyuz@vmware.com>
>
> Hi Wenyu.
>
> I reverted this commit because it dumps core in test 1048 "ofproto-dpif
> - Flow IPFIX sanity check" with the following backtrace.  It crashes
> the same way whether I use your original commit or my modified one.
>
> #0  hmap_first_with_hash (hmap=<optimized out>, hmap=<optimized out>,
>     hash=<optimized out>) at ../lib/hmap.h:328
> #1  smap_find__ (smap=0x94, key=key@entry=0x817f7ab "virtual_obs_id",
>     key_len=14, hash=2537071222) at ../lib/smap.c:366
> #2  0x0812b9d7 in smap_get_node (smap=0x9738a276,
>     key=0x817f7ab "virtual_obs_id") at ../lib/smap.c:198
> #3  0x0812ba30 in smap_get (smap=0x94, key=0x817f7ab "virtual_obs_id")
>     at ../lib/smap.c:189
> #4  0x08055a60 in bridge_configure_ipfix (br=<optimized out>)
>     at ../vswitchd/bridge.c:1237
> #5  bridge_reconfigure (ovs_cfg=0x94) at ../vswitchd/bridge.c:666
> #6  0x080568d3 in bridge_run () at ../vswitchd/bridge.c:2972
> #7  0x0804c9dd in main (argc=10, argv=0xffd8b934)
>     at ../vswitchd/ovs-vswitchd.c:112
>
> When you're ready, please post a fixed version.  Please start from the
> version that I committed.
>
> Before you post a patch, run the unit tests.
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
Wenyu Zhang June 24, 2016, 10:20 p.m. UTC | #3
Thanks for your catch.
Iam working on it.

发自我的 iPhone

> 在 2016年6月25日,上午4:55,William Tu <u9012063@gmail.com> 写道:

> 

> Hi Wenyu,

> 

> I was debugging a little bit and the issue is a NULL pointer deference

> of be_cfg at   virtual_obs_id = smap_get(&be_cfg->other_config,

> "virtual_obs_id");

> 

> Maybe adding if (valid_be_cfg) check before the deference? I will

> leave you to fix it. Also I hope you can add a test case to this case.

> 

> Regards,

> William

> 

> 

>> On Fri, Jun 24, 2016 at 1:42 PM, Ben Pfaff <blp@ovn.org> wrote:

>>> On Fri, Jun 24, 2016 at 05:25:57AM -0700, Wenyu Zhang wrote:

>>> In virtual network, users want more info about the virtual point to observe the traffic.

>>> It should be a string to provide clear info, not a simple interger ID.

>>> 

>>> Introduce "other-config: virtual_obs_id" in IPFIX, which is a string configured by user.

>>> Introduce an enterprise IPFIX entity "virtualObsID"(898) to export the value. The entity is a

>>> variable-length string.

>>> 

>>> Signed-off-by: Wenyu Zhang <wenyuz@vmware.com>

>> 

>> Hi Wenyu.

>> 

>> I reverted this commit because it dumps core in test 1048 "ofproto-dpif

>> - Flow IPFIX sanity check" with the following backtrace.  It crashes

>> the same way whether I use your original commit or my modified one.

>> 

>> #0  hmap_first_with_hash (hmap=<optimized out>, hmap=<optimized out>,

>>    hash=<optimized out>) at ../lib/hmap.h:328

>> #1  smap_find__ (smap=0x94, key=key@entry=0x817f7ab "virtual_obs_id",

>>    key_len=14, hash=2537071222) at ../lib/smap.c:366

>> #2  0x0812b9d7 in smap_get_node (smap=0x9738a276,

>>    key=0x817f7ab "virtual_obs_id") at ../lib/smap.c:198

>> #3  0x0812ba30 in smap_get (smap=0x94, key=0x817f7ab "virtual_obs_id")

>>    at ../lib/smap.c:189

>> #4  0x08055a60 in bridge_configure_ipfix (br=<optimized out>)

>>    at ../vswitchd/bridge.c:1237

>> #5  bridge_reconfigure (ovs_cfg=0x94) at ../vswitchd/bridge.c:666

>> #6  0x080568d3 in bridge_run () at ../vswitchd/bridge.c:2972

>> #7  0x0804c9dd in main (argc=10, argv=0xffd8b934)

>>    at ../vswitchd/ovs-vswitchd.c:112

>> 

>> When you're ready, please post a fixed version.  Please start from the

>> version that I committed.

>> 

>> Before you post a patch, run the unit tests.

>> _______________________________________________

>> dev mailing list

>> dev@openvswitch.org

>> https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailman_listinfo_dev&d=CwIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=MJBX2NeoFFE5o-wzhi1dy7zWKBNufpc6ky5q7EaQJBo&m=YK-ciEyP0lB9loxNsFHuW5TCW0xNKneVS3R9D-CunQo&s=GYVyJZTPKYoaidcS5nQES3WHzwOjvINyEuv41aVqlTg&e=
Wenyu Zhang June 24, 2016, 11 p.m. UTC | #4
Sorry for the stupid mistake.
I am working on it,and will verify it by running check.

发自我的 iPhone

> 在 2016年6月25日,上午4:43,Ben Pfaff <blp@ovn.org> 写道:

> 

>> On Fri, Jun 24, 2016 at 05:25:57AM -0700, Wenyu Zhang wrote:

>> In virtual network, users want more info about the virtual point to observe the traffic.

>> It should be a string to provide clear info, not a simple interger ID.

>> 

>> Introduce "other-config: virtual_obs_id" in IPFIX, which is a string configured by user.

>> Introduce an enterprise IPFIX entity "virtualObsID"(898) to export the value. The entity is a

>> variable-length string.

>> 

>> Signed-off-by: Wenyu Zhang <wenyuz@vmware.com>

> 

> Hi Wenyu.

> 

> I reverted this commit because it dumps core in test 1048 "ofproto-dpif

> - Flow IPFIX sanity check" with the following backtrace.  It crashes

> the same way whether I use your original commit or my modified one.

> 

> #0  hmap_first_with_hash (hmap=<optimized out>, hmap=<optimized out>, 

>    hash=<optimized out>) at ../lib/hmap.h:328

> #1  smap_find__ (smap=0x94, key=key@entry=0x817f7ab "virtual_obs_id", 

>    key_len=14, hash=2537071222) at ../lib/smap.c:366

> #2  0x0812b9d7 in smap_get_node (smap=0x9738a276, 

>    key=0x817f7ab "virtual_obs_id") at ../lib/smap.c:198

> #3  0x0812ba30 in smap_get (smap=0x94, key=0x817f7ab "virtual_obs_id")

>    at ../lib/smap.c:189

> #4  0x08055a60 in bridge_configure_ipfix (br=<optimized out>)

>    at ../vswitchd/bridge.c:1237

> #5  bridge_reconfigure (ovs_cfg=0x94) at ../vswitchd/bridge.c:666

> #6  0x080568d3 in bridge_run () at ../vswitchd/bridge.c:2972

> #7  0x0804c9dd in main (argc=10, argv=0xffd8b934)

>    at ../vswitchd/ovs-vswitchd.c:112

> 

> When you're ready, please post a fixed version.  Please start from the

> version that I committed.

> 

> Before you post a patch, run the unit tests.
Wenyu Zhang June 25, 2016, 12:54 a.m. UTC | #5
I have fixed the issue, and sent out a new patch. 

Due to the result of this feature only can been seen in the exported IPFIX packet, the test case for it may be a big work. We may consider it with other IPFIX features tests a litter later.





发自我的 iPhone
> 在 2016年6月25日,上午6:20,Wenyu Zhang <wenyuz@vmware.com> 写道:

> 

> Thanks for your catch.

> Iam working on it.

> 

> 发自我的 iPhone

> 

>> 在 2016年6月25日,上午4:55,William Tu <u9012063@gmail.com> 写道:

>> 

>> Hi Wenyu,

>> 

>> I was debugging a little bit and the issue is a NULL pointer deference

>> of be_cfg at   virtual_obs_id = smap_get(&be_cfg->other_config,

>> "virtual_obs_id");

>> 

>> Maybe adding if (valid_be_cfg) check before the deference? I will

>> leave you to fix it. Also I hope you can add a test case to this case.

>> 

>> Regards,

>> William

>> 

>> 

>>>> On Fri, Jun 24, 2016 at 1:42 PM, Ben Pfaff <blp@ovn.org> wrote:

>>>> On Fri, Jun 24, 2016 at 05:25:57AM -0700, Wenyu Zhang wrote:

>>>> In virtual network, users want more info about the virtual point to observe the traffic.

>>>> It should be a string to provide clear info, not a simple interger ID.

>>>> 

>>>> Introduce "other-config: virtual_obs_id" in IPFIX, which is a string configured by user.

>>>> Introduce an enterprise IPFIX entity "virtualObsID"(898) to export the value. The entity is a

>>>> variable-length string.

>>>> 

>>>> Signed-off-by: Wenyu Zhang <wenyuz@vmware.com>

>>> 

>>> Hi Wenyu.

>>> 

>>> I reverted this commit because it dumps core in test 1048 "ofproto-dpif

>>> - Flow IPFIX sanity check" with the following backtrace.  It crashes

>>> the same way whether I use your original commit or my modified one.

>>> 

>>> #0  hmap_first_with_hash (hmap=<optimized out>, hmap=<optimized out>,

>>>   hash=<optimized out>) at ../lib/hmap.h:328

>>> #1  smap_find__ (smap=0x94, key=key@entry=0x817f7ab "virtual_obs_id",

>>>   key_len=14, hash=2537071222) at ../lib/smap.c:366

>>> #2  0x0812b9d7 in smap_get_node (smap=0x9738a276,

>>>   key=0x817f7ab "virtual_obs_id") at ../lib/smap.c:198

>>> #3  0x0812ba30 in smap_get (smap=0x94, key=0x817f7ab "virtual_obs_id")

>>>   at ../lib/smap.c:189

>>> #4  0x08055a60 in bridge_configure_ipfix (br=<optimized out>)

>>>   at ../vswitchd/bridge.c:1237

>>> #5  bridge_reconfigure (ovs_cfg=0x94) at ../vswitchd/bridge.c:666

>>> #6  0x080568d3 in bridge_run () at ../vswitchd/bridge.c:2972

>>> #7  0x0804c9dd in main (argc=10, argv=0xffd8b934)

>>>   at ../vswitchd/ovs-vswitchd.c:112

>>> 

>>> When you're ready, please post a fixed version.  Please start from the

>>> version that I committed.

>>> 

>>> Before you post a patch, run the unit tests.

>>> _______________________________________________

>>> dev mailing list

>>> dev@openvswitch.org

>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailman_listinfo_dev&d=CwIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=MJBX2NeoFFE5o-wzhi1dy7zWKBNufpc6ky5q7EaQJBo&m=YK-ciEyP0lB9loxNsFHuW5TCW0xNKneVS3R9D-CunQo&s=GYVyJZTPKYoaidcS5nQES3WHzwOjvINyEuv41aVqlTg&e=
diff mbox

Patch

diff --git a/ofproto/ipfix-enterprise-entities.def b/ofproto/ipfix-enterprise-entities.def
index ea94586..73a520c 100644
--- a/ofproto/ipfix-enterprise-entities.def
+++ b/ofproto/ipfix-enterprise-entities.def
@@ -12,5 +12,6 @@  IPFIX_ENTERPRISE_ENTITY(TUNNEL_DESTINATION_IPV4_ADDRESS, 894, 4, tunnelDestinati
 IPFIX_ENTERPRISE_ENTITY(TUNNEL_PROTOCOL_IDENTIFIER, 895, 1, tunnelProtocolIdentifier, IPFIX_ENTERPRISE_VMWARE)
 IPFIX_ENTERPRISE_ENTITY(TUNNEL_SOURCE_TRANSPORT_PORT, 896, 2, tunnelSourceTransportPort, IPFIX_ENTERPRISE_VMWARE)
 IPFIX_ENTERPRISE_ENTITY(TUNNEL_DESTINATION_TRANSPORT_PORT, 897, 2, tunnelDestinationTransportPort, IPFIX_ENTERPRISE_VMWARE)
+IPFIX_ENTERPRISE_ENTITY(VIRTUAL_OBS_ID, 898, 0, virtualObsID, IPFIX_ENTERPRISE_VMWARE)
 
 #undef IPFIX_ENTERPRISE_ENTITY
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index f166014..81cd07c 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -101,6 +101,8 @@  struct dpif_ipfix_exporter {
     struct ovs_list cache_flow_start_timestamp_list;  /* ipfix_flow_cache_entry. */
     uint32_t cache_active_timeout;  /* In seconds. */
     uint32_t cache_max_flows;
+    char *virtual_obs_id;
+    uint8_t virtual_obs_len;
 
     ofproto_ipfix_stats stats;
 };
@@ -367,6 +369,32 @@  struct ipfix_data_record_aggregated_ip {
 BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_aggregated_ip) == 32);
 
 /*
+ * Refer to RFC 7011, the length of Variable length element is 0~65535:
+ * In most case, it should be less than 255 octets:
+ *  0                   1                   2                   3
+ *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  | Length (< 255)|          Information Element                  |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  |                      ... continuing as needed                 |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ * When it is greater than or equeal to 255 octets:
+ *  0                   1                   2                   3
+ *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  |      255      |      Length (0 to 65535)      |       IE      |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  |                      ... continuing as needed                 |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ *
+ * Now, only the virtual_obs_id whose length < 255 is implemented.
+ */
+
+#define IPFIX_VIRTUAL_OBS_MAX_LEN 254
+
+/*
  * support tunnel key for:
  * VxLAN: 24-bit VIN,
  * GRE: 32-bit key,
@@ -436,6 +464,12 @@  static void get_export_time_now(uint64_t *, uint32_t *);
 static void dpif_ipfix_cache_expire_now(struct dpif_ipfix_exporter *, bool);
 
 static bool
+nullable_string_is_equal(const char *a, const char *b)
+{
+    return a ? b && !strcmp(a, b) : !b;
+}
+
+static bool
 ofproto_ipfix_bridge_exporter_options_equal(
     const struct ofproto_ipfix_bridge_exporter_options *a,
     const struct ofproto_ipfix_bridge_exporter_options *b)
@@ -448,7 +482,8 @@  ofproto_ipfix_bridge_exporter_options_equal(
             && a->enable_tunnel_sampling == b->enable_tunnel_sampling
             && a->enable_input_sampling == b->enable_input_sampling
             && a->enable_output_sampling == b->enable_output_sampling
-            && sset_equals(&a->targets, &b->targets));
+            && sset_equals(&a->targets, &b->targets)
+            && nullable_string_is_equal(a->virtual_obs_id, b->virtual_obs_id));
 }
 
 static struct ofproto_ipfix_bridge_exporter_options *
@@ -458,6 +493,8 @@  ofproto_ipfix_bridge_exporter_options_clone(
     struct ofproto_ipfix_bridge_exporter_options *new =
         xmemdup(old, sizeof *old);
     sset_clone(&new->targets, &old->targets);
+    new->virtual_obs_id = old->virtual_obs_id ? xstrdup(old->virtual_obs_id)
+                                              : NULL;
     return new;
 }
 
@@ -467,6 +504,7 @@  ofproto_ipfix_bridge_exporter_options_destroy(
 {
     if (options) {
         sset_destroy(&options->targets);
+        free(options->virtual_obs_id);
         free(options);
     }
 }
@@ -480,7 +518,8 @@  ofproto_ipfix_flow_exporter_options_equal(
             && a->cache_active_timeout == b->cache_active_timeout
             && a->cache_max_flows == b->cache_max_flows
             && a->enable_tunnel_sampling == b->enable_tunnel_sampling
-            && sset_equals(&a->targets, &b->targets));
+            && sset_equals(&a->targets, &b->targets)
+            && nullable_string_is_equal(a->virtual_obs_id, b->virtual_obs_id));
 }
 
 static struct ofproto_ipfix_flow_exporter_options *
@@ -490,6 +529,8 @@  ofproto_ipfix_flow_exporter_options_clone(
     struct ofproto_ipfix_flow_exporter_options *new =
         xmemdup(old, sizeof *old);
     sset_clone(&new->targets, &old->targets);
+    new->virtual_obs_id = old->virtual_obs_id ? xstrdup(old->virtual_obs_id)
+                                              : NULL;
     return new;
 }
 
@@ -499,6 +540,7 @@  ofproto_ipfix_flow_exporter_options_destroy(
 {
     if (options) {
         sset_destroy(&options->targets);
+        free(options->virtual_obs_id);
         free(options);
     }
 }
@@ -513,6 +555,8 @@  dpif_ipfix_exporter_init(struct dpif_ipfix_exporter *exporter)
     ovs_list_init(&exporter->cache_flow_start_timestamp_list);
     exporter->cache_active_timeout = 0;
     exporter->cache_max_flows = 0;
+    exporter->virtual_obs_id = NULL;
+    exporter->virtual_obs_len = 0;
 }
 
 static void
@@ -527,6 +571,9 @@  dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter *exporter)
     exporter->last_template_set_time = 0;
     exporter->cache_active_timeout = 0;
     exporter->cache_max_flows = 0;
+    free(exporter->virtual_obs_id);
+    exporter->virtual_obs_id = NULL;
+    exporter->virtual_obs_len = 0;
 }
 
 static void
@@ -540,8 +587,10 @@  static bool
 dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter *exporter,
                                 const struct sset *targets,
                                 const uint32_t cache_active_timeout,
-                                const uint32_t cache_max_flows)
+                                const uint32_t cache_max_flows,
+                                const char *virtual_obs_id)
 {
+    size_t virtual_obs_len;
     collectors_destroy(exporter->collectors);
     collectors_create(targets, IPFIX_DEFAULT_COLLECTOR_PORT,
                       &exporter->collectors);
@@ -553,6 +602,16 @@  dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter *exporter,
     }
     exporter->cache_active_timeout = cache_active_timeout;
     exporter->cache_max_flows = cache_max_flows;
+    virtual_obs_len = virtual_obs_id ? strlen(virtual_obs_id) : 0;
+    if (virtual_obs_len > IPFIX_VIRTUAL_OBS_MAX_LEN) {
+        VLOG_WARN_RL(&rl, "Virtual obsevation ID too long(%d bytes), "
+                     "should not be longer than %d bytes.",
+                     exporter->virtual_obs_len, IPFIX_VIRTUAL_OBS_MAX_LEN);
+        dpif_ipfix_exporter_clear(exporter);
+        return false;
+    }
+    exporter->virtual_obs_len = virtual_obs_len;
+    exporter->virtual_obs_id = virtual_obs_id ? xstrdup(virtual_obs_id) : NULL;
     return true;
 }
 
@@ -707,7 +766,8 @@  dpif_ipfix_bridge_exporter_set_options(
             < sset_count(&options->targets)) {
         if (!dpif_ipfix_exporter_set_options(
                 &exporter->exporter, &options->targets,
-                options->cache_active_timeout, options->cache_max_flows)) {
+                options->cache_active_timeout, options->cache_max_flows,
+                options->virtual_obs_id)) {
             return;
         }
     }
@@ -795,7 +855,8 @@  dpif_ipfix_flow_exporter_set_options(
             < sset_count(&options->targets)) {
         if (!dpif_ipfix_exporter_set_options(
                 &exporter->exporter, &options->targets,
-                options->cache_active_timeout, options->cache_max_flows)) {
+                options->cache_active_timeout, options->cache_max_flows,
+                options->virtual_obs_id)) {
             return false;
         }
     }
@@ -1074,6 +1135,7 @@  ipfix_define_template_entity(enum ipfix_entity_id id,
 static uint16_t
 ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
                              enum ipfix_proto_l4 l4, enum ipfix_proto_tunnel tunnel,
+                             bool virtual_obs_id_set,
                              struct dp_packet *msg)
 {
     uint16_t count = 0;
@@ -1145,7 +1207,12 @@  ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
         DEF(TUNNEL_KEY);
     }
 
-    /* 2. Flow aggregated data. */
+    /* 2. virtual obs ID, which is not a part of flow key */
+    if (virtual_obs_id_set) {
+        DEF(VIRTUAL_OBS_ID);
+    }
+
+    /* 3. Flow aggregated data. */
 
     DEF(FLOW_START_DELTA_MICROSECONDS);
     DEF(FLOW_END_DELTA_MICROSECONDS);
@@ -1159,8 +1226,6 @@  ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
         DEF(MINIMUM_IP_TOTAL_LENGTH);
         DEF(MAXIMUM_IP_TOTAL_LENGTH);
     }
-
-
 #undef DEF
 
     return count;
@@ -1254,7 +1319,8 @@  ipfix_send_template_msgs(struct dpif_ipfix_exporter *exporter,
                     tmpl_hdr->template_id = htons(
                         ipfix_get_template_id(l2, l3, l4, tunnel));
                     field_count =
-                        ipfix_define_template_fields(l2, l3, l4, tunnel, &msg);
+                        ipfix_define_template_fields(l2, l3, l4, tunnel,
+                             exporter->virtual_obs_id != NULL, &msg);
                     tmpl_hdr = (struct ipfix_template_record_header*)
                         ((uint8_t*)dp_packet_data(&msg) + tmpl_hdr_offset);
                     tmpl_hdr->field_count = htons(field_count);
@@ -1738,6 +1804,8 @@  static void
 ipfix_put_data_set(uint32_t export_time_sec,
                    struct ipfix_flow_cache_entry *entry,
                    enum ipfix_flow_end_reason flow_end_reason,
+                   const char *virtual_obs_id,
+                   uint8_t virtual_obs_len,
                    struct dp_packet *msg)
 {
     size_t set_hdr_offset;
@@ -1754,6 +1822,12 @@  ipfix_put_data_set(uint32_t export_time_sec,
     dp_packet_put(msg, entry->flow_key.flow_key_msg_part,
                entry->flow_key.flow_key_msg_part_size);
 
+    /* Export virtual obs ID */
+    if (virtual_obs_id) {
+        dp_packet_put(msg, &virtual_obs_len, sizeof(virtual_obs_len));
+        dp_packet_put(msg, virtual_obs_id, virtual_obs_len);
+    }
+
     /* Put the non-key part of the data record. */
 
     {
@@ -1816,7 +1890,9 @@  ipfix_send_data_msg(struct dpif_ipfix_exporter *exporter,
 
     ipfix_init_header(export_time_sec, exporter->seq_number++,
                       entry->flow_key.obs_domain_id, &msg);
-    ipfix_put_data_set(export_time_sec, entry, flow_end_reason, &msg);
+    ipfix_put_data_set(export_time_sec, entry, flow_end_reason,
+                       exporter->virtual_obs_id, exporter->virtual_obs_len,
+                       &msg);
     tx_errors = ipfix_send_msg(exporter->collectors, &msg);
 
     dp_packet_uninit(&msg);
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index 774b5b6..26c687c 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -82,6 +82,7 @@  struct ofproto_ipfix_bridge_exporter_options {
     bool enable_tunnel_sampling;
     bool enable_input_sampling;
     bool enable_output_sampling;
+    char *virtual_obs_id;
 };
 
 struct ofproto_ipfix_flow_exporter_options {
@@ -90,6 +91,7 @@  struct ofproto_ipfix_flow_exporter_options {
     uint32_t cache_active_timeout;
     uint32_t cache_max_flows;
     bool enable_tunnel_sampling;
+    char *virtual_obs_id;
 };
 
 struct ofproto_rstp_status {
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 4273552..b4bede0 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1165,6 +1165,7 @@  bridge_configure_ipfix(struct bridge *br)
     struct ofproto_ipfix_bridge_exporter_options be_opts;
     struct ofproto_ipfix_flow_exporter_options *fe_opts = NULL;
     size_t n_fe_opts = 0;
+    const char *virtual_obs_id;
 
     OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
         if (ovsrec_fscs_is_valid(fe_cfg, br)) {
@@ -1209,6 +1210,10 @@  bridge_configure_ipfix(struct bridge *br)
 
         be_opts.enable_output_sampling = !smap_get_bool(&be_cfg->other_config,
                                               "enable-output-sampling", false);
+
+        virtual_obs_id = smap_get(&be_cfg->other_config, "virtual_obs_id");
+        be_opts.virtual_obs_id = virtual_obs_id ? xstrdup(virtual_obs_id)
+                                                : NULL;
     }
 
     if (n_fe_opts > 0) {
@@ -1228,6 +1233,10 @@  bridge_configure_ipfix(struct bridge *br)
                 opts->enable_tunnel_sampling = smap_get_bool(
                                                    &fe_cfg->ipfix->other_config,
                                                   "enable-tunnel-sampling", true);
+                virtual_obs_id = smap_get(&be_cfg->other_config,
+                                     "virtual_obs_id");
+                opts->virtual_obs_id = virtual_obs_id ? xstrdup(virtual_obs_id)
+                                                      : NULL;
                 opts++;
             }
         }
@@ -1238,6 +1247,7 @@  bridge_configure_ipfix(struct bridge *br)
 
     if (valid_be_cfg) {
         sset_destroy(&be_opts.targets);
+        free(be_opts.virtual_obs_id);
     }
 
     if (n_fe_opts > 0) {
@@ -1245,6 +1255,7 @@  bridge_configure_ipfix(struct bridge *br)
         size_t i;
         for (i = 0; i < n_fe_opts; i++) {
             sset_destroy(&opts->targets);
+            free(opts->virtual_obs_id);
             opts++;
         }
         free(fe_opts);
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 6e0ee80..c2dd5ee 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -4743,6 +4743,40 @@ 
       </p>
     </column>
 
+    <column name="other_config" key="virtual_obs_id"
+            type='{"type": "string"}'>
+      The IPFIX virtual observation ID sent with each IPFIX flow reord.
+      It is an identifier of a virtual obsvervation point that is locally
+      unique in a virtual network. It describes a location in the virtual
+      network where IP packets can be observed. The max length is limited to
+      254 bytes.
+      If not specified or the length of the specified ID exceeds the max
+      length, nothing exported with IPFIX flow record.
+      <p>
+        The following enterprise entity reports the specified virtual
+        observation ID:
+      </p>
+
+      <dl>
+        <dt>virtualObsID:</dt>
+        <dd>
+          <p>ID: 898, and enterprise ID 6876 (VMware).</p>
+          <p>type: variable-length string.</p>
+          <p>data type semantics: identifier.</p>
+          <p>description: The Identifier of the virtual observation domain ID
+             that is locally unique in a virtual network.
+          </p>
+        </dd>
+      </dl>
+
+      <p>
+        Before Open vSwitch 2.5.90, <ref column="other_config"
+        key="virtual_obs_id"/> was not supported.  Open vSwitch 2.6 and later
+        support <ref column="other_config" key="virtual_obs_id"/> for
+        per-bridge and per-flow sampling.
+      </p>
+    </column>
+
     <group title="Per-Bridge Sampling">
       <p>
         These values affect only per-bridge sampling.  See above for a