Refactor Multi Node Analyzers #1646
Conversation
diamonwiggins
changed the title
There was a problem hiding this comment.
The code changes looks good, I just have few comments and questions.
Understand that all other analyzers will have to use function retrieveCollectedContents to retrieve the files before analyzing.
We will have to ensure that other host collectors follow directory convention of base dir system/<nodeName>/<file.json>
E.g. diskUsage doesn't follow this at the moment.
host-collectors
├── diskUsage
│ └── gerard-kurl-0
│ └── diskUsage.json
└── system
├── gerard-kurl-0
│ ├── block_devices.json
│ ├── cpu.json
│ ├── hostos_info.json
│ └── memory.json
└── node_list.json
Also, does this PR mean other analyzers will just have to implement method CheckCondition for the analyzing part?
Yup, every analyzer will just need to move their |
nvanthao
force-pushed
the
diamonwiggins/refactor-multi-node-analyzers
branch
from
d217ad1
to
c05d677
Compare
There was a problem hiding this comment.
I like the direction this PR has taken. This will DRY quite many of the host analysers. I've left some minor code comment changes
A change we can consider doing is moving node-list.json a directory higher to remove the duplicate file that gets created. It's not a major issue since it will contain the same contents.
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to evaluate outcomes") | ||
| } | ||
| results = append(results, analyzeResult...) |
There was a problem hiding this comment.
analyzeResult can be nil. Check if nil
| type CollectedContent struct { | ||
| NodeName string | ||
| Data CollectorData | ||
| } | ||
|
|
||
| type CollectorData interface{} | ||
|
|
||
| type NodeNames struct { | ||
| Nodes []string `json:"nodes"` | ||
| } |
There was a problem hiding this comment.
Make these structs private. They are not used outside of analyze package
Ignore this comment. I just saw code doing this |
Description, Motivation and Context
Refactoring analyzers to be easier to write. All analyzers implement a function to check a condition and then return true/false. All analyzers also evaluate outcomes in a similar way.
The recently added code to check if there are contents from remote collection present and then proceed to use them and if not fallback to local contents is also code that can be shared amongst all analyzers.
The goal is that these changes makes it easier to update all remaining analyzers to be able to process remote collected contents, and also just easier to maintain and update host analyzers going forward.
TODO:
Checklist
Does this PR introduce a breaking change?