Skip to content

GitLab

  • Projects
  • Groups
  • Snippets
  • Help
    • Loading...
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Sign in / Register
Kea
Kea
  • Project overview
    • Project overview
    • Details
    • Activity
    • Releases
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
  • Issues 457
    • Issues 457
    • List
    • Boards
    • Labels
    • Service Desk
    • Milestones
  • Merge Requests 75
    • Merge Requests 75
  • Operations
    • Operations
    • Incidents
  • Packages & Registries
    • Packages & Registries
    • Container Registry
  • Analytics
    • Analytics
    • Repository
    • Value Stream
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Members
    • Members
  • Activity
  • Graph
  • Create a new issue
  • Commits
  • Issue Boards
Collapse sidebar
  • ISC Open Source Projects
  • KeaKea
  • Issues
  • #1458

Closed
Open
Created Oct 07, 2020 by Razvan Becheriu@razvanDeveloper

suboptimal retrieval of host reservation in the case of db backends

in void AllocEngine::findReservation(ClientContext[4|6]& ctx)

void
AllocEngine::findReservation(ClientContext[4|6]& ctx) {
    ctx.hosts_.clear();

    // If there is no subnet, there is nothing to do.
    if (!ctx.subnet_) {
        return;
    }

    auto subnet = ctx.subnet_;

    std::map<SubnetID, ConstHostPtr> host_map;
    SharedNetwork[4|6]Ptr network;
    subnet->getSharedNetwork(network);

    if (subnet->getHostReservationMode() == Network::HR_GLOBAL) {
        ConstHostPtr ghost = findGlobalReservation(ctx);
        if (ghost) {
            ctx.hosts_[SUBNET_ID_GLOBAL] = ghost;

            // @todo In theory, to support global as part of HR_ALL,
            //  we would just keep going, instead of returning.
            return;
        }
    }

    // If the subnet belongs to a shared network it is usually going to be
    // more efficient to make a query for all reservations for a particular
    // client rather than a query for each subnet within this shared network.
    // The only case when it is going to be less efficient is when there are
    // more host identifier types in use than subnets within a shared network.
    // As it breaks RADIUS use of host caching this can be disabled by the
    // host manager.
    const bool use_single_query = network &&
        !HostMgr::instance().getDisableSingleQuery() &&
        (network->getAllSubnets()->size() > ctx.host_identifiers_.size());

    if (use_single_query) {
        for (auto id_pair : ctx.host_identifiers_) {
            ConstHostCollection hosts = HostMgr::instance().getAll(id_pair.first,
                                                                   &id_pair.second[0],
                                                                   id_pair.second.size());
            // Store the hosts in the temporary map, because some hosts may
            // belong to subnets outside of the shared network. We'll need
            // to eliminate them.
            for (auto host = hosts.begin(); host != hosts.end(); ++host) {
                if ((*host)->getIPv[4|6]SubnetID()) {
                    host_map[(*host)->getIPv[4|6]SubnetID()] = *host;
                }
            }
        }
    }

    // We can only search for the reservation if a subnet has been selected.
    while (subnet) {

        // Only makes sense to get reservations if the client has access
        // to the class and host reservations are enabled.
        if (subnet->clientSupported(ctx.query_->getClasses()) &&
            (subnet->getHostReservationMode() != Network::HR_DISABLED)) {
            // Iterate over configured identifiers in the order of preference
            // and try to use each of them to search for the reservations.
            for (auto id_pair : ctx.host_identifiers_) {
                if (use_single_query) {
                    if (host_map.count(subnet->getID()) > 0) {
                        ctx.hosts_[subnet->getID()] = host_map[subnet->getID()];
                        break;
                    }
                } else {
                    // Attempt to find a host using a specified identifier.
                    ConstHostPtr host = HostMgr::instance().get[4|6](subnet->getID(),
                                                                 id_pair.first,
                                                                 &id_pair.second[0],
                                                                 id_pair.second.size());
                    // If we found matching host for this subnet.
                    if (host) {
                        ctx.hosts_[subnet->getID()] = host;
                        break;
                    }
                }
            }
        }

        // We need to get to the next subnet if this is a shared network. If it
        // is not (a plain subnet), getNextSubnet will return NULL and we're
        // done here.
        subnet = subnet->getNextSubnet(ctx.subnet_, ctx.query_->getClasses());
    }
}

vs

