pkg/api: adjustment stripping fixes. #251
Conversation
klihub
force-pushed
the
fixes/adjustment-strip
branch
2 times, most recently
from
cb72710 to
1fb35ba
Compare
pkg/api/strip.go
Outdated
| if len(s.Flags) == 0 { | ||
| s.Flags = nil |
There was a problem hiding this comment.
Only had a quick glance, but if this is moved to the start, I think we'd be able to use an early return instead of having to check each option for non-empty.
There was a problem hiding this comment.
@thaJeztah Thanks for pointing that out. I rewrote all the newly added functions to avoid using an empty boolean, deal with any zero length map/slice reduction to nil first, and use an early return instead where we used to do an empty = false.
There was a problem hiding this comment.
I'll take a look at making corresponding changes to the existing bits, but maybe in a separate PR.
klihub
force-pushed
the
fixes/adjustment-strip
branch
from
1fb35ba to
9c9e50f
Compare
|
|
||
| "github.com/containerd/nri/pkg/api" | ||
|
|
||
| faker "github.com/brianvoe/gofakeit/v7" |
There was a problem hiding this comment.
Let's see... license is MIT.. one owner.. a couple maintainer/committers.. seems to respond quickly to PRs.. seems fine.
faked random value testing.. semi-deterministic.. interesting shortcut..
Normally we have explicit tests, should we have both deterministic tests and also add these randomly generated values?
There was a problem hiding this comment.
Not sure if it's fully comparable, but in moby, we started using pgregory.net/rapid for property based tests; see moby/moby#50519 (and the repository https://github.com/flyingmutant/rapid).
| if len(l.NetDevices) != 0 { | ||
| empty = false | ||
| } | ||
|
|
|
needs rebase.. |
klihub
force-pushed
the
fixes/adjustment-strip
branch
from
9c9e50f to
d4cfc89
Compare
|
Thoughts on deterministic testing vs randomly generated values, just a vague concern that this may result in semi-random testing results. |
pkg/api/strip_test.go
Outdated
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestContainerAdjustmentStrip(t *testing.T) { |
There was a problem hiding this comment.
It's probably also worth having tests that validate Strip does successfully normalize zero-valued structs/maps/slices to nil.
There was a problem hiding this comment.
@samuelkarp And I also pushed a fix for my seriously brain-dead idea of stripping both the collected and the expected results. It is a really bad idea. We should never Strip() the expected result, as it only masks all the various Strip()ping bugs we have/had in place where a non-empty adjustment is incorrectly reduced to nil, which then goes unnoticed if we Strip() both of them (incorrectly) as then they will compare equal.
There was a problem hiding this comment.
@samuelkarp Having taken a look at your #259 I think the best course of action would be to extend/fix the tests added here to also verify valid reductions to nil, then cherry-pick from this PR all the other commits except the hand-fix to Strip() to #259, finalize and merge that.
Would it be okay with you, if I extend the tests here, then cherry-pick it and the other commits and push them to your #259 ?
There was a problem hiding this comment.
We can also merge this first, then I can rebase #259 on top of it. That might be a cleaner history.
There was a problem hiding this comment.
Yes, true. I think it'd make more sense that way around.
There was a problem hiding this comment.
It's probably also worth having tests that validate Strip does successfully normalize zero-valued structs/maps/slices to nil.
I added test cases for verifying reduction of empty adjustments to nil. I'm not sure if it is a comprehensive enough set.
Many of our tests set a single field in a container adjustment then does a protocmp against an expected result. Add tests for verifying that stripping an adjustment with a single field set does not unexpectedly drop that field. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Add missing handling for LinuxScheduler, LinuxIOPriority, LinuxSeccomp, LinuxNamespaces, Sysctl and LinuxNetDevice in adjustment stripping. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Okay, let me try to be less stupid here... Stop Strip()ping expected results before comparing them to collected ones. It only masks bugs unnoticable if Strip() incorrectly reduces a non-empty adjustment or update to nil. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Remove incorrect 'clearing' tests for linux scheduling policy and I/O priority. We don't really have support for clearing either at the moment. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
klihub
force-pushed
the
fixes/adjustment-strip
branch
from
d306151 to
3333527
Compare
4 participants
Add missing handling of recently introduced adjustment fields in Strip().