| Message ID | 1501140438-23874-1-git-send-email-roid@mellanox.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Jul 27, 2017 at 10:27:18AM +0300, Roi Dayan wrote: > Since it's an error message log it with VLOG_ERR instead of VLOG_INFO. > > Signed-off-by: Roi Dayan <roid@mellanox.com> > --- > lib/dpif-netlink.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > index 55effd1..7b53ed5 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -972,8 +972,8 @@ dpif_netlink_rtnl_port_create_and_add(struct dpif_netlink *dpif, > error = dpif_netlink_rtnl_port_create(netdev); > if (error) { > if (error != EOPNOTSUPP) { > - VLOG_INFO_RL(&rl, "Failed to create %s with rtnetlink: %s", > - netdev_get_name(netdev), ovs_strerror(error)); > + VLOG_ERR_RL(&rl, "Failed to create %s with rtnetlink: %s", > + netdev_get_name(netdev), ovs_strerror(error)); > } > return error; > } > -- > 2.7.4 I don't think this is appropriate. If creating with rtnetlink fails we'll fall back to using the compat interface. As such, we may still successfully create the tunnel. Maybe WARN would be more appropriate?
On 27 July 2017 at 05:43, Eric Garver <e@erig.me> wrote: > On Thu, Jul 27, 2017 at 10:27:18AM +0300, Roi Dayan wrote: >> Since it's an error message log it with VLOG_ERR instead of VLOG_INFO. >> >> Signed-off-by: Roi Dayan <roid@mellanox.com> >> --- >> lib/dpif-netlink.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >> index 55effd1..7b53ed5 100644 >> --- a/lib/dpif-netlink.c >> +++ b/lib/dpif-netlink.c >> @@ -972,8 +972,8 @@ dpif_netlink_rtnl_port_create_and_add(struct dpif_netlink *dpif, >> error = dpif_netlink_rtnl_port_create(netdev); >> if (error) { >> if (error != EOPNOTSUPP) { >> - VLOG_INFO_RL(&rl, "Failed to create %s with rtnetlink: %s", >> - netdev_get_name(netdev), ovs_strerror(error)); >> + VLOG_ERR_RL(&rl, "Failed to create %s with rtnetlink: %s", >> + netdev_get_name(netdev), ovs_strerror(error)); >> } >> return error; >> } >> -- >> 2.7.4 > > I don't think this is appropriate. If creating with rtnetlink fails > we'll fall back to using the compat interface. As such, we may still > successfully create the tunnel. Maybe WARN would be more appropriate? This probably also depends on which kernels we think are most common. If the next release we expect to be deployed with kernels that don't support rtnetlink+metadata_dst for the tunnel types we want, then raising the log level doesn't sound appropriate. But if we expect that in most cases people are deploying with either out-of-tree modules, or with kernels that support rtnetlink+metadata_dst then we could consider raising the log level, for instance to WARN at first and maybe ERR later.
On Thu, Jul 27, 2017 at 10:17:48AM -0700, Joe Stringer wrote: > On 27 July 2017 at 05:43, Eric Garver <e@erig.me> wrote: > > On Thu, Jul 27, 2017 at 10:27:18AM +0300, Roi Dayan wrote: > >> Since it's an error message log it with VLOG_ERR instead of VLOG_INFO. > >> > >> Signed-off-by: Roi Dayan <roid@mellanox.com> > >> --- > >> lib/dpif-netlink.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > >> index 55effd1..7b53ed5 100644 > >> --- a/lib/dpif-netlink.c > >> +++ b/lib/dpif-netlink.c > >> @@ -972,8 +972,8 @@ dpif_netlink_rtnl_port_create_and_add(struct dpif_netlink *dpif, > >> error = dpif_netlink_rtnl_port_create(netdev); > >> if (error) { > >> if (error != EOPNOTSUPP) { > >> - VLOG_INFO_RL(&rl, "Failed to create %s with rtnetlink: %s", > >> - netdev_get_name(netdev), ovs_strerror(error)); > >> + VLOG_ERR_RL(&rl, "Failed to create %s with rtnetlink: %s", > >> + netdev_get_name(netdev), ovs_strerror(error)); > >> } > >> return error; > >> } > >> -- > >> 2.7.4 > > > > I don't think this is appropriate. If creating with rtnetlink fails > > we'll fall back to using the compat interface. As such, we may still > > successfully create the tunnel. Maybe WARN would be more appropriate? > > This probably also depends on which kernels we think are most common. > If the next release we expect to be deployed with kernels that don't > support rtnetlink+metadata_dst for the tunnel types we want, then > raising the log level doesn't sound appropriate. But if we expect that > in most cases people are deploying with either out-of-tree modules, or > with kernels that support rtnetlink+metadata_dst then we could > consider raising the log level, for instance to WARN at first and > maybe ERR later. COLLECT_METADATA for GENEVE, VXLAN, and GRE was introduced in v4.3, which was tagged Nov 1, 2015. I would guess many people are using a new enough kernel or they're using the out-of-tree modules. As such, I'm okay with making this log a WARN.
On 27/07/2017 23:03, Eric Garver wrote: > On Thu, Jul 27, 2017 at 10:17:48AM -0700, Joe Stringer wrote: >> On 27 July 2017 at 05:43, Eric Garver <e@erig.me> wrote: >>> On Thu, Jul 27, 2017 at 10:27:18AM +0300, Roi Dayan wrote: >>>> Since it's an error message log it with VLOG_ERR instead of VLOG_INFO. >>>> >>>> Signed-off-by: Roi Dayan <roid@mellanox.com> >>>> --- >>>> lib/dpif-netlink.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >>>> index 55effd1..7b53ed5 100644 >>>> --- a/lib/dpif-netlink.c >>>> +++ b/lib/dpif-netlink.c >>>> @@ -972,8 +972,8 @@ dpif_netlink_rtnl_port_create_and_add(struct dpif_netlink *dpif, >>>> error = dpif_netlink_rtnl_port_create(netdev); >>>> if (error) { >>>> if (error != EOPNOTSUPP) { >>>> - VLOG_INFO_RL(&rl, "Failed to create %s with rtnetlink: %s", >>>> - netdev_get_name(netdev), ovs_strerror(error)); >>>> + VLOG_ERR_RL(&rl, "Failed to create %s with rtnetlink: %s", >>>> + netdev_get_name(netdev), ovs_strerror(error)); >>>> } >>>> return error; >>>> } >>>> -- >>>> 2.7.4 >>> >>> I don't think this is appropriate. If creating with rtnetlink fails >>> we'll fall back to using the compat interface. As such, we may still >>> successfully create the tunnel. Maybe WARN would be more appropriate? >> >> This probably also depends on which kernels we think are most common. >> If the next release we expect to be deployed with kernels that don't >> support rtnetlink+metadata_dst for the tunnel types we want, then >> raising the log level doesn't sound appropriate. But if we expect that >> in most cases people are deploying with either out-of-tree modules, or >> with kernels that support rtnetlink+metadata_dst then we could >> consider raising the log level, for instance to WARN at first and >> maybe ERR later. > > COLLECT_METADATA for GENEVE, VXLAN, and GRE was introduced in v4.3, > which was tagged Nov 1, 2015. I would guess many people are using a new > enough kernel or they're using the out-of-tree modules. > > As such, I'm okay with making this log a WARN. > ok thanks. i'll update the patch.
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 55effd1..7b53ed5 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -972,8 +972,8 @@ dpif_netlink_rtnl_port_create_and_add(struct dpif_netlink *dpif, error = dpif_netlink_rtnl_port_create(netdev); if (error) { if (error != EOPNOTSUPP) { - VLOG_INFO_RL(&rl, "Failed to create %s with rtnetlink: %s", - netdev_get_name(netdev), ovs_strerror(error)); + VLOG_ERR_RL(&rl, "Failed to create %s with rtnetlink: %s", + netdev_get_name(netdev), ovs_strerror(error)); } return error; }
Since it's an error message log it with VLOG_ERR instead of VLOG_INFO. Signed-off-by: Roi Dayan <roid@mellanox.com> --- lib/dpif-netlink.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)