void
AllocEngine::findReservation(ClientContext[4|6]& ctx) {
    ctx.hosts_.clear();

    // If there is no subnet, there is nothing to do.
    if (!ctx.subnet_) {
        return;
    }

    auto subnet = ctx.subnet_;

    std::map<SubnetID, ConstHostPtr> host_map;
    SharedNetwork[4|6]Ptr network;
    subnet->getSharedNetwork(network);

    if (subnet->getHostReservationMode() == Network::HR_GLOBAL) {
        ConstHostPtr ghost = findGlobalReservation(ctx);
        if (ghost) {
            ctx.hosts_[SUBNET_ID_GLOBAL] = ghost;

            // @todo In theory, to support global as part of HR_ALL,
            //  we would just keep going, instead of returning.
            return;
        }
    }

    // If the subnet belongs to a shared network it is usually going to be
    // more efficient to make a query for all reservations for a particular
    // client rather than a query for each subnet within this shared network.
    // The only case when it is going to be less efficient is when there are
    // more host identifier types in use than subnets within a shared network.
    // As it breaks RADIUS use of host caching this can be disabled by the
    // host manager.
***
    // Can not call getAll[4|6] because RADIUS does not support it.
    // While the performance is acceptable for RADIUS and CfgHosts, doing
    // network->getAllSubnets()->size() * ctx.host_identifiers_.size() for
    // db backends is suboptimal, as getAll[4|6] is supported.
    // This results in
    // network->getAllSubnets()->size() * ctx.host_identifiers_.size()
    // roundtrips vs network->getAllSubnets()->size() number of roundtrips
    // Should this function be implemented as a Host virtual function?
***
    const bool use_single_query = network &&
        !HostMgr::instance().getDisableSingleQuery() &&
        (network->getAllSubnets()->size() > ctx.host_identifiers_.size());

    if (use_single_query) {
        for (auto id_pair : ctx.host_identifiers_) {
            ConstHostCollection hosts = HostMgr::instance().getAll(id_pair.first,
                                                                   &id_pair.second[0],
                                                                   id_pair.second.size());
            // Store the hosts in the temporary map, because some hosts may
            // belong to subnets outside of the shared network. We'll need
            // to eliminate them.
            for (auto host = hosts.begin(); host != hosts.end(); ++host) {
                if ((*host)->getIPv[4|6]SubnetID()) {
                    host_map[(*host)->getIPv[4|6]SubnetID()] = *host;
                }
            }
        }
    }

    // We can only search for the reservation if a subnet has been selected.
    while (subnet) {

        // Only makes sense to get reservations if the client has access
        // to the class and host reservations are enabled.
        if (subnet->clientSupported(ctx.query_->getClasses()) &&
            (subnet->getHostReservationMode() != Network::HR_DISABLED)) {
            // Iterate over configured identifiers in the order of preference
            // and try to use each of them to search for the reservations.
            ConstHostCollection subnet_hosts;
            if (!use_single_query) {
                subnet_hosts = HostMgr::instance().getAll[4|6](subnet->getID());
            }
            for (auto id_pair : ctx.host_identifiers_) {
                if (use_single_query) {
                    if (host_map.count(subnet->getID()) > 0) {
                        ctx.hosts_[subnet->getID()] = host_map[subnet->getID()];
                    }
                } else {
                    ConstHostPtr host;
                    for (auto current : subnet_hosts) {
                        if (current->getIdentifierType() != id_pair.first) {
                            continue;
                        }
                        if (current->getIdentifier() != id_pair.second) {
                            continue;
                        }
                        host = current;
                        break;
                    }
                    // If we found matching host for this subnet.
                    if (host) {
                        ctx.hosts_[subnet->getID()] = host;
                        break;
                    }
                }
            }
        }

        // We need to get to the next subnet if this is a shared network. If it
        // is not (a plain subnet), getNextSubnet will return NULL and we're
        // done here.
        subnet = subnet->getNextSubnet(ctx.subnet_, ctx.query_->getClasses());
    }
}

Things to note:

  1. the following comment is misleading:
    // If the subnet belongs to a shared network it is usually going to be
    // more efficient to make a query for all reservations for a particular
    // client rather than a query for each subnet within this shared network.
    // The only case when it is going to be less efficient is when there are
    // more host identifier types in use than subnets within a shared network.
    // As it breaks RADIUS use of host caching this can be disabled by the
    // host manager.
  1. the following check is wrong, because it uses the slow path for db backends when using global subnets:
    const bool use_single_query = network &&
        !HostMgr::instance().getDisableSingleQuery() &&
        (network->getAllSubnets()->size() > ctx.host_identifiers_.size());

because of the RADIUS limitation, there are still network->getAllSubnets()->size() * ctx.host_identifiers_.size() calls. This is optimal for CfgHosts and it is a limitation for RADIUS, but it is very bad on db backends as it does a lot of roundtrips (slower than actual in RAM filtering by calling getAll[4|6] and keeping hosts with matching identifiers).

Because there is no easy way to optimize this, maybe this function can be implemented as a virtual function in each backend: CfgHosts and Radius can use current implementation by calling get[4|6] for each subnet and for each identifier type, and mysql, postgresql and cassandra can call getAll[4|6] for each subnet and filter the identifiers in ram (effectively lowering the number of roundtrips).

It can also be implemented by adding a flag just like in case of RADIUS, 'getDisableSingleQuery', like 'getAllQueryEnabled', but , by using this approach, there is no way RADIUS can function with a db backend in an optimal way.

Edited Oct 07, 2020 by Razvan Becheriu
To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information
Assignee
Assign to
kea1.9.4
Milestone
kea1.9.4 (Past due)
Assign milestone
Time tracking
None
Due date
None