diff mbox

[ovs-dev,2/2] ipfix: Update Timestamp when flow updated

Message ID 20170606010451.14125-2-blp@ovn.org
State Deferred
Headers show

Commit Message

Ben Pfaff June 6, 2017, 1:04 a.m. UTC
From: Greg Rose <gvrose8192@gmail.com>

Reported-by: Felix Konstantin Maurer <felix.maurer@comsys.rwth-aachen.de>
Signed-off-by: Greg Rose <gvrose8192@gmail.com>
[blp@ovn.org changed this to use ipfix_now()]
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ofproto/ofproto-dpif-ipfix.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Gregory Rose June 6, 2017, 3:34 a.m. UTC | #1
On 06/05/2017 06:04 PM, Ben Pfaff wrote:
> From: Greg Rose <gvrose8192@gmail.com>
>
> Reported-by: Felix Konstantin Maurer <felix.maurer@comsys.rwth-aachen.de>
> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
> [blp@ovn.org changed this to use ipfix_now()]
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>   ofproto/ofproto-dpif-ipfix.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> index 5589b0ea05e1..bc63b7b0294b 100644
> --- a/ofproto/ofproto-dpif-ipfix.c
> +++ b/ofproto/ofproto-dpif-ipfix.c
> @@ -1643,6 +1643,7 @@ ipfix_cache_update(struct dpif_ipfix_exporter *exporter,
>           ipfix_cache_aggregate_entries(entry, old_entry);
>           free(entry);
>           ipfix_update_stats(exporter, false, current_flows, sampled_pkt_type);
> +        old_entry->flow_end_timestamp_usec = ipfix_now();
>       }
>   }
>
>
Looks good, thanks Ben!

- Greg
Ben Pfaff June 6, 2017, 3:22 p.m. UTC | #2
On Mon, Jun 05, 2017 at 08:34:32PM -0700, Greg Rose wrote:
> On 06/05/2017 06:04 PM, Ben Pfaff wrote:
> >From: Greg Rose <gvrose8192@gmail.com>
> >
> >Reported-by: Felix Konstantin Maurer <felix.maurer@comsys.rwth-aachen.de>
> >Signed-off-by: Greg Rose <gvrose8192@gmail.com>
> >[blp@ovn.org changed this to use ipfix_now()]
> >Signed-off-by: Ben Pfaff <blp@ovn.org>
> >---
> >  ofproto/ofproto-dpif-ipfix.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> >diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> >index 5589b0ea05e1..bc63b7b0294b 100644
> >--- a/ofproto/ofproto-dpif-ipfix.c
> >+++ b/ofproto/ofproto-dpif-ipfix.c
> >@@ -1643,6 +1643,7 @@ ipfix_cache_update(struct dpif_ipfix_exporter *exporter,
> >          ipfix_cache_aggregate_entries(entry, old_entry);
> >          free(entry);
> >          ipfix_update_stats(exporter, false, current_flows, sampled_pkt_type);
> >+        old_entry->flow_end_timestamp_usec = ipfix_now();
> >      }
> >  }
> >
> >
> Looks good, thanks Ben!

Thanks for the review!

If I recall correctly, Felix reported that your original patch didn't
help, though, so probably this one doesn't either.  We should track that
down before we go farther, I guess.
Gregory Rose June 6, 2017, 8:42 p.m. UTC | #3
On 06/06/2017 08:22 AM, Ben Pfaff wrote:
> On Mon, Jun 05, 2017 at 08:34:32PM -0700, Greg Rose wrote:
> > On 06/05/2017 06:04 PM, Ben Pfaff wrote:
> >> From: Greg Rose <gvrose8192@gmail.com>
> >>
> >> Reported-by: Felix Konstantin Maurer <felix.maurer@comsys.rwth-aachen.de>
> >> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
> >> [blp@ovn.org changed this to use ipfix_now()]
> >> Signed-off-by: Ben Pfaff <blp@ovn.org>
> >> ---
> >>   ofproto/ofproto-dpif-ipfix.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> >> index 5589b0ea05e1..bc63b7b0294b 100644
> >> --- a/ofproto/ofproto-dpif-ipfix.c
> >> +++ b/ofproto/ofproto-dpif-ipfix.c
> >> @@ -1643,6 +1643,7 @@ ipfix_cache_update(struct dpif_ipfix_exporter *exporter,
> >>           ipfix_cache_aggregate_entries(entry, old_entry);
> >>           free(entry);
> >>           ipfix_update_stats(exporter, false, current_flows, sampled_pkt_type);
> >> +        old_entry->flow_end_timestamp_usec = ipfix_now();
> >>       }
> >>   }
> >>
> >>
> > Looks good, thanks Ben!
>
> Thanks for the review!
>
> If I recall correctly, Felix reported that your original patch didn't
> help, though, so probably this one doesn't either.  We should track that
> down before we go farther, I guess.
>
Yes, I'm am following up.  Having all sorts of problems getting a working ipfix flow collector to work though.  The ManageEngine netflow collector claims to work with ipfix but SFAICT it does not so I'm not going to waste more time on it.  Once I can start
collecting and analyzing the work should proceed quickly.

