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.

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

Fixes #20716: Improve dynamic group computation speed #4155

Closed
Show file tree
Hide file tree
Changes from 1 commit
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
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,31 @@ final case object NodeStateComparator extends NodeCriterionType {
}


final case object NodeStringComparator extends NodeCriterionType {
Copy link
Member Author

Choose a reason for hiding this comment

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

I should make it a case class, with a parameter telling on which attribute of nodeinfo we should search

Copy link
Member Author

Choose a reason for hiding this comment

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

However, the big trouble here is that it breaks the detection on which group a pending node will appear, as the pending node is not in cache

Copy link
Member Author

Choose a reason for hiding this comment

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

and properties are not correctly used to detect in which group the node will be

override val comparators = BaseComparators.comparators

override protected def validateSubCase(v: String, comparator: CriterionComparator) = {
if(null == v || v.isEmpty) Left(Inconsistency("Empty string not allowed")) else {
comparator match {
case Regex | NotRegex => validateRegex(v)
case x => Right(v)
}
}
}

override def matches(comparator: CriterionComparator, value: String): NodeInfoMatcher = {
comparator match {
case Equals => NodeInfoMatcher(s"Prop equals '${value}'", (node: NodeInfo) => node.hostname == value )
case NotEquals => NodeInfoMatcher(s"Prop not equals '${value}'", (node: NodeInfo) => node.hostname != value )
case Regex => NodeInfoMatcher(s"Prop matches regex '${value}'", (node: NodeInfo) => node.hostname.matches(value) )
case NotRegex => NodeInfoMatcher(s"Prop matches not regex '${value}'", (node: NodeInfo) => !node.hostname.matches(value) )
case Exists => NodeInfoMatcher(s"Prop exists", (node: NodeInfo) => node.hostname.nonEmpty )
case NotExists => NodeInfoMatcher(s"Prop doesn't exists", (node: NodeInfo) => node.hostname.isEmpty )

}
}

}

