Sketch: a third take on defaulting values #2587

thockin · 2014-11-24T23:18:23Z

This still fails round-trip tests. Ideas there?

ref #1502

This still fails round-trip tests. Ideas there?

lavalamp · 2014-11-24T23:28:00Z

Liking the look of this more.

I expect if you tell the fuzzer not to generate blank strings, then round-trip would pass.

Longer-term, we need two separate fuzz tests:

One that uses truly random values everywhere, throws the result at apiserver to hurt for crashes/validation bugs.
One that generates random but mostly-legal stuff, that we use for testing round-tripping.

The second one ought to get fuzz functions that generate a random but valid setting for enum fields; this would also make the round-trip test pass for you, I expect.

thockin · 2014-11-25T00:56:48Z

Brian asked for test that was failing. As an example, it looks like the default value for Volume.Source is being used. Not sure why the fuzzer doesn't descend. Maybe that's the better fix?

        serialization_test.go:165: 1: PodList: diff: {"metadata":{"selfLink":"\u003e奛呎Ħ趔","resourceVersion":"23605793843120366"},"items":[{"metadata":{"name":"3ʉ#Ĉ5D冲+,ő襝KW.","selfLink":"S諆慻É崵ʊU\u0026bZǧ鴾*qL?Ł","resourceVersion":"36326607031304659","creationTimestamp":null},"spec":{"volumes":[{"name":"娀ê","source":

                A: null}],"containers":[{"name":"Ƃč@R","image":"Ț¹ɲnj糬BQG辴j","workingDir":"NywȖ´訆ǻ躖嗶Ɍ匑PTDƴ聰:仗","ports":[{"name":"jŃu勠ƖʮȨÌW","hostPort":8982957898900148083,"containerPort":4925888727650424677,"protocol":"囡;fǂ燏ɹO栻ǀǰ凮操皊龏厯","hostIP":"1ĜHaZl蕲"}],"memory":1629318258256292882,"cpu":-3481486892126426206,"volumeMounts":[{"name":"靝癸ƻ歜禣×i·¿8藣","readOnly":true,"mountPath":"ǦĐʌb_牟刼eǽ嵁荰牭羓+鑟þȶ"}],"livenessProbe":{"tcpSocket":{"port":"痞餫
裫nÇ籫Wɑ-"},"exec":{},"initialDelaySeconds":4381171747482839024},"lifecycle":{"postStart":{},"preStop":{"httpGet":{"path":"É蟞śSpǠ怎ƻF§1瞸暆E¸","port":5460034994827928605,"host":"喗蕊Ǔ-³sɤ晕dC憐凪ǈ}F!寞N涫"}}},"terminationMessagePath":"罨","imagePullPolicy":"ƽx"}],"restartPolicy":{}},"status":{"phase":"Pending","host":"ȣ4舻僴巤u嚷{ȑƻSƖr虿Ć","hostIP":"蠆鋤腻","podIP":"垮tŬVłE.癔ǲɈʭȳ}ɪï§¹ȰȦ"}}]}

                B: {"hostDir":null,"emptyDir":{},"persistentDisk":null,"gitRepo":null}}],"containers":[{"name":"Ƃč@R","image":"Ț¹ɲnj糬BQG辴j","workingDir":"NywȖ´訆ǻ躖嗶Ɍ匑PTDƴ聰:仗","ports":[{"name":"jŃu勠ƖʮȨÌW","hostPort":8982957898900148083,"containerPort":4925888727650424677,"protocol":"囡;fǂ燏ɹO栻ǀǰ凮操皊龏厯","hostIP":"1ĜHaZl蕲"}],"memory":1629318258256292882,"cpu":-3481486892126426206,"volumeMounts":[{"name":"靝癸ƻ歜禣×i·¿8藣","readOnly":true,"mountPath":"ǦĐʌb_牟
刼eǽ嵁荰牭羓+鑟þȶ"}],"livenessProbe":{"tcpSocket":{"port":"痞餫裫nÇ籫Wɑ-"},"exec":{},"initialDelaySeconds":4381171747482839024},"lifecycle":{"postStart":{},"preStop":{"httpGet":{"path":"É蟞śSpǠ怎ƻF§1瞸暆E¸","port":5460034994827928605,"host":"喗蕊Ǔ-³sɤ晕dC憐凪ǈ}F!寞N涫"}}},"terminationMessagePath":"罨","imagePullPolicy":"ƽx"}],"restartPolicy":{"always":{}}},"status":{"phase":"Pending","host":"ȣ4舻僴巤u嚷{ȑƻSƖr虿Ć","hostIP":"蠆鋤腻","podIP":"垮tŬVłE.癔ǲɈʭȳ}ɪï§¹ȰȦ"}}]}

bgrant0607 · 2014-11-25T01:39:49Z

How hard would it be to use the same approach to diffing that we plan to use for config: only diff the fields set in the source?

bgrant0607 · 2014-11-25T01:41:26Z

pkg/api/v1beta1/defaults.go

+)
+
+func init() {
+	api.Scheme.AddDefaultingFuncs(


This is pretty clear/clean. I like it.

thockin · 2014-11-25T02:03:14Z

New push is up with a new commit that hacks a bit at the testing.

@lavalamp might be that improving the fuzzer lib could help...

thockin · 2014-11-25T02:04:21Z

@bgrant0607 what does "set" mean for somethign that did not come from JSON/YAML? All fields are set, just some are set to nil or ""

bgrant0607 · 2014-11-25T06:16:45Z

How do we ensure that objects created this way are valid?

We could create versioned objects, marshal them, decode, encode, compare set fields, re-decode, and compare with previously decoded objects.

thockin · 2014-11-25T06:46:54Z

They are not valid in the sense of validation would pass them, but all this test is verifying is round-trip between internal and external - all of the registered conversion functions. This has been sufficient and useful so far, but maybe not any further. Would be nice if it still held.

kubernetes-bot · 2014-11-26T04:58:17Z

Can one of the admins verify this patch?

lavalamp · 2014-12-05T18:38:01Z

pkg/api/serialization_test.go

@@ -138,6 +145,16 @@ var apiObjectFuzzer = fuzz.New().NilChance(.5).NumElements(1, 1).Funcs(
 			c.RandString(): c.RandString(),
 		}
 	},
+	func(rp *api.RestartPolicy, c fuzz.Continue) {
+		// Exactly one of the fields should be set.
+		//FIXME: the fuzz can still end up nil.  What if fuzz allowed me to say that?


Just look up a bit at the "NilChance(.5)". Maybe fuzz.Continue should let you modify that temporarily.

dchen1107 · 2015-01-12T23:44:18Z

What is blocking us on making progress of this PR beside rebase?

bgrant0607 · 2015-01-13T01:22:24Z

I think this was blocked on a couple fuzzer-related things:

how to determine that a round-tripped object is the same after default values have been set
how to ensure the conversion and/or defaulting logic in the round-trip process behaves correctly for randomly generated objects

Alternatives for (1) are to apply defaults before round-tripping or to only diff selected fields, as we plan to do in config, such as by keeping track of which fields the fuzzer populates.

I think the only sane thing to do for (2) is to ensure that we round-trip only valid objects. This means they must have defaults applied and must pass validation.

lavalamp · 2015-01-13T01:33:14Z

A solution to 1 is to use the Semantic.DeepEqual I added the other day, or make a separate one that specifically ignores defaults (probably a good idea anyway).

The conversion routines don't have to do correct things on random input, but they must NOT crash. Fuzz testing should continue to be used to verify this property even if we have to do something more fancy to verify the round-trip property. There should be two tests here, not the one we have right now.

thockin · 2015-01-13T02:07:50Z

Main blocker is a person. To work on it.
On Jan 12, 2015 5:33 PM, "Daniel Smith" notifications@github.com wrote:

A solution to 1 is to use the Semantic.DeepEqual I added the other day, or
make a separate one that specifically ignores defaults (probably a good
idea anyway).

The conversion routines don't have to do correct things on random input,
but they must NOT crash. Fuzz testing should continue to be used to verify
this property even if we have to do something more fancy to verify the
round-trip property. There should be two tests here, not the one we have
right now.

Reply to this email directly or view it on GitHub
#2587 (comment)
.

bgrant0607 · 2015-02-06T06:54:12Z

#3854 has been merged. This is obsolete.

thockin added 2 commits November 24, 2014 13:28

Make room for defaulting funcs

9d17e2d

Sketch: a third take on defaulting values

ad056f1

This still fails round-trip tests. Ideas there?

bgrant0607 reviewed Nov 25, 2014
View reviewed changes

pkg/api/v1beta1/defaults.go

)

func init() {

api.Scheme.AddDefaultingFuncs(

Copy link

Member

bgrant0607 Nov 25, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty clear/clean. I like it.

hack at testing issues

878728e

bgrant0607 mentioned this pull request Dec 4, 2014

Sketch: a different take on defaulting values #2571

Closed

bgrant0607 self-assigned this Dec 4, 2014

lavalamp reviewed Dec 5, 2014
View reviewed changes

dchen1107 mentioned this pull request Jan 12, 2015

If the image with :latest tag specified in spec, pull even PullIfNotPresent is set. #2388

Merged

erictune mentioned this pull request Jan 15, 2015

Admission control plugin for resource defaults #3472

Merged

bgrant0607 mentioned this pull request Jan 16, 2015

Can we get consistent defaulting for API? #1502

Closed

bgrant0607 mentioned this pull request Jan 27, 2015

Add timeouts to HealthChecks and retry checks #3593

Merged

yujuhong mentioned this pull request Jan 28, 2015

Migrate API defaulting to a centralized location #3854

Merged

bgrant0607 closed this Feb 6, 2015

thockin deleted the defaults-v3 branch June 25, 2015 04:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sketch: a third take on defaulting values #2587

Sketch: a third take on defaulting values #2587

thockin commented Nov 24, 2014

lavalamp commented Nov 24, 2014

thockin commented Nov 25, 2014

bgrant0607 commented Nov 25, 2014

bgrant0607 Nov 25, 2014

thockin commented Nov 25, 2014

thockin commented Nov 25, 2014

bgrant0607 commented Nov 25, 2014

thockin commented Nov 25, 2014

kubernetes-bot commented Nov 26, 2014

lavalamp Dec 5, 2014

dchen1107 commented Jan 12, 2015

bgrant0607 commented Jan 13, 2015

lavalamp commented Jan 13, 2015

thockin commented Jan 13, 2015

bgrant0607 commented Feb 6, 2015

Sketch: a third take on defaulting values #2587

Sketch: a third take on defaulting values #2587

Conversation

thockin commented Nov 24, 2014

lavalamp commented Nov 24, 2014

thockin commented Nov 25, 2014

bgrant0607 commented Nov 25, 2014

bgrant0607 Nov 25, 2014

Choose a reason for hiding this comment

thockin commented Nov 25, 2014

thockin commented Nov 25, 2014

bgrant0607 commented Nov 25, 2014

thockin commented Nov 25, 2014

kubernetes-bot commented Nov 26, 2014

lavalamp Dec 5, 2014

Choose a reason for hiding this comment

dchen1107 commented Jan 12, 2015

bgrant0607 commented Jan 13, 2015

lavalamp commented Jan 13, 2015

thockin commented Jan 13, 2015

bgrant0607 commented Feb 6, 2015