Thanks,

- Greg
Felix Konstantin Maurer June 7, 2017, 7:23 a.m. UTC | #4
In case you know a little Go, here is my script to test my IPFIX setup.
I also use Wireshark which works quite well.
I'll look into it too, today.

Regards
Felix

On 06/06/2017 10:42 PM, Greg Rose wrote:
> On 06/06/2017 08:22 AM, Ben Pfaff wrote:
>> On Mon, Jun 05, 2017 at 08:34:32PM -0700, Greg Rose wrote:
>> > On 06/05/2017 06:04 PM, Ben Pfaff wrote:
>> >> From: Greg Rose <gvrose8192@gmail.com>
>> >>
>> >> Reported-by: Felix Konstantin Maurer
>> <felix.maurer@comsys.rwth-aachen.de>
>> >> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
>> >> [blp@ovn.org changed this to use ipfix_now()]
>> >> Signed-off-by: Ben Pfaff <blp@ovn.org>
>> >> ---
>> >>   ofproto/ofproto-dpif-ipfix.c | 1 +
>> >>   1 file changed, 1 insertion(+)
>> >>
>> >> diff --git a/ofproto/ofproto-dpif-ipfix.c
>> b/ofproto/ofproto-dpif-ipfix.c
>> >> index 5589b0ea05e1..bc63b7b0294b 100644
>> >> --- a/ofproto/ofproto-dpif-ipfix.c
>> >> +++ b/ofproto/ofproto-dpif-ipfix.c
>> >> @@ -1643,6 +1643,7 @@ ipfix_cache_update(struct dpif_ipfix_exporter
>> *exporter,
>> >>           ipfix_cache_aggregate_entries(entry, old_entry);
>> >>           free(entry);
>> >>           ipfix_update_stats(exporter, false, current_flows,
>> sampled_pkt_type);
>> >> +        old_entry->flow_end_timestamp_usec = ipfix_now();
>> >>       }
>> >>   }
>> >>
>> >>
>> > Looks good, thanks Ben!
>>
>> Thanks for the review!
>>
>> If I recall correctly, Felix reported that your original patch didn't
>> help, though, so probably this one doesn't either.  We should track that
>> down before we go farther, I guess.
>>
> Yes, I'm am following up.  Having all sorts of problems getting a
> working ipfix flow collector to work though.  The ManageEngine netflow
> collector claims to work with ipfix but SFAICT it does not so I'm not
> going to waste more time on it.  Once I can start
> collecting and analyzing the work should proceed quickly.
> 
> Thanks,
> 
> - Greg
Gregory Rose June 7, 2017, 2:57 p.m. UTC | #5
On 06/07/2017 12:23 AM, Felix Konstantin Maurer wrote:
> In case you know a little Go, here is my script to test my IPFIX setup.
> I also use Wireshark which works quite well.
> I'll look into it too, today.
> 
> Regards
> Felix

Yes, I'm just capturing the traffic to a pcap file and will try wireshark.

All the ipfix collectors that I can afford (i.e. free) take a lot of work
to get configured and working correctly.  I'd prefer to spend time working
on the actual problem here and I can get an ipfix collector set up later.

Thanks,

- Greg

