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

Fix syncEgressFirewall (truncate ACL names) and SetupMaster (stop recreating cluster-wide PGs) #3466

Merged
merged 3 commits into from
Mar 7, 2023
Merged

Fix syncEgressFirewall (truncate ACL names) and SetupMaster (stop recreating cluster-wide PGs) #3466

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d996d0e
Add test to showcase syncEgressFirewall isn't truncating ACL names
tssurya Mar 6, 2023
7bfe50e
Fix syncEgressFirewall to truncate ACL names
tssurya Mar 6, 2023
935bc55
Don't recreate clusterPGs and clusterRtrPGs unless needed
tssurya Mar 7, 2023
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
45 changes: 33 additions & 12 deletions go-controller/pkg/ovn/master.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"k8s.io/klog/v2"
utilnet "k8s.io/utils/net"

libovsdbclient "github.com/ovn-org/libovsdb/client"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/kube"
"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/libovsdbops"
Expand Down Expand Up @@ -118,23 +119,43 @@ func (oc *DefaultNetworkController) SetupMaster(existingNodeNames []string) erro
}
oc.defaultCOPPUUID = *(logicalRouter.Copp)

// Create a cluster-wide port group that all logical switch ports are part of
pg := libovsdbops.BuildPortGroup(types.ClusterPortGroupName, types.ClusterPortGroupName, nil, nil)
err = libovsdbops.CreateOrUpdatePortGroups(oc.nbClient, pg)
if err != nil {
klog.Errorf("Failed to create cluster port group: %v", err)
pg := &nbdb.PortGroup{
Name: types.ClusterPortGroupName,
}
pg, err = libovsdbops.GetPortGroup(oc.nbClient, pg)
if err != nil && err != libovsdbclient.ErrNotFound {
return err
}
if pg == nil {
// we didn't find an existing clusterPG, let's create a new empty PG (fresh cluster install)
// Create a cluster-wide port group that all logical switch ports are part of
pg := libovsdbops.BuildPortGroup(types.ClusterPortGroupName, types.ClusterPortGroupName, nil, nil)
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure, but cleaning up ports from this port group even when it exists may be intended, since there is no cleanup for ports AFAIK. So we may need some new sync function for nodes to delete possibly stale ports?

Copy link
Member Author

@tssurya tssurya Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yea that was also a concern I had but couldn't really find if this was intended or something that we just did. I hope the ACLs are synced elsewhere in each of these features.

I'd Hope when node add/dels happen we do remove these ports from these port groups. Also these mp0 ports and router ports are not supposed to change that frequently in real envs.
Regardless a cleanup on syncNodes might be a good part for syncing things when nodes come up and ensuring we are keeping a clean list? -> since risk is less I could do this in a FUP if it makes sense
@trozet -> WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

we saw delete node will remove the LSP from the port group so the chances of leaking are small and shouldn't have meaningful impact

Copy link
Member Author

Choose a reason for hiding this comment

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

@trozet checked a node deletion, we do remove k8s* ports when a node goes away, that automatically removes it from the PG. So we are good here.

err = libovsdbops.CreateOrUpdatePortGroups(oc.nbClient, pg)
if err != nil {
klog.Errorf("Failed to create cluster port group: %v", err)
return err
}
}

// Create a cluster-wide port group with all node-to-cluster router
// logical switch ports. Currently the only user is multicast but it might
// be used for other features in the future.
pg = libovsdbops.BuildPortGroup(types.ClusterRtrPortGroupName, types.ClusterRtrPortGroupName, nil, nil)
err = libovsdbops.CreateOrUpdatePortGroups(oc.nbClient, pg)
if err != nil {
klog.Errorf("Failed to create cluster port group: %v", err)
pg = &nbdb.PortGroup{
Name: types.ClusterRtrPortGroupName,
}
pg, err = libovsdbops.GetPortGroup(oc.nbClient, pg)
if err != nil && err != libovsdbclient.ErrNotFound {
return err
}
if pg == nil {
// we didn't find an existing clusterRtrPG, let's create a new empty PG (fresh cluster install)
// Create a cluster-wide port group with all node-to-cluster router
// logical switch ports. Currently the only user is multicast but it might
// be used for other features in the future.
pg = libovsdbops.BuildPortGroup(types.ClusterRtrPortGroupName, types.ClusterRtrPortGroupName, nil, nil)
err = libovsdbops.CreateOrUpdatePortGroups(oc.nbClient, pg)
if err != nil {
klog.Errorf("Failed to create cluster port group: %v", err)
return err
}
}

// If supported, enable IGMP relay on the router to forward multicast
// traffic between nodes.
Expand Down