Fixes #8344: Missing relevant information about why the compliance is what it is on node #1103

fanf · 2016-05-24T14:31:59Z

https://www.rudder-project.org/redmine/issues/8344

fanf · 2016-05-24T14:42:38Z

PR rebased

ncharles · 2016-05-24T14:46:07Z

rudder-core/src/main/scala/com/normation/rudder/reports/execution/ReportsExecutionService.scala

@@ -149,6 +160,7 @@ class ReportsExecutionService (
        reportsRepository.getReportsWithLowestId match {
          case Full(Some((id, report))) =>
            logger.debug(s"Initializing the status execution update to  id ${id}, date ${report.executionTimestamp}")
+            this.idForCheck = id


why a this here ?

the idForCheck ? For init, else it remains at 0 (it would change on next run, but I don't think it's a problem ?)

no, question is why a "this." only here

ah, because the param is also named "id", and if I don't qualify the other id, there is a name conflict

I would understand if the 'this.' was on 'id', but I don't understand why you need to add it to IdForCheck

idForCheck = id

Would have been what I expect!

I still don't understand you explaination @fanf :( I don't see the conflict

VinceMacBuche · 2016-05-24T14:57:11Z

Why is there still two commits, 94e73d3 should be removed as it was merged elsewhere

fanf · 2016-05-24T14:58:58Z

PR rebased

ncharles · 2016-05-24T15:11:40Z

rudder-core/src/main/scala/com/normation/rudder/services/reports/ReportingServiceImpl.scala

@@ -95,17 +96,17 @@ trait RuleOrNodeReportingServiceImpl extends ReportingService {
      nodeIds <- confExpectedRepo.findCurrentNodeIds(ruleId)
      reports <- findRuleNodeStatusReports(nodeIds, Set(ruleId))
    } yield {
-      RuleStatusReport(ruleId, reports.toIterable)
+      val toKeep = reports.values.flatMap( _._2.filter( _.ruleId == ruleId))


why are you refiltering ? findRuleNodeStatusReports is already filtering by ruleId

No good reason, I guess

fanf · 2016-05-24T15:11:49Z

PR rebased

ncharles · 2016-05-24T15:15:32Z

rudder-core/src/main/scala/com/normation/rudder/services/reports/ReportingServiceImpl.scala

-      report  =  reports.groupBy(_.nodeId).map { case(nodeId, reports) => NodeStatusReport(nodeId, reports) }.toSet
-      result  <- if(report.size == 1) Full(report.head) else Failure("Found bad number of reports for node " + nodeId)
+      reports    <- findRuleNodeStatusReports(Set(nodeId), Set())
+      (run, set) <- reports.get(nodeId)


This will fail is there are no reports on the given node -with a nasty stacktrace)

checkAndUpdateCache can return an empty map

no, it's an option with get, but you're right, it should be better with an error message

VinceMacBuche · 2016-05-24T15:16:33Z

If I undestand changes on ReportingService correctly, you change how Rule reports are sent from a 'Set of the reports on the Rule' to 'A Map that associate For a Node(id) to a couple RunAndConfig ( run and configuration id state) and the set of reports applied to that node ?

fanf · 2016-05-24T15:17:02Z

Yes

ncharles · 2016-05-24T15:20:00Z

rudder-core/src/main/scala/com/normation/rudder/services/reports/ReportingServiceImpl.scala

-          case Some(c) => s"configId:${c.value}"
-        }
-        s"${r.ruleId.value}/${r.serial}[exp:${r.expirationDate}][${run}][${cfid}]${r.compliance.toString}"
+        s"${r.ruleId.value}/${r.serial}[exp:${r.expirationDate}]${r.compliance.toString}"


why aren't you serializing agent run time anymore ? (config_id is toLog)

It's just below, because it's common to all reports for that run

fanf · 2016-05-24T15:20:26Z

PR rebased

ncharles · 2016-05-24T15:25:29Z

rudder-web/src/main/scala/com/normation/rudder/web/services/ReportDisplayer.scala

+          (
+            <p>{currentConfigId(expectedConfigId)}</p>
+            <p>But we received a run, started at {lastRunDateTime.toString(dateFormat)}, with a different configuration ID ({lastRunConfigId.configId.value})</p>
+            <p>The time since generation of the new configuration is greater than the grace period, so every received reports will be mark as unexpected</p>


will be marked

s/every/all/

s/greater/longer/

fanf · 2016-05-24T16:28:41Z

PR rebased

fanf · 2016-05-24T16:35:10Z

PR rebased

VinceMacBuche · 2016-05-24T17:25:15Z

rudder-core/src/main/scala/com/normation/rudder/services/reports/ReportingServiceImpl.scala

-      report  =  reports.groupBy(_.nodeId).map { case(nodeId, reports) => NodeStatusReport(nodeId, reports) }.toSet
-      result  <- if(report.size == 1) Full(report.head) else Failure("Found bad number of reports for node " + nodeId)
+      reports    <- findRuleNodeStatusReports(Set(nodeId), Set())
+      (run, set) <- Box(reports.get(nodeId)) ?~! s"Can not find report for node with ID ${nodeId.value}"


So much awesomeness (was reallly complicated before ...)

VinceMacBuche · 2016-05-24T17:34:08Z

It seems ok to me ! (but i can't say my brain can compute all cases)

If @ncharles as nothing more to say, it's ok to merge !!

jooooooon · 2016-05-24T17:54:10Z

rudder-core/src/main/scala/com/normation/rudder/domain/logger/ComplianceDebugLogger.scala

        s" last run: nodeConfigId: ${lastRunConfigId.toLog} received at ${lastRunDateTime} |"+
-        s" expire at ${lastRunExpiration}]"
+        s" expire at ${lastRunExpiration}"


s/expire/expired/

jooooooon · 2016-05-24T17:59:35Z

rudder-web/src/main/scala/com/normation/rudder/web/services/ReportDisplayer.scala

+        case NoReportInInterval(expectedConfigId) =>
+          (
+            <p>{currentConfigId(expectedConfigId)}</p>
+            <p>No recent runs were received for that node, and the grace period to get them expired.</p>


s/that node/node/

s/get them/receive them/

fanf · 2016-05-25T07:39:07Z

PR rebased

fanf · 2016-05-25T07:40:57Z

PR rebased

fanf · 2016-05-25T07:43:29Z

PR rebased

fanf · 2016-05-25T07:51:45Z

PR rebased

ncharles · 2016-05-25T08:53:29Z

except for my question, this looks ok
Caution, you'll probably need to change on 3.2 to handle the disabled reporting case

fanf · 2016-05-25T09:12:43Z

PR rebased

… what it is on node

fanf · 2016-05-25T09:41:46Z

PR rebased

ncharles · 2016-05-25T09:55:51Z

looks ok to me with the explaination

fanf force-pushed the bug_8344/missing_relevant_information_about_why_the_compliance_is_what_it_is_on_node branch from 6f28748 to 85addd4 Compare May 24, 2016 14:42

ncharles reviewed May 24, 2016
View reviewed changes

fanf force-pushed the bug_8344/missing_relevant_information_about_why_the_compliance_is_what_it_is_on_node branch from 85addd4 to de7c2cb Compare May 24, 2016 14:58

ncharles reviewed May 24, 2016
View reviewed changes

fanf force-pushed the bug_8344/missing_relevant_information_about_why_the_compliance_is_what_it_is_on_node branch from de7c2cb to 070eeea Compare May 24, 2016 15:11

ncharles reviewed May 24, 2016
View reviewed changes

fanf force-pushed the bug_8344/missing_relevant_information_about_why_the_compliance_is_what_it_is_on_node branch from 070eeea to c7fd6e2 Compare May 24, 2016 15:20

ncharles reviewed May 24, 2016
View reviewed changes

fanf force-pushed the bug_8344/missing_relevant_information_about_why_the_compliance_is_what_it_is_on_node branch from c7fd6e2 to 119f4da Compare May 24, 2016 16:28

fanf force-pushed the bug_8344/missing_relevant_information_about_why_the_compliance_is_what_it_is_on_node branch from 119f4da to 8a28641 Compare May 24, 2016 16:35

VinceMacBuche reviewed May 24, 2016
View reviewed changes

jooooooon reviewed May 24, 2016
View reviewed changes

fanf force-pushed the bug_8344/missing_relevant_information_about_why_the_compliance_is_what_it_is_on_node branch from 8a28641 to 7d8b216 Compare May 25, 2016 07:39

fanf force-pushed the bug_8344/missing_relevant_information_about_why_the_compliance_is_what_it_is_on_node branch from 7d8b216 to 7ecb070 Compare May 25, 2016 07:40

fanf force-pushed the bug_8344/missing_relevant_information_about_why_the_compliance_is_what_it_is_on_node branch from 7ecb070 to 963ecc0 Compare May 25, 2016 07:43

fanf force-pushed the bug_8344/missing_relevant_information_about_why_the_compliance_is_what_it_is_on_node branch from 963ecc0 to bb17a07 Compare May 25, 2016 07:51

fanf force-pushed the bug_8344/missing_relevant_information_about_why_the_compliance_is_what_it_is_on_node branch from bb17a07 to 159bfc8 Compare May 25, 2016 09:12

Fixes #8344: Missing relevant information about why the compliance is…

08add9f

… what it is on node

fanf force-pushed the bug_8344/missing_relevant_information_about_why_the_compliance_is_what_it_is_on_node branch from 159bfc8 to 08add9f Compare May 25, 2016 09:41

ncharles merged commit e216788 into Normation:branches/rudder/3.0 May 25, 2016

fanf deleted the bug_8344/missing_relevant_information_about_why_the_compliance_is_what_it_is_on_node branch March 15, 2024 10:52

This pull request was closed.

Fixes #8344: Missing relevant information about why the compliance is what it is on node #1103

Fixes #8344: Missing relevant information about why the compliance is what it is on node #1103

Conversation

fanf commented May 24, 2016

fanf commented May 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinceMacBuche commented May 24, 2016

fanf commented May 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanf commented May 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinceMacBuche commented May 24, 2016

fanf commented May 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanf commented May 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanf commented May 24, 2016

fanf commented May 24, 2016

Choose a reason for hiding this comment

VinceMacBuche commented May 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanf commented May 25, 2016

fanf commented May 25, 2016

fanf commented May 25, 2016

fanf commented May 25, 2016

ncharles commented May 25, 2016

fanf commented May 25, 2016

fanf commented May 25, 2016

ncharles commented May 25, 2016