Fixes #23927: Migrate away from NodeInfoService #5269
Conversation
| withDefautls, | ||
| policyServer, | ||
| withDefautls.toNodeInfo, | ||
| policyServer.toNodeInfo, |
There was a problem hiding this comment.
InterpolationContext and CompareProperties are let untouched for now, so we have some conversion happening. It will be clean-up in a further iteration.
| // policy servers are sorted by their promiximity with root, root first | ||
| a.sortBy(x => NodePriority(x)).map(_.id.value), // simple nodes are sorted alphanum | ||
| // policy servers are sorted by their proximity with root, root first | ||
| a.sortBy(x => NodePriority(x.id, x.isPolicyServer, x.policyServerId)) |
There was a problem hiding this comment.
we're really just using these three attributes for priority, which are available on both NodeInfo and CoreNodeFact. It eases migration to be more specific here.
| val onlySyslogSupported = { | ||
| if (versionHasSyslogOnly(Some(nodeInfo.rudderAgent.version), nodeInfo.rudderAgent.agentType)) Some(nodeInfo.rudderAgent) | ||
| else None | ||
| } |
There was a problem hiding this comment.
We always have an agent now, so I adapted the logic
|
PR updated with a new commit |
| allNodeIds <- nodeInfoService.getAll().map(_.filter { case (_, n) => n.state != NodeState.Ignored }.keySet) | ||
| allNodeIds <- nodeFactRepository | ||
| .getAll() | ||
| .map(_.collect { case (_, n) if (n.rudderSettings.state != NodeState.Ignored) => n.id }.toSet) |
There was a problem hiding this comment.
(refactoring): we don't need to create a full collection with filter since we just want the keys, collect is better
| build("n4", Some(PolicyMode.Audit)) | ||
| ).toMap | ||
|
|
||
| CoreNodeFactRepository.make(NoopFactStorage, NoopGetNodesbySofwareName, Map(), accepted, Chunk.empty).runNow |
There was a problem hiding this comment.
The base repo for ReportingServiceTest is now a fact repo. It doesn't really matters since all tests are commented out in any case.
| @@ -272,65 +280,66 @@ ootapja6lKOaIpqp0kmmYN7gFIhp | |||
| val rootHostname = "server.rudder.local" | |||
| val rootAdmin = "root" | |||
|
|
|||
| val rootNode = Node( | |||
| val factRoot = CoreNodeFact( | |||
There was a problem hiding this comment.
important change in test data creation in NodeConfigDate: now, the base data is a CoreNodeFact, and Node and NodeInfo are derived from it.
| ) | ||
|
|
||
| val node1Node = Node( | ||
| val rootNode = factRoot.toNode | ||
| val root = factRoot.toNodeInfo |
There was a problem hiding this comment.
derivation from facts for root
| // node1 us a relay | ||
| val node2Node = node1Node.copy(id = id2, name = id2.value) | ||
| val node2 = node1.copy(node = node2Node, hostname = hostname2, policyServerId = root.id) | ||
| val fact2 = fact1.modify(_.id).setTo(id2).modify(_.fqdn).setTo(hostname2) |
There was a problem hiding this comment.
use quicklens to make more consistant update patterns on data
| } | ||
| val accepted = nodes.map { case (n, _, _) => n }.toMap | ||
| val nodeFactRepo = | ||
| CoreNodeFactRepository.make(NoopFactStorage, NoopGetNodesbySofwareName, Map(), accepted, Chunk.empty).runNow |
There was a problem hiding this comment.
Now the base repo for CachedFindRuleNodeStatusReportsTest is a NodeFactRepository
| @@ -282,6 +286,8 @@ class NodeApi( | |||
| params: DefaultParams, | |||
| authzToken: AuthzToken | |||
| ): LiftResponse = { | |||
| implicit val qc: QueryContext = authzToken.qc | |||
There was a problem hiding this comment.
general pattern for API now: the QueryContext is weaved into authzToken, so just expose it
| } yield { | ||
| optNode match { | ||
| case Some(node) | ||
| if (node.agentsName.exists(a => a.agentType == AgentType.CfeCommunity || a.agentType == AgentType.CfeEnterprise)) => | ||
| if (node.rudderAgent.agentType == AgentType.CfeCommunity || node.rudderAgent.agentType == AgentType.CfeEnterprise) => |
There was a problem hiding this comment.
we always have a rudder agent now
| else if (pending.contains(id)) PendingInventory.name | ||
| else "deleted" | ||
| } | ||
| val status = nodes.get(NodeId(id)).map(_.rudderSettings.status.name).getOrElse("deleted") |
| } else { | ||
| val propMap = nodes.values.groupMapReduce(_.id)(n => n.properties.filter(_.name == property))(_ ++ _) | ||
| propMap.map { case (k, v) => (k, v.toList.map(_.toJson)) }.succeed | ||
| }): IOResult[Map[NodeId, List[JValue]]] |
There was a problem hiding this comment.
refactor:
propMaponly used in the second casepropertiesare nowChunkin place oflist=>++
| toJsonError(None, message) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I didn't find any use of these two methods, not even in HTML or plugins
| val message = (eb ?~ ("Could not fetch pending Nodes")).msg | ||
| toJsonError(None, message) | ||
| } | ||
| } |
There was a problem hiding this comment.
Same than previously: not found any use.
| private[this] val asyncDeploymentAgent = RudderConfig.asyncDeploymentAgent | ||
| private[this] val configService = RudderConfig.configService | ||
| private[this] val boxNodeInfo = nodeInfoService.getNodeInfo(nodeId).toBox | ||
|
|
||
| def agentPolicyModeEditForm = new AgentPolicyModeEditForm() | ||
|
|
There was a problem hiding this comment.
Here (), the removal of nodeInfoService lead to substantial, non interesting changes and some simplification since we have NodeFact around, not just sometime inventory and sometime node info.
| (for { | ||
| _ <- nodeRepo.updateNode(newNodeInfo.node, modId, user, None) | ||
| _ <- nodeFactRepo.save(NodeFact.fromMinimal(newNodeFact))(cc) |
There was a problem hiding this comment.
Now us a ChangeContext to commit update to Node.
Two observations:
- perhaps we would like to have moreinfo in
CurrentUserlike user IP, - it seems that we want to have a
nodeFactRepo.savethat directly works onCoreNodeFactand manage for the us the data conversion (plus it would help it knows how to optimize save)
| @@ -99,13 +100,15 @@ class AsyncComplianceService( | |||
|
|
|||
| // Compute compliance level for all rules in a future so it will be displayed asynchronously | |||
| val futureCompliance: Future[Box[Map[Kind, Option[ComplianceLevel]]]] = { | |||
| implicit val qc: QueryContext = CurrentUser.queryContext | |||
|
|
|||
There was a problem hiding this comment.
I wonder if there is something special with Future and query context. I don't think it matters in our usage (the user must be logged to call that), but something that may be relevant in complicated bugs at some point.
| @@ -96,18 +98,16 @@ object DisplayNode extends Loggable { | |||
| private[this] val removeNodeService = RudderConfig.removeNodeService | |||
| private[this] val asyncDeploymentAgent = RudderConfig.asyncDeploymentAgent | |||
| private[this] val uuidGen = RudderConfig.stringUuidGenerator | |||
| private[this] val nodeInfoService = RudderConfig.nodeInfoService | |||
| private[this] val linkUtil = RudderConfig.linkUtil | |||
There was a problem hiding this comment.
Like in previous file, the removal of nodeInfoService leads to simplification and some changes.
| val detailsId = htmlId(jsId, "details_") | ||
| val sm = nodeFact.toFullInventory |
There was a problem hiding this comment.
We keep the mapping with FullNodeInventory because that part is low risk, but big => it can be done in latter iteration.
| @@ -477,32 +480,25 @@ object DisplayNode extends Loggable { | |||
|
|
|||
| // mimic the content of server_details/ShowNodeDetailsFromNode | |||
| def showNodeDetails( | |||
| sm: FullInventory, | |||
| nodeAndGlobalMode: Option[(NodeInfo, GlobalPolicyMode)], | |||
There was a problem hiding this comment.
now, we don't have to wonder if we have the node info - we always have it from node fat. And not getting the GlobalCompliance is a normal error, there's no special case to do for it.
| </div> | ||
| {displayServerRole(sm, inventoryStatus)} | ||
| <div><label>Agent:</label> { | ||
| sm.node.agents.map { a => |
There was a problem hiding this comment.
we have exactly one agent now
| } | ||
| </div> | ||
| <div> | ||
| <label>Inventory received:</label> { | ||
| sm.node.receiveDate.map(DateFormaterService.getDisplayDate(_)).getOrElse("Unknown") | ||
| DateFormaterService.getDisplayDate(nodeFact.factProcessedDate) |
There was a problem hiding this comment.
the intent is clearer now: it's not the date when the inventory was received on rudder server, but the date when it was processed by rudder web app.
| case AcceptedInventory => | ||
| val nodeInfoBox = nodeInfoService.getNodeInfo(nodeId).either.runNow |
There was a problem hiding this comment.
this is not relevant anymore. Plus, avoid more queries when we already have the data
| .rudder_global_policy_mode() | ||
| .chainError(s" Could not get global policy mode when getting node '${uuid.value}' details") | ||
| data <- historyRepos.get(uuid, selectedDate) | ||
| } yield (globalMode, data)).toBox match { |
There was a problem hiding this comment.
impact of previous change about really wanting the global policy mode: it needs to be retrieved here, too.
| globalMode <- configService | ||
| .rudder_global_policy_mode() | ||
| .chainError(s" Could not get global policy mode when getting node '${uuid.value}' details") | ||
| m <- historyRepos.get(uuid, date) |
| globalMode <- configService | ||
| .rudder_global_policy_mode() | ||
| .chainError(s" Could not get global policy mode when getting node '${id.value}' details") | ||
| m <- history.get(id, version) |
|
PR updated with a new commit |
fanf
force-pushed
the
arch_23927/migrate_away_from_nodeinfoservice
branch
3 times, most recently
from
f65ea26
to
f8f32cf
Compare
fanf
force-pushed
the
arch_23927/migrate_away_from_nodeinfoservice
branch
from
f8f32cf
to
22155e2
Compare
|
OK, merging this PR |
Normation-Quality-Assistant
merged commit 22155e2
into
Normation:master
https://issues.rudder.io/issues/23927
A not very exiting PR: mainly:
NodeInfoServicebyNodeFactRepositoryNodeInfotowardCoreNodeFactand adapt to the small attribute naming changes (add.rudderSettings,fqdnin place ofhostname, and alway one agent and not a list)implicit qr: QueryContextand weave them upword until we find a direct interaction with a user page / API.So I tried to comment on all the things that are not trivials to make reviewable.