Fixes #13559: Check node, not group, for need of change request #119
Conversation
fanf
force-pushed
the
bug_13559/check_node_not_group_for_need_of_change_request
branch
from
822f45e
to
8006503
Compare
|
PR rebased |
| * is supervised. | ||
| */ | ||
| def checkNodeTargetByRule(groups: FullNodeGroupCategory, allNodeInfo: Map[NodeId, NodeInfo], monitored: Set[SimpleTarget], rules: Set[Rule]): Boolean = { | ||
| val monitoredNodes = groups.getNodeIds(monitored.map(identity), allNodeInfo) |
There was a problem hiding this comment.
Why do you need to '.map(identity)' ?
There was a problem hiding this comment.
because set is monovariant, and I have a Set[SimpleTarget] where I need a Set[RuleTarget].
| /* | ||
| * Just check if the node group is the list of monitored groups | ||
| */ | ||
| override def forNodeGroup(actor: EventActor, change: NodeGroupChangeRequest): Box[Boolean] = { |
There was a problem hiding this comment.
We should check that the node group is not applying to the list of monitored Nodes
Let's say your group is already applied in a Rule, but not monitored and not containing monitored nodes, if you modify this group to include some monitored nodes, the rule will apply new configuration to monitored nodes.
same if the group loses some nodes and some configuration is not applied anymore on the monitored nodes
|
PR rebased |
fanf
force-pushed
the
bug_13559/check_node_not_group_for_need_of_change_request
branch
from
8006503
to
4d9f4df
Compare
| monitored <- monitoredTargets() | ||
| } yield { | ||
| val targetNodes = change.newGroup.serverList ++ change.previousGroup.map(_.serverList).getOrElse(Set()) | ||
| val exists = groups.getNodeIds(monitored.map(identity), allNodeInfo).find(nodeId => targetNodes.contains(nodeId)) |
There was a problem hiding this comment.
Don't we want to use intersect and list all Nodes making a conflict ? could be a problem if there is a lot of Nodes though ...
There was a problem hiding this comment.
I prefer to just give an example, else we are going to list the union of all nodes in faulty group, so perhaps thousand of them.
And I use the idea that either the user missed a special case (and there will be few nodes on it, so just an example should be ok), or he forgot a whole group (and in that case, just an example of the node should be ok).
I could also limit to for ex 10 nodes, so that in the case of special cases, it's likely that they are all displayed.
There was a problem hiding this comment.
Ok !! I merge this !! let's try that because and we'll see if it's ok or not when using it
| // we want to let the log knows why the change request need validation | ||
| if(exists && ChangeValidationLogger.isDebugEnabled) { | ||
| rules.foreach { rule => | ||
| groups.getNodeIds(rule.targets, allNodeInfo).find(nodeId => monitoredNodes.contains(nodeId)).foreach { node => |
There was a problem hiding this comment.
You could use intersect between the two Sets, and you could also put groups.getNodeIds(changes, allNodeInfo) in a value before
There was a problem hiding this comment.
The find is necessary for to avoid unecessary walk of the set. OK for the variable def.
| monitored <- monitoredTargets() | ||
| } yield { | ||
| val targetNodes = change.newGroup.serverList ++ change.previousGroup.map(_.serverList).getOrElse(Set()) | ||
| val exists = groups.getNodeIds(monitored.map(identity), allNodeInfo).find(nodeId => targetNodes.contains(nodeId)) |
There was a problem hiding this comment.
Ok !! I merge this !! let's try that because and we'll see if it's ok or not when using it
|
OK, merging this PR |
https://www.rudder-project.org/redmine/issues/13559