Skip to content

✨ Delete router/network/subnet #522

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.

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

Merged
merged 1 commit into from
Mar 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 18 additions & 1 deletion controllers/openstackcluster_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,23 @@ func (r *OpenStackClusterReconciler) reconcileDelete(ctx context.Context, log lo
}
}

// TODO(sbueringer) Delete network/subnet/router/... if created by CAPO
if openStackCluster.Status.Network.Router != nil {
log.Info("Deleting router", "name", openStackCluster.Status.Network.Router.Name)
if err := networkingService.DeleteRouter(openStackCluster.Status.Network); err != nil {
return ctrl.Result{}, errors.Errorf("failed to delete router: %v", err)
}
log.Info("OpenStack router deleted successfully")
}

if openStackCluster.Status.Network != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember clearly, do we have a chance that the network/router is not created by CPAO?
others LGTM

Copy link
Member

Choose a reason for hiding this comment

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

I think it's okay as the cluster network/subnet are afaik always created by us

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, I didn't remember clearly, will try this later on and submit PR if need :)

log.Info("Deleting network", "name", openStackCluster.Status.Network.Name)
if err := networkingService.DeleteNetwork(openStackCluster.Status.Network); err != nil {
return ctrl.Result{}, errors.Errorf("failed to delete network: %v", err)
}
log.Info("OpenStack network deleted successfully")
}

log.Info("OpenStack cluster deleted successfully")

// Cluster is deleted so remove the finalizer.
controllerutil.RemoveFinalizer(openStackCluster, infrav1.ClusterFinalizer)
Expand Down Expand Up @@ -297,6 +313,7 @@ func (r *OpenStackClusterReconciler) reconcileNormal(ctx context.Context, log lo
}

openStackCluster.Status.Ready = true
log.Info("Reconciled Cluster create successfully")
return ctrl.Result{}, nil
}

Expand Down
30 changes: 30 additions & 0 deletions pkg/cloud/services/networking/network.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,22 @@ func (s *Service) ReconcileNetwork(clusterName string, openStackCluster *infrav1
return nil
}

func (s *Service) DeleteNetwork(network *infrav1.Network) error {
if network == nil || network.ID == "" {
s.logger.V(4).Info("No need to delete network since no network exists.")
return nil
}
exists, err := s.existsNetwork(network.ID)
if err != nil {
return err
}
if !exists {
s.logger.Info("Skipping network deletion because network doesn't exist", "network", network.ID)
return nil
}
return networks.Delete(s.client, network.ID).ExtractErr()
}

func (s *Service) ReconcileSubnet(clusterName string, openStackCluster *infrav1.OpenStackCluster) error {

if openStackCluster.Status.Network == nil || openStackCluster.Status.Network.ID == "" {
Expand Down Expand Up @@ -188,3 +204,17 @@ func (s *Service) getNetworkByName(networkName string) (networks.Network, error)
}
return networks.Network{}, errors.New("too many resources")
}

func (s *Service) existsNetwork(networkID string) (bool, error) {
if networkID == "" {
return false, nil
}
network, err := networks.Get(s.client, networkID).Extract()
if err != nil {
return false, err
}
if network == nil {
return false, nil
}
return true, nil
}
41 changes: 41 additions & 0 deletions pkg/cloud/services/networking/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,33 @@ INTERFACE_LOOP:
return nil
}

func (s *Service) DeleteRouter(network *infrav1.Network) error {
if network.Router == nil || network.Router.ID == "" {
s.logger.V(4).Info("No need to delete router since no router exists.")
return nil
}
exists, err := s.existsRouter(network.Router.ID)
Copy link
Member

@sbueringer sbueringer Mar 25, 2020

Choose a reason for hiding this comment

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

I think you still have to check for empty routerID as in you're previous commit. The problem is if the ID is an empty string
and you list all routers in existsRouter without an ID you get all existing routers (same for network)

EDIT: Just looked it up. I would suggest using routers.Get (& networks.Get) instead. Should be even save with empty ID and just returns 0 or 1 router not all so should be more efficient. Although I would still check for the empty string before and then checking for the existence with get

if err != nil {
return err
}
if !exists {
s.logger.Info("Skipping router deletion because router doesn't exist", "router", network.Router.ID)
return nil
}
if network.Subnet == nil || network.Subnet.ID == "" {
s.logger.V(4).Info("Skipping removing router interface since no subnet exists.")
} else {
_, err = routers.RemoveInterface(s.client, network.Router.ID, routers.RemoveInterfaceOpts{
SubnetID: network.Subnet.ID,
}).Extract()
if err != nil {
return fmt.Errorf("unable to remove router interface: %v", err)
}
s.logger.V(4).Info("Removed RouterInterface of Router", "id", network.Router.ID)
}
return routers.Delete(s.client, network.Router.ID).ExtractErr()
}

func (s *Service) getRouterInterfaces(routerID string) ([]ports.Port, error) {
allPages, err := ports.List(s.client, ports.ListOpts{
DeviceID: routerID,
Expand Down Expand Up @@ -247,3 +274,17 @@ func GetSubnetsByFilter(networkClient *gophercloud.ServiceClient, opts subnets.L
}
return snets, nil
}

func (s *Service) existsRouter(routerID string) (bool, error) {
if routerID == "" {
return false, nil
}
router, err := routers.Get(s.client, routerID).Extract()
if err != nil {
return false, err
}
if router == nil {
return false, nil
}
return true, nil
}