Hi Mark, Sorry for the late reply. You wrote below that the first part of this RFS doesn't make sense — okay, that's a fair point.
Regarding the second part, you mentioned that the |ovn-trace|and |ovn-debug|utilities weren't updated to account for the changes and were taking constants as-is. I have to disagree here — I also converted the pipeline lengths to be stored in variables, and the code uses those values. Also, about the potential needs you mentioned, where you're used to looking in |controller/lflow.h|to find the required OpenFlow table number. Nothing changes in that regard (only the file name changes to |lib/oftable.c|). You can refer to it until the moment |northd|is updated forward; only after that should you use the utilities (|ovn-debug|). It seems to me that if a user has upgraded |northd|ahead of the controller, they should now rely not on the code in |lib/oftable.c|but on the values provided by the utilities. What do you think? If you're okay with it, I can resubmit the code without the RFS version and without the first part. On 19.09.2025 22:59, Mark Michelson wrote: > Hi Alexandra, > > Since this is an RFC, I'm going to respond to the entire series here > rather than addressing each patch individually. > > There are essentially two things being done in this RFC. > > 1. If the OVS DB has external_ids:validate-northd-actions, then check > for matching supported actions between ovn-northd and ovn-controller. > ovn-northd places its list of action names in the SB_Global database. > ovn-controller then can compare its action list to northd's. If northd > has actions not supported by ovn-controller, then ovn-controller will > keep its current OF flows in place and will not process updates from > ovn-northd. > > The idea is that this could provide a more granular check than simply > checking the northd version number. Even if the northd version number > is different, so long as ovn-controller knows all of northd's actions, > then ovn-controller can operate as normal. > > There are some flaws with this: > * The code compares action names from northd with action names from > ovn-controller. This does not account for semantic changes within an > action. For instance, the same action may exist on northd and > ovn-controller. However, the northd version of the action takes 4 > parameters, but the ovn-controller version of the action only takes 3. > Or maybe the number of parameters is the same, but the syntax for the > parameters has been altered. > * In some cases, northd makes use of chassis feature config (see > build_chassis_features() in northd/en-global-config.c). northd may > support a particular action, but it will only use that action if all > connected ovn-controllers support the action. Therefore, northd may > have an action ovn-controller does not support, but it may be > completely safe to run ovn-controller as normal since northd is not > going to use that action anyway. > * Even if all actions are the same between northd and ovn-controller, > it doesn't necessarily guarantee compatibility, since other factors > may be at play as well, like OVS or kernel features on the > ovn-controller hypervisor. > * Generally, the guidance for users is to upgrade ovn-controller > before ovn-northd. This way, ovn-controller will know about new > actions and will be aware of syntactic/semantic changes to actions > before northd does. But what about if an action is deprecated and > removed? In this case when ovn-controller is upgraded, it doesn't have > the action, but northd does. Deprecation of an action would be > introduced gradually, in such a way that ovn-northd would have > provisions not to use the action if it is connected to a version of > ovn-controller that does not have the action. > > Overall, I'm having trouble finding how this would be better than the > current safeguards we have in place. If users are upgrading > ovn-controller before ovn-northd, then everything should work since > that is how the code is designed to be upgraded. If users are not > following the guidelines and are upgrading ovn-northd first, then the > northd version mismatch check is more likely to catch > incompatibilities than comparing supported action names. > > 2. If ovn-northd and ovn-controller have mismatched pipeline lengths, > then dynamically update ovn-controller so that it will use the > pipeline lengths that northd expects. > > There are some problems with the implementation. > * The ovn-trace and ovn-debug utilities have not been updated to deal > with the mismatch. They just read the constants directly. This could > be a pain to try to fix since I'm not sure how you can know from these > utilities if you are running the same code as ovn-northd or > ovn-controller. > * This is going to make debugging more difficult (at least for me :) > ). I tend to reference controller/lflow.h in order to determine the OF > table numbers when looking through an OF dump. Since the initial > values in lib/oftable.c may not correspond to the actual values used > on a particular hypervisor, I can no longer trust the values in the > code. This could be remedied by providing an ovn-appctl command to > dump the table constants. > > I think (2) has more potential than (1). If users are following > guidelines and upgrading ovn-controller before ovn-northd, then this > should never be an issue. If the number of stages increases between > versions, then ovn-controller will have the updated higher values > before ovn-northd tries to use them. If the number of stages decreases > between versions, then we likely would keep the PIPELINE_LEN constants > the same, even if we actually are not using as many stages. > > However, if users are not following the guidelines and are upgrading > ovn-northd first, then this change has the potential to avoid some > catastrophes if northd thinks the pipeline lengths are longer than > ovn-controller does. > > > To sum up, I think (1) (patch 2 in the series) should be dropped. > However, I think (2) (patches 1 and 3 in the series) is worth > following up on, with the flaws addressed that I described above. > > On 8/29/25 2:16 PM, Alexandra Rukomoinikova wrote: >> Replaced macro definitions for OpenFlow table numbers >> with runtime variables to allow dynamic configuration. >> >> This change enables future dynamic adjustment of table numbering. >> >> Signed-off-by: Alexandra Rukomoinikova <[email protected]> >> --- >> controller/lflow.h | 45 -------------------------------- >> controller/mac-cache.c | 1 + >> controller/statctrl.c | 1 + >> include/ovn/actions.h | 1 + >> lib/automake.mk | 4 ++- >> lib/oftable.c | 55 +++++++++++++++++++++++++++++++++++++++ >> lib/oftable.h | 58 ++++++++++++++++++++++++++++++++++++++++++ >> lib/ovn-util.c | 3 +++ >> lib/ovn-util.h | 4 +-- >> utilities/ovn-debug.c | 1 + >> 10 files changed, 125 insertions(+), 48 deletions(-) >> create mode 100644 lib/oftable.c >> create mode 100644 lib/oftable.h >> >> diff --git a/controller/lflow.h b/controller/lflow.h >> index c8a87c886..f4ab70d6c 100644 >> --- a/controller/lflow.h >> +++ b/controller/lflow.h >> @@ -59,51 +59,6 @@ struct simap; >> struct sset; >> struct uuid; >> -/* OpenFlow table numbers. >> - * >> - * These are heavily documented in ovn-architecture(7), please >> update it if >> - * you make any changes. */ >> -#define OFTABLE_PHY_TO_LOG 0 >> - >> -/* Start of LOG_PIPELINE_LEN tables. */ >> -#define OFTABLE_LOG_INGRESS_PIPELINE 8 >> -#define OFTABLE_OUTPUT_LARGE_PKT_DETECT 41 >> -#define OFTABLE_OUTPUT_LARGE_PKT_PROCESS 42 >> -#define OFTABLE_REMOTE_OUTPUT 43 >> -#define OFTABLE_REMOTE_VTEP_OUTPUT 44 >> -#define OFTABLE_LOCAL_OUTPUT 45 >> -#define OFTABLE_CHECK_LOOPBACK 46 >> - >> -/* Start of the OUTPUT section of the pipeline. */ >> -#define OFTABLE_OUTPUT_INIT OFTABLE_OUTPUT_LARGE_PKT_DETECT >> - >> -/* Start of LOG_PIPELINE_LEN tables. */ >> -#define OFTABLE_LOG_EGRESS_PIPELINE 47 >> -#define OFTABLE_SAVE_INPORT 64 >> -#define OFTABLE_LOG_TO_PHY 65 >> -#define OFTABLE_MAC_BINDING 66 >> -#define OFTABLE_MAC_LOOKUP 67 >> -#define OFTABLE_CHK_LB_HAIRPIN 68 >> -#define OFTABLE_CHK_LB_HAIRPIN_REPLY 69 >> -#define OFTABLE_CT_SNAT_HAIRPIN 70 >> -#define OFTABLE_GET_FDB 71 >> -#define OFTABLE_LOOKUP_FDB 72 >> -#define OFTABLE_CHK_IN_PORT_SEC 73 >> -#define OFTABLE_CHK_IN_PORT_SEC_ND 74 >> -#define OFTABLE_CHK_OUT_PORT_SEC 75 >> -#define OFTABLE_ECMP_NH_MAC 76 >> -#define OFTABLE_ECMP_NH 77 >> -#define OFTABLE_CHK_LB_AFFINITY 78 >> -#define OFTABLE_MAC_CACHE_USE 79 >> -#define OFTABLE_CT_ZONE_LOOKUP 80 >> -#define OFTABLE_CT_ORIG_NW_DST_LOAD 81 >> -#define OFTABLE_CT_ORIG_IP6_DST_LOAD 82 >> -#define OFTABLE_CT_ORIG_TP_DST_LOAD 83 >> -#define OFTABLE_FLOOD_REMOTE_CHASSIS 84 >> -#define OFTABLE_CT_STATE_SAVE 85 >> -#define OFTABLE_CT_ORIG_PROTO_LOAD 86 >> -#define OFTABLE_GET_REMOTE_FDB 87 >> - >> /* Common defines shared between some controller components. */ >> #define CHASSIS_FLOOD_INDEX_START 0x8000 >> diff --git a/controller/mac-cache.c b/controller/mac-cache.c >> index fcf5fa7d4..0fea70333 100644 >> --- a/controller/mac-cache.c >> +++ b/controller/mac-cache.c >> @@ -19,6 +19,7 @@ >> #include "lflow.h" >> #include "lib/mac-binding-index.h" >> #include "lib/vec.h" >> +#include "lib/oftable.h" >> #include "local_data.h" >> #include "lport.h" >> #include "mac-cache.h" >> diff --git a/controller/statctrl.c b/controller/statctrl.c >> index 454314dda..d84274834 100644 >> --- a/controller/statctrl.c >> +++ b/controller/statctrl.c >> @@ -20,6 +20,7 @@ >> #include "latch.h" >> #include "lflow.h" >> #include "lib/vec.h" >> +#include "lib/oftable.h" >> #include "mac-cache.h" >> #include "openvswitch/ofp-errors.h" >> #include "openvswitch/ofp-flow.h" >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h >> index 0eaef9112..82d128f33 100644 >> --- a/include/ovn/actions.h >> +++ b/include/ovn/actions.h >> @@ -27,6 +27,7 @@ >> #include "openvswitch/uuid.h" >> #include "util.h" >> #include "ovn/features.h" >> +#include "oftable.h" >> struct expr; >> struct lexer; >> diff --git a/lib/automake.mk b/lib/automake.mk >> index a59c722d6..16baeb157 100644 >> --- a/lib/automake.mk >> +++ b/lib/automake.mk >> @@ -10,6 +10,8 @@ lib_libovn_la_LDFLAGS += $(VIF_PLUG_PROVIDER_LDFLAGS) >> endif >> lib_libovn_la_SOURCES = \ >> + lib/oftable.c \ >> + lib/oftable.h \ >> lib/acl-log.c \ >> lib/acl-log.h \ >> lib/actions.c \ >> @@ -22,8 +24,8 @@ lib_libovn_la_SOURCES = \ >> lib/extend-table.h \ >> lib/extend-table.c \ >> lib/features.c \ >> - lib/ovn-parallel-hmap.h \ >> lib/ovn-parallel-hmap.c \ >> + lib/ovn-parallel-hmap.h \ >> lib/ip-mcast-index.c \ >> lib/ip-mcast-index.h \ >> lib/mac-binding-index.c \ >> diff --git a/lib/oftable.c b/lib/oftable.c >> new file mode 100644 >> index 000000000..721b3ad49 >> --- /dev/null >> +++ b/lib/oftable.c >> @@ -0,0 +1,55 @@ >> +/* Copyright (c) 2015, 2016 Nicira, Inc. >> + * >> + * Licensed under the Apache License, Version 2.0 (the "License"); >> + * you may not use this file except in compliance with the License. >> + * You may obtain a copy of the License at: >> + * >> + * http://www.apache.org/licenses/LICENSE-2.0 >> + * >> + * Unless required by applicable law or agreed to in writing, software >> + * distributed under the License is distributed on an "AS IS" BASIS, >> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >> implied. >> + * See the License for the specific language governing permissions and >> + * limitations under the License. >> + */ >> +#include <config.h> >> + >> +#include "lib/oftable.h" >> + >> +int OFTABLE_PHY_TO_LOG = 0; >> + >> +/* Start of LOG_PIPELINE_LEN tables. */ >> +int OFTABLE_LOG_INGRESS_PIPELINE = 8; >> +int OFTABLE_OUTPUT_LARGE_PKT_DETECT = 41, OFTABLE_OUTPUT_INIT = 41; >> +int OFTABLE_OUTPUT_LARGE_PKT_PROCESS = 42; >> +int OFTABLE_REMOTE_OUTPUT = 43; >> +int OFTABLE_REMOTE_VTEP_OUTPUT = 44; >> +int OFTABLE_LOCAL_OUTPUT = 45; >> +int OFTABLE_CHECK_LOOPBACK = 46; >> + >> +/* Start of LOG_PIPELINE_LEN tables. */ >> +int OFTABLE_LOG_EGRESS_PIPELINE = 47; >> +int OFTABLE_SAVE_INPORT = 64; >> +int OFTABLE_LOG_TO_PHY = 65; >> +int OFTABLE_MAC_BINDING = 66; >> +int OFTABLE_MAC_LOOKUP = 67; >> +int OFTABLE_CHK_LB_HAIRPIN = 68; >> +int OFTABLE_CHK_LB_HAIRPIN_REPLY = 69; >> +int OFTABLE_CT_SNAT_HAIRPIN = 70; >> +int OFTABLE_GET_FDB = 71; >> +int OFTABLE_LOOKUP_FDB = 72; >> +int OFTABLE_CHK_IN_PORT_SEC = 73; >> +int OFTABLE_CHK_IN_PORT_SEC_ND = 74; >> +int OFTABLE_CHK_OUT_PORT_SEC = 75; >> +int OFTABLE_ECMP_NH_MAC = 76; >> +int OFTABLE_ECMP_NH = 77; >> +int OFTABLE_CHK_LB_AFFINITY = 78; >> +int OFTABLE_MAC_CACHE_USE = 79; >> +int OFTABLE_CT_ZONE_LOOKUP = 80; >> +int OFTABLE_CT_ORIG_NW_DST_LOAD = 81; >> +int OFTABLE_CT_ORIG_IP6_DST_LOAD = 82; >> +int OFTABLE_CT_ORIG_TP_DST_LOAD = 83; >> +int OFTABLE_FLOOD_REMOTE_CHASSIS = 84; >> +int OFTABLE_CT_STATE_SAVE = 85; >> +int OFTABLE_CT_ORIG_PROTO_LOAD = 86; >> +int OFTABLE_GET_REMOTE_FDB = 87; >> diff --git a/lib/oftable.h b/lib/oftable.h >> new file mode 100644 >> index 000000000..54425964d >> --- /dev/null >> +++ b/lib/oftable.h >> @@ -0,0 +1,58 @@ >> +/* Copyright (c) 2015, 2016 Nicira, Inc. >> + * >> + * Licensed under the Apache License, Version 2.0 (the "License"); >> + * you may not use this file except in compliance with the License. >> + * You may obtain a copy of the License at: >> + * >> + * http://www.apache.org/licenses/LICENSE-2.0 >> + * >> + * Unless required by applicable law or agreed to in writing, software >> + * distributed under the License is distributed on an "AS IS" BASIS, >> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >> implied. >> + * See the License for the specific language governing permissions and >> + * limitations under the License. >> + */ >> +#include <stdint.h> >> + >> +/* OpenFlow table numbers. >> + * >> + * These are heavily documented in ovn-architecture(7), please >> update it if >> + * you make any changes. */ >> +extern int OFTABLE_PHY_TO_LOG; >> + >> +/* Start of LOG_PIPELINE_LEN tables. */ >> +extern int OFTABLE_LOG_INGRESS_PIPELINE; >> +extern int OFTABLE_OUTPUT_LARGE_PKT_DETECT; >> +extern int OFTABLE_OUTPUT_INIT; >> +extern int OFTABLE_OUTPUT_LARGE_PKT_PROCESS; >> +extern int OFTABLE_REMOTE_OUTPUT; >> +extern int OFTABLE_REMOTE_VTEP_OUTPUT; >> +extern int OFTABLE_LOCAL_OUTPUT; >> +extern int OFTABLE_CHECK_LOOPBACK; >> + >> +/* Start of LOG_PIPELINE_LEN tables. */ >> +extern int OFTABLE_LOG_EGRESS_PIPELINE; >> +extern int OFTABLE_SAVE_INPORT; >> +extern int OFTABLE_LOG_TO_PHY; >> +extern int OFTABLE_MAC_BINDING; >> +extern int OFTABLE_MAC_LOOKUP; >> +extern int OFTABLE_CHK_LB_HAIRPIN; >> +extern int OFTABLE_CHK_LB_HAIRPIN_REPLY; >> +extern int OFTABLE_CT_SNAT_HAIRPIN; >> +extern int OFTABLE_GET_FDB; >> +extern int OFTABLE_LOOKUP_FDB; >> +extern int OFTABLE_CHK_IN_PORT_SEC; >> +extern int OFTABLE_CHK_IN_PORT_SEC_ND; >> +extern int OFTABLE_CHK_OUT_PORT_SEC; >> +extern int OFTABLE_ECMP_NH_MAC; >> +extern int OFTABLE_ECMP_NH; >> +extern int OFTABLE_CHK_LB_AFFINITY; >> +extern int OFTABLE_MAC_CACHE_USE; >> +extern int OFTABLE_CT_ZONE_LOOKUP; >> +extern int OFTABLE_CT_ORIG_NW_DST_LOAD; >> +extern int OFTABLE_CT_ORIG_IP6_DST_LOAD; >> +extern int OFTABLE_CT_ORIG_TP_DST_LOAD; >> +extern int OFTABLE_FLOOD_REMOTE_CHASSIS; >> +extern int OFTABLE_CT_STATE_SAVE; >> +extern int OFTABLE_CT_ORIG_PROTO_LOAD; >> +extern int OFTABLE_GET_REMOTE_FDB; >> diff --git a/lib/ovn-util.c b/lib/ovn-util.c >> index 8b583fa6d..47922276e 100644 >> --- a/lib/ovn-util.c >> +++ b/lib/ovn-util.c >> @@ -39,6 +39,9 @@ VLOG_DEFINE_THIS_MODULE(ovn_util); >> #define DEFAULT_PROBE_INTERVAL_MSEC 5000 >> +int LOG_PIPELINE_INGRESS_LEN = 32; >> +int LOG_PIPELINE_EGRESS_LEN = 14; >> + >> void ovn_conn_show(struct unixctl_conn *conn, int argc OVS_UNUSED, >> const char *argv[] OVS_UNUSED, void *idl_) >> { >> diff --git a/lib/ovn-util.h b/lib/ovn-util.h >> index 63beae3e5..abda66963 100644 >> --- a/lib/ovn-util.h >> +++ b/lib/ovn-util.h >> @@ -320,8 +320,8 @@ BUILD_ASSERT_DECL( >> #define SCTP_ABORT_CHUNK_FLAG_T (1 << 0) >> /* The number of tables for the ingress and egress pipelines. */ >> -#define LOG_PIPELINE_INGRESS_LEN 32 >> -#define LOG_PIPELINE_EGRESS_LEN 14 >> +extern int LOG_PIPELINE_INGRESS_LEN; >> +extern int LOG_PIPELINE_EGRESS_LEN; >> static inline uint32_t >> hash_add_in6_addr(uint32_t hash, const struct in6_addr *addr) >> diff --git a/utilities/ovn-debug.c b/utilities/ovn-debug.c >> index 0a0d2202b..64d2b8bab 100644 >> --- a/utilities/ovn-debug.c >> +++ b/utilities/ovn-debug.c >> @@ -21,6 +21,7 @@ >> #include "controller/lflow.h" >> #include "northd/northd.h" >> #include "ovn-util.h" >> +#include "lib/oftable.h" >> struct ovn_lflow_stage { >> const char *name; > -- regards, Alexandra. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
