Conversation
zwass
left a comment
zwass
left a comment
There was a problem hiding this comment.
Overall I think this solution is headed in the right direction, but I have a nontrivial change to propose.
Some of my assumptions about the common case:
- There may be quite a lot of unique software throughout a fleet (potentially making loading all software quite expensive)
- A host will typically (on most update intervals) have minor changes, if any, to the installed software.
Currently, this method loads all software for the host, and all software for all hosts, before performing the diff. What if instead in our first step we just loaded the software for the host?
Then we can diff the software received from the host with what we have stored and then look up the IDs (inserting if necessary) for any that are not yet stored for the host.
Does that make sense? What do you think of the proposal?
| func (d *Datastore) generateChangesForNewSoftware(host *fleet.Host) ( | ||
| softwareChanges, error, | ||
| ) { |
There was a problem hiding this comment.
Should this also take the transaction for consistency?
There was a problem hiding this comment.
In order to do the selects within the tx as well, you mean?
| for currentKey := range current { | ||
| if _, ok := incoming[currentKey]; !ok { | ||
| deletesHostSoftware = append(deletesHostSoftware, allSoftware[currentKey]) | ||
| // TODO: delete from software if no host has it |
There was a problem hiding this comment.
I think it would be fine to do this as an out of band cleanup job.
So we would check the differences between the host software only. If nothing changed (assumed it's the most common case, we exit early). Otherwise, we delete what we have to delete (we already should have the IDs). Then we get the IDs for what needs adding. Whatever is not there, we insert in Did I understand your idea correctly? I think it makes sense. |
|
Yep, that's correct! |
zwass
left a comment
zwass
left a comment
There was a problem hiding this comment.
Strategy looks good! I requested some clarifications -- please make those before merging if you agree.
| var deletesHostSoftware []interface{} | ||
| deletesHostSoftware = append(deletesHostSoftware, hostID) | ||
|
|
||
| for currentKey := range currentIdmap { | ||
| if _, ok := incomingBitmap[currentKey]; !ok { | ||
| deletesHostSoftware = append(deletesHostSoftware, currentIdmap[currentKey]) | ||
| // TODO: delete from software if no host has it | ||
| } | ||
| } | ||
| if len(deletesHostSoftware) > 1 { | ||
| sql := fmt.Sprintf( | ||
| `DELETE FROM host_software WHERE host_id = ? AND software_id IN (%s)`, | ||
| strings.TrimSuffix(strings.Repeat("?,", len(deletesHostSoftware)-1), ","), | ||
| ) | ||
| if _, err := tx.Exec(sql, deletesHostSoftware...); err != nil { | ||
| return errors.Wrap(err, "delete host software") | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
What do you think of this clarification?
| var deletesHostSoftware []interface{} | |
| deletesHostSoftware = append(deletesHostSoftware, hostID) | |
| for currentKey := range currentIdmap { | |
| if _, ok := incomingBitmap[currentKey]; !ok { | |
| deletesHostSoftware = append(deletesHostSoftware, currentIdmap[currentKey]) | |
| // TODO: delete from software if no host has it | |
| } | |
| } | |
| if len(deletesHostSoftware) > 1 { | |
| sql := fmt.Sprintf( | |
| `DELETE FROM host_software WHERE host_id = ? AND software_id IN (%s)`, | |
| strings.TrimSuffix(strings.Repeat("?,", len(deletesHostSoftware)-1), ","), | |
| ) | |
| if _, err := tx.Exec(sql, deletesHostSoftware...); err != nil { | |
| return errors.Wrap(err, "delete host software") | |
| } | |
| } | |
| return nil | |
| var deletesHostSoftware []uint | |
| for currentKey := range currentIdmap { | |
| if _, ok := incomingBitmap[currentKey]; !ok { | |
| deletesHostSoftware = append(deletesHostSoftware, currentIdmap[currentKey]) | |
| // TODO: delete from software if no host has it | |
| } | |
| } | |
| if len(deletesHostSoftware) == 0 { | |
| return nil | |
| } | |
| sql := fmt.Sprintf( | |
| `DELETE FROM host_software WHERE host_id = ? AND software_id IN (%s)`, | |
| strings.TrimSuffix(strings.Repeat("?,", len(deletesHostSoftware)), ","), | |
| ) | |
| if _, err := tx.Exec(sql, hostID, deletesHostSoftware...); err != nil { | |
| return errors.Wrap(err, "delete host software") | |
| } | |
| return nil |
There was a problem hiding this comment.
+1 to the early exit of if len(deletesHostSoftware) == 0 {
The rest doesn't seem to work, the deletesHostSoftware needs to be of interface{} because otherwise it needs a cast for each element. And tx.Exec(sql, hostID, deletesHostSoftware...) doesn't seem to work either sadly.
The fun fact here is that I did try those things, so we are very much aligned :)
| currentIdmap map[string]uint, | ||
| incomingBitmap map[string]bool, | ||
| ) error { | ||
| var insertsHostSoftware []interface{} |
There was a problem hiding this comment.
| var insertsHostSoftware []interface{} | |
| var insertsHostSoftware []uint |
There was a problem hiding this comment.
I agree with it, but we can't do that because otherwise later on when we use it we would have to cast it to []interface{}, which involves going through each item and casting them individually.
| selectFunc := d.db.Select | ||
| if tx != nil { | ||
| selectFunc = tx.Select | ||
| } |
|
|
||
| currentBitmap := make(map[string]bool) | ||
| for _, s := range current { | ||
| currentBitmap[softwareToUniqueString(s)] = false |
There was a problem hiding this comment.
I find it a bit easier to follow when true is used for sets (even though it doesn't change the behavior)
| currentBitmap[softwareToUniqueString(s)] = false | |
| currentBitmap[softwareToUniqueString(s)] = true |
2 participants
No description provided.