> 
> On 06/06/2017 10:42 PM, Greg Rose wrote:
>> On 06/06/2017 08:22 AM, Ben Pfaff wrote:
>>> On Mon, Jun 05, 2017 at 08:34:32PM -0700, Greg Rose wrote:
>>>> On 06/05/2017 06:04 PM, Ben Pfaff wrote:
>>>>> From: Greg Rose <gvrose8192@gmail.com>
>>>>>
>>>>> Reported-by: Felix Konstantin Maurer
>>> <felix.maurer@comsys.rwth-aachen.de>
>>>>> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
>>>>> [blp@ovn.org changed this to use ipfix_now()]
>>>>> Signed-off-by: Ben Pfaff <blp@ovn.org>
>>>>> ---
>>>>>    ofproto/ofproto-dpif-ipfix.c | 1 +
>>>>>    1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/ofproto/ofproto-dpif-ipfix.c
>>> b/ofproto/ofproto-dpif-ipfix.c
>>>>> index 5589b0ea05e1..bc63b7b0294b 100644
>>>>> --- a/ofproto/ofproto-dpif-ipfix.c
>>>>> +++ b/ofproto/ofproto-dpif-ipfix.c
>>>>> @@ -1643,6 +1643,7 @@ ipfix_cache_update(struct dpif_ipfix_exporter
>>> *exporter,
>>>>>            ipfix_cache_aggregate_entries(entry, old_entry);
>>>>>            free(entry);
>>>>>            ipfix_update_stats(exporter, false, current_flows,
>>> sampled_pkt_type);
>>>>> +        old_entry->flow_end_timestamp_usec = ipfix_now();
>>>>>        }
>>>>>    }
>>>>>
>>>>>
>>>> Looks good, thanks Ben!
>>>
>>> Thanks for the review!
>>>
>>> If I recall correctly, Felix reported that your original patch didn't
>>> help, though, so probably this one doesn't either.  We should track that
>>> down before we go farther, I guess.
>>>
>> Yes, I'm am following up.  Having all sorts of problems getting a
>> working ipfix flow collector to work though.  The ManageEngine netflow
>> collector claims to work with ipfix but SFAICT it does not so I'm not
>> going to waste more time on it.  Once I can start
>> collecting and analyzing the work should proceed quickly.
>>
>> Thanks,
>>
>> - Greg
>
Ben Pfaff Oct. 24, 2017, 10:19 p.m. UTC | #6
On Tue, Jun 06, 2017 at 01:42:08PM -0700, Greg Rose wrote:
> On 06/06/2017 08:22 AM, Ben Pfaff wrote:
> >On Mon, Jun 05, 2017 at 08:34:32PM -0700, Greg Rose wrote:
> >> On 06/05/2017 06:04 PM, Ben Pfaff wrote:
> >>> From: Greg Rose <gvrose8192@gmail.com>
> >>>
> >>> Reported-by: Felix Konstantin Maurer <felix.maurer@comsys.rwth-aachen.de>
> >>> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
> >>> [blp@ovn.org changed this to use ipfix_now()]
> >>> Signed-off-by: Ben Pfaff <blp@ovn.org>
> >>> ---
> >>>   ofproto/ofproto-dpif-ipfix.c | 1 +
> >>>   1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> >>> index 5589b0ea05e1..bc63b7b0294b 100644
> >>> --- a/ofproto/ofproto-dpif-ipfix.c
> >>> +++ b/ofproto/ofproto-dpif-ipfix.c
> >>> @@ -1643,6 +1643,7 @@ ipfix_cache_update(struct dpif_ipfix_exporter *exporter,
> >>>           ipfix_cache_aggregate_entries(entry, old_entry);
> >>>           free(entry);
> >>>           ipfix_update_stats(exporter, false, current_flows, sampled_pkt_type);
> >>> +        old_entry->flow_end_timestamp_usec = ipfix_now();
> >>>       }
> >>>   }
> >>>
> >>>
> >> Looks good, thanks Ben!
> >
> >Thanks for the review!
> >
> >If I recall correctly, Felix reported that your original patch didn't
> >help, though, so probably this one doesn't either.  We should track that
> >down before we go farther, I guess.
> >
> Yes, I'm am following up.  Having all sorts of problems getting a working ipfix flow collector to work though.  The ManageEngine netflow collector claims to work with ipfix but SFAICT it does not so I'm not going to waste more time on it.  Once I can start
> collecting and analyzing the work should proceed quickly.

I never applied this patch (from June) because we were continuing to
look deeper.  I don't know whether that ever bore fruit.  Should I apply
this patch now?

Thanks,

