Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upgrade changes for OVN IC #2656

Merged
merged 1 commit into from
Aug 30, 2023
Merged

Upgrade changes for OVN IC #2656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/routeagent_driver/handlers/ovn/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ func (ovn *Handler) GetNetworkPlugins() []string {
}

func (ovn *Handler) Init() error {
ovn.LegacyCleanup()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LegacyCleanup is deleting the ovn-k8s-sub0 and br-submariner interfaces.
What about the flows like Logical Router policies that are programmed in the ovn_cluster_router and submariner_router ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aswinsuryan Can correct if my understanding is wrong, but those exist only on node where NetworkPluginSyncer was running and will be removed as part of Cleanup when NetworkPluginSyncer is "stopped" as part of upgrade process.

This LegacyCleanup is for resources that are created on all nodes as part of RouteAgent and is not covered by NetowrkPluginSyncer's cleanup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya that is right , the changes done here submariner-io/submariner-operator#2759 , will delete any networkplugin-syncer deployment if found and that should cleanup the routes added to ovn routers.

Copy link
Contributor

@tpantelis tpantelis Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aswinsuryan Are you assuming the operator will initiate the uninstall process for the networkplugin-syncer? If so, this is not (currently) the case - the operator just deletes the networkplugin-syncer deployment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ok , I was thinking on delete Stop would be called. If that is not the case we will have to add it here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also it is not sure if OVN is going to preserve these flows that submariner have created when it changes from non-ic to ic, as it is not OVN created flows.

Good point. We have to understand how OVN-IC upgrade happens, whether it will read the etcd and change the state of the system to match it or if it uses the info from the NBDB/SBDB.

We may have to test it to see the state after an upgrade.

Normally for a regular linux interface, when its deleted the associated routes/ips on that interface are deleted. In case of OVN, when br-submariner is deleted, does the associated openflow rules get deleted or remain as stale rules?

Another way is to use a logic similar to here on startup to delete the flows. Like if the CIDR and priority matches but nethop does not delete the flow.

Are you suggesting that we do this while programming the required flows or as part of init() operation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so it sounds like you think we should do whatever legacy NPS cleanup is needed here.

Yes that is right

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also it is not sure if OVN is going to preserve these flows that submariner have created when it changes from non-ic to ic, as it is not OVN created flows.

Good point. We have to understand how OVN-IC upgrade happens, whether it will read the etcd and change the state of the system to match it or if it uses the info from the NBDB/SBDB.

We may have to test it to see the state after an upgrade.

Normally for a regular linux interface, when its deleted the associated routes/ips on that interface are deleted. In case of OVN, when br-submariner is deleted, does the associated openflow rules get deleted or remain as stale rules?

Ya right it should get deleted when we remove ovn-k8s-sub0

Another way is to use a logic similar to here on startup to delete the flows. Like if the CIDR and priority matches but nethop does not delete the flow.

Are you suggesting that we do this while programming the required flows or as part of init() operation?

As a part of init().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I gave more thought on it , I think calling it from nps may not work too. If the OVN changes from non-IC to IC , nps cannot connect to ovn db the way it does now . Also it is not sure if OVN is going to preserve these flows that submariner have created when it changes from non-ic to ic, as it is not OVN created flows.

Is that a valid upgrade path? Upgrading from non-ic to ic before upgrading means deployment will be using an unsupported deployment of Submariner [< 0.16 with ic]. Proper way to upgrade should be to upgrade Submariner to 0.16+ before upgrading to IC coz 0.16 is only one that supports IC and non IC both.

We may have to test it to see the state after an upgrade.

But for a submariner upgrade for Opeshift < 4.14 this would work.

So, requiring a Submariner upgrade before upgrading OCP should take care of it. I am sure other projects will also have a similar requirement, upgrade them before switching OCP tp 4.14 coz older code will likely be incompatible with OCP 4.14.

Another way is to use a logic similar to here on startup to delete the flows. Like if the CIDR and priority matches but nethop does not delete the flow.

https://github.com/submariner-io/submariner/blob/devel/pkg/routeagent_driver/handlers/ovn/ovn_logical_routes.go#L78

Will this be still needed if we require submariner upgrade before OCP?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needed if we require submariner upgrade before OCP?

This should solve the cleanup issue if we add the code to remove flows during netwrok plugin uninstall. However the operator needs to discover the CNI again to move from non-IC to IC. Currently, we do not support on-the-fly change from non-ic mode to IC mode.


err := ovn.initIPtablesChains()
if err != nil {
return err
Expand Down
2 changes: 1 addition & 1 deletion pkg/routeagent_driver/handlers/ovn/vsctl/vsctl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func AddBridge(bridgeName string) error {
}

func DelBridge(bridgeName string) error {
_, err := vsctlCmd("del-br", bridgeName)
_, err := vsctlCmd("--if-exists", "del-br", bridgeName)

return err
}
Expand Down
Loading