Raise if oscilating mutation is found #1030
Conversation
pkg/mutation/system.go
Outdated
| return false, nil | ||
| } | ||
| if cmp.Equal(original, obj) { | ||
| return false, fmt.Errorf("ocilating mutation for %s %s", obj.GroupVersionKind().Group, obj.GroupVersionKind().Kind) |
| if i == 0 { | ||
| return false, nil | ||
| } | ||
| if cmp.Equal(original, obj) { |
There was a problem hiding this comment.
Do we want to do deep comparisons to find oscillating mutations? This could ~double the cost of mutation as I imagine for large objects the deep comparison is likely the most expensive part. If we get an oscillating mutation, we could just wait for a non-convergence error.
I think the tradeoff here is: if we have a system of 20 mutations and we detect oscillation after the 2nd application, we catch it early, but successful mutations now have ~40 comparisons.
Without this check: if we have the same oscillating system, we detect it after ~20 mutations, but successful mutations now have ~20 comparisons.
There was a problem hiding this comment.
Note that if line 92 is true, we will always return, either with an error or without.
The number of mutations will stay the same as before the change, and the number of comparison will be increased by 1 only (always).
If we have 20 mutations, and end after the second iteration, both before the change we have 40 mutation, and 3 comparisons (2 with old, 1 with original).
if cmp.Equal(old, obj) { // If this is true, we always leave the if with a return
if i == 0 {
return false, nil
}
if cmp.Equal(original, obj) { // If not true, we return anyway
return false, fmt.Errorf("ocilating mutation for %s %s", obj.GroupVersionKind().Group, obj.GroupVersionKind().Kind)
}
return true, nil // We always return here anyway
}
There was a problem hiding this comment.
You are right, sorry for the invalid feedback!
mmirecki
force-pushed
the
raiseIfOscilatingMutationsWasFound
branch
2 times, most recently
from
afd2f88 to
56fb5bf
Compare
Codecov Report
@@ Coverage Diff @@
## master #1030 +/- ##
==========================================
- Coverage 46.40% 46.24% -0.16%
==========================================
Files 62 62
Lines 4060 4063 +3
==========================================
- Hits 1884 1879 -5
- Misses 1932 1936 +4
- Partials 244 248 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
mmirecki
changed the title
pkg/mutation/system.go
Outdated
| @@ -73,10 +73,10 @@ func (s *System) Upsert(m Mutator) error { | |||
| } | |||
|
|
|||
| // Mutate applies the mutation in place to the given object | |||
There was a problem hiding this comment.
Can you pls add a comment about the return values?
Signed-off-by: mmirecki <mmirecki@redhat.com>
mmirecki
force-pushed
the
raiseIfOscilatingMutationsWasFound
branch
from
56fb5bf to
e179a9e
Compare
No description provided.