Ben.
Gregory Rose Oct. 31, 2017, 5:08 p.m. UTC | #7
On 10/24/2017 03:19 PM, Ben Pfaff wrote:
> On Tue, Jun 06, 2017 at 01:42:08PM -0700, Greg Rose wrote:
>> On 06/06/2017 08:22 AM, Ben Pfaff wrote:
>>> On Mon, Jun 05, 2017 at 08:34:32PM -0700, Greg Rose wrote:
>>>> On 06/05/2017 06:04 PM, Ben Pfaff wrote:
>>>>> From: Greg Rose <gvrose8192@gmail.com>
>>>>>
>>>>> Reported-by: Felix Konstantin Maurer <felix.maurer@comsys.rwth-aachen.de>
>>>>> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
>>>>> [blp@ovn.org changed this to use ipfix_now()]
>>>>> Signed-off-by: Ben Pfaff <blp@ovn.org>
>>>>> ---
>>>>>    ofproto/ofproto-dpif-ipfix.c | 1 +
>>>>>    1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
>>>>> index 5589b0ea05e1..bc63b7b0294b 100644
>>>>> --- a/ofproto/ofproto-dpif-ipfix.c
>>>>> +++ b/ofproto/ofproto-dpif-ipfix.c
>>>>> @@ -1643,6 +1643,7 @@ ipfix_cache_update(struct dpif_ipfix_exporter *exporter,
>>>>>            ipfix_cache_aggregate_entries(entry, old_entry);
>>>>>            free(entry);
>>>>>            ipfix_update_stats(exporter, false, current_flows, sampled_pkt_type);
>>>>> +        old_entry->flow_end_timestamp_usec = ipfix_now();
>>>>>        }
>>>>>    }
>>>>>
>>>>>
>>>> Looks good, thanks Ben!
>>>
>>> Thanks for the review!
>>>
>>> If I recall correctly, Felix reported that your original patch didn't
>>> help, though, so probably this one doesn't either.  We should track that
>>> down before we go farther, I guess.
>>>
>> Yes, I'm am following up.  Having all sorts of problems getting a working ipfix flow collector to work though.  The ManageEngine netflow collector claims to work with ipfix but SFAICT it does not so I'm not going to waste more time on it.  Once I can start
>> collecting and analyzing the work should proceed quickly.
> 
> I never applied this patch (from June) because we were continuing to
> look deeper.  I don't know whether that ever bore fruit.  Should I apply
> this patch now?
> 
> Thanks,
> 
> Ben.
> 

I think this one got lost.  I never followed up on it and got busy with other more pressing issues.

- Greg
Ben Pfaff Oct. 31, 2017, 5:37 p.m. UTC | #8
On Tue, Oct 31, 2017 at 10:08:08AM -0700, Greg Rose wrote:
> On 10/24/2017 03:19 PM, Ben Pfaff wrote:
> >On Tue, Jun 06, 2017 at 01:42:08PM -0700, Greg Rose wrote:
> >>On 06/06/2017 08:22 AM, Ben Pfaff wrote:
> >>>On Mon, Jun 05, 2017 at 08:34:32PM -0700, Greg Rose wrote:
> >>>>On 06/05/2017 06:04 PM, Ben Pfaff wrote:
> >>>>>From: Greg Rose <gvrose8192@gmail.com>
> >>>>>
> >>>>>Reported-by: Felix Konstantin Maurer <felix.maurer@comsys.rwth-aachen.de>
> >>>>>Signed-off-by: Greg Rose <gvrose8192@gmail.com>
> >>>>>[blp@ovn.org changed this to use ipfix_now()]
> >>>>>Signed-off-by: Ben Pfaff <blp@ovn.org>
> >>>>>---
> >>>>>   ofproto/ofproto-dpif-ipfix.c | 1 +
> >>>>>   1 file changed, 1 insertion(+)
> >>>>>
> >>>>>diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> >>>>>index 5589b0ea05e1..bc63b7b0294b 100644
> >>>>>--- a/ofproto/ofproto-dpif-ipfix.c
> >>>>>+++ b/ofproto/ofproto-dpif-ipfix.c
> >>>>>@@ -1643,6 +1643,7 @@ ipfix_cache_update(struct dpif_ipfix_exporter *exporter,
> >>>>>           ipfix_cache_aggregate_entries(entry, old_entry);
> >>>>>           free(entry);
> >>>>>           ipfix_update_stats(exporter, false, current_flows, sampled_pkt_type);
> >>>>>+        old_entry->flow_end_timestamp_usec = ipfix_now();
> >>>>>       }
> >>>>>   }
> >>>>>
> >>>>>
> >>>>Looks good, thanks Ben!
> >>>
> >>>Thanks for the review!
> >>>
> >>>If I recall correctly, Felix reported that your original patch didn't
> >>>help, though, so probably this one doesn't either.  We should track that
> >>>down before we go farther, I guess.
> >>>
> >>Yes, I'm am following up.  Having all sorts of problems getting a working ipfix flow collector to work though.  The ManageEngine netflow collector claims to work with ipfix but SFAICT it does not so I'm not going to waste more time on it.  Once I can start
> >>collecting and analyzing the work should proceed quickly.
> >
> >I never applied this patch (from June) because we were continuing to
> >look deeper.  I don't know whether that ever bore fruit.  Should I apply
> >this patch now?
> >
> >Thanks,
> >
> >Ben.
> >
> 
> I think this one got lost.  I never followed up on it and got busy with other more pressing issues.