/*
* This comparator is used for "node properties"-like attribute, i.e:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ class DitQueryData(dit: InventoryDit, nodeDit: NodeDit, rudderDit: RudderDit, ge
ObjectCriterion(OC_NODE, Seq(
Criterion("OS",OstypeComparator)
, Criterion(A_NODE_UUID, StringComparator)
, Criterion(A_HOSTNAME, StringComparator)
, Criterion(A_HOSTNAME, NodeStringComparator)
, Criterion(A_OS_NAME,OsNameComparator)
, Criterion(A_OS_FULL_NAME, OrderedStringComparator)
, Criterion(A_OS_VERSION, OrderedStringComparator)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,10 @@ trait NodeInfoServiceCached extends NodeInfoService with NamedZioLogger with Cac
)
}

def getAllNodesEntry() = {
Copy link
Member Author

Choose a reason for hiding this comment

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

i'm not reall proud of this one

nodeCache.map(x => x.nodeInfos.values.toSeq.map(_._1.nodeEntry))
}

/**
* Update cache, without doing anything with the data
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
package com.normation.rudder.services.queries

import java.util.regex.Pattern

import com.normation.inventory.domain._
import com.normation.inventory.ldap.core.LDAPConstants._
import com.normation.inventory.ldap.core._
Expand All @@ -47,8 +46,7 @@ import com.normation.rudder.domain._
import com.normation.rudder.domain.nodes.NodeInfo
import com.normation.rudder.domain.queries._
import com.normation.rudder.repository.ldap.LDAPEntityMapper
import com.normation.rudder.services.nodes.LDAPNodeInfo
import com.normation.rudder.services.nodes.NodeInfoService
import com.normation.rudder.services.nodes.{LDAPNodeInfo, NodeInfoService, NodeInfoServiceCached}
import com.normation.utils.Control.sequence
import com.unboundid.ldap.sdk.DereferencePolicy.NEVER
import com.unboundid.ldap.sdk.{LDAPConnection => _, SearchScope => _, _}
Expand Down Expand Up @@ -111,6 +109,8 @@ final case class LDAPNodeQuery(
//that map MUST not contains node related filters
, objectTypesFilters: Map[DnType, Map[String, List[SubQuery]]]
, nodeInfoFilters : Seq[NodeInfoMatcher]
, noFilterButTakeAllFromCache: Boolean // this is when we don't have ldapfilter, but only nodeinfo filter, so we
// need to take eveything from cache
)

/*
Expand Down Expand Up @@ -148,7 +148,7 @@ class AcceptedNodesLDAPQueryProcessor(
nodeDit : NodeDit
, inventoryDit : InventoryDit
, processor : InternalLDAPQueryProcessor
, nodeInfoService: NodeInfoService
, nodeInfoService: NodeInfoServiceCached
) extends QueryProcessor with Loggable {

private[this] case class QueryResult(
Expand All @@ -174,7 +174,7 @@ class AcceptedNodesLDAPQueryProcessor(
val timePreCompute = System.currentTimeMillis

for {
res <- processor.internalQueryProcessor(query,select,limitToNodeIds,debugId).toBox
res <- processor.internalQueryProcessor(query,select,limitToNodeIds,debugId, nodeInfoService.getAllNodesEntry).toBox
Copy link
Member Author

Choose a reason for hiding this comment

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

this should not always we fetched, only when there is no filter

Copy link
Member Author

Choose a reason for hiding this comment

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

and we are on the acceptedInventoryDIT only

timeres = (System.currentTimeMillis - timePreCompute)
_ = logger.debug(s"LDAP result: ${res.entries.size} entries obtained in ${timeres}ms for query ${query.toString}")
ldapEntries <- nodeInfoService.getLDAPNodeInfo(res.entries.flatMap(x => x(A_NODE_UUID).map(NodeId(_))).toSet, res.nodeFilters, query.composition).toBox
Expand Down Expand Up @@ -357,6 +357,8 @@ class InternalLDAPQueryProcessor(
, select : Seq[String] = Seq()
, limitToNodeIds: Option[Seq[NodeId]] = None
, debugId : Long = 0L
, allNodesEntry : Option[Seq[LDAPEntry]] = None// this is hackish, to have the list of all node if
// only nodeinfofilters, so that we don't look for all
) : IOResult[LdapQueryProcessorResult] = {


Expand Down Expand Up @@ -470,13 +472,29 @@ class InternalLDAPQueryProcessor(
_ <- logPure.debug(s"[${debugId}] Start search for ${query.toString}")
// Construct & normalize the data
nq <- normalizedQuery
lots <- ldapObjectTypeSets(nq)
dmms <- dnMapMapSets(nq, lots)
optdms = dnMapSets(nq, dmms)
// special case: no query, but we create a dummy one,
// identified by noFilterButTakeAllFromCache = true
// in this case, we skip all this part
optdms <- {
if (nq.noFilterButTakeAllFromCache) {
None.succeed
} else {

for {
lots <- ldapObjectTypeSets(nq)
dmms <- dnMapMapSets(nq, lots)
inneroptdms = dnMapSets(nq, dmms)
} yield {
inneroptdms
}
}
}
// If dnMapSets returns a None, then it means that we are ANDing composition with an empty value
// so we skip the last query
results <- optdms match {
case None =>
case None if nq.noFilterButTakeAllFromCache =>
allNodesEntry.getOrElse(Seq()).succeed
case None =>
Seq[LDAPEntry]().succeed
case Some(dms) =>
(for {
Expand Down Expand Up @@ -973,17 +991,20 @@ class InternalLDAPQueryProcessor(
subQueries = groupedSetFilter.view.mapValues(_.view.filterKeys { _ != "node" }.toMap).filterNot( _._2.isEmpty).toMap
} yield {
// at that point, it may happen that nodeFilters and otherFilters are empty
val mainFilters = if(groupedSetFilter.isEmpty) {
val (mainFilters, andAndEmpty) = if(groupedSetFilter.isEmpty) {
query.composition match {
// In that case, we add a "get all nodes" query and all filters will be done in node info.
case And => Some(Set[ExtendedFilter](LDAPFilter(BuildFilter.ALL)))
// we should have a specific case here, saying: it's empty, we are AND, so we AND with all the cache
// case And => (None, true)

case And => (Some(Set[ExtendedFilter](LDAPFilter(BuildFilter.ALL))), true)
Copy link
Member Author

Choose a reason for hiding this comment

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

Here for the test, it's always true, but it will cause many problems for the non accepted DIT

// In that case, We should return no Nodes from this query and we will only query nodes with filters from node info.
case Or => None
case Or => (None, false)
}
} else { nodeFilters }
} else { (nodeFilters, false) }

val nodeInfos = nodeInfoFilters.map { case QueryFilter.NodeInfo(c, comp, value) => c.matches(comp, value)}
LDAPNodeQuery(mainFilters, query.composition, query.transform, subQueries, nodeInfos)
LDAPNodeQuery(mainFilters, query.composition, query.transform, subQueries, nodeInfos, andAndEmpty)
}
}
}