Should it get revived?
Gregory Rose Nov. 1, 2017, 3:25 p.m. UTC | #9
On 10/31/2017 10:37 AM, Ben Pfaff wrote:
> On Tue, Oct 31, 2017 at 10:08:08AM -0700, Greg Rose wrote:
>> On 10/24/2017 03:19 PM, Ben Pfaff wrote:
>>> On Tue, Jun 06, 2017 at 01:42:08PM -0700, Greg Rose wrote:
>>>> On 06/06/2017 08:22 AM, Ben Pfaff wrote:
>>>>> On Mon, Jun 05, 2017 at 08:34:32PM -0700, Greg Rose wrote:
>>>>>> On 06/05/2017 06:04 PM, Ben Pfaff wrote:
>>>>>>> From: Greg Rose <gvrose8192@gmail.com>
>>>>>>>
>>>>>>> Reported-by: Felix Konstantin Maurer <felix.maurer@comsys.rwth-aachen.de>
>>>>>>> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
>>>>>>> [blp@ovn.org changed this to use ipfix_now()]
>>>>>>> Signed-off-by: Ben Pfaff <blp@ovn.org>
>>>>>>> ---
>>>>>>>    ofproto/ofproto-dpif-ipfix.c | 1 +
>>>>>>>    1 file changed, 1 insertion(+)
>>>>>>>
>>>>>>> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
>>>>>>> index 5589b0ea05e1..bc63b7b0294b 100644
>>>>>>> --- a/ofproto/ofproto-dpif-ipfix.c
>>>>>>> +++ b/ofproto/ofproto-dpif-ipfix.c
>>>>>>> @@ -1643,6 +1643,7 @@ ipfix_cache_update(struct dpif_ipfix_exporter *exporter,
>>>>>>>            ipfix_cache_aggregate_entries(entry, old_entry);
>>>>>>>            free(entry);
>>>>>>>            ipfix_update_stats(exporter, false, current_flows, sampled_pkt_type);
>>>>>>> +        old_entry->flow_end_timestamp_usec = ipfix_now();
>>>>>>>        }
>>>>>>>    }
>>>>>>>
>>>>>>>
>>>>>> Looks good, thanks Ben!
>>>>>
>>>>> Thanks for the review!
>>>>>
>>>>> If I recall correctly, Felix reported that your original patch didn't
>>>>> help, though, so probably this one doesn't either.  We should track that
>>>>> down before we go farther, I guess.
>>>>>
>>>> Yes, I'm am following up.  Having all sorts of problems getting a working ipfix flow collector to work though.  The ManageEngine netflow collector claims to work with ipfix but SFAICT it does not so I'm not going to waste more time on it.  Once I can start
>>>> collecting and analyzing the work should proceed quickly.
>>>
>>> I never applied this patch (from June) because we were continuing to
>>> look deeper.  I don't know whether that ever bore fruit.  Should I apply
>>> this patch now?
>>>
>>> Thanks,
>>>
>>> Ben.
>>>
>>
>> I think this one got lost.  I never followed up on it and got busy with other more pressing issues.
> 
> Should it get revived?
>
Well, when I think about it I have been using ipfix for quite a while
and I haven't seen any timestamp issues.  So unless we see a reported
bug I'd hold off.

Thanks,

- Greg
diff mbox

Patch

diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index 5589b0ea05e1..bc63b7b0294b 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -1643,6 +1643,7 @@  ipfix_cache_update(struct dpif_ipfix_exporter *exporter,
         ipfix_cache_aggregate_entries(entry, old_entry);
         free(entry);
         ipfix_update_stats(exporter, false, current_flows, sampled_pkt_type);
+        old_entry->flow_end_timestamp_usec = ipfix_now();
     }
 }