Migrate API defaulting to a centralized location #3854

yujuhong · 2015-01-28T00:48:06Z

This PR is based on #2587 to fix issue #1502. Most API defaulting in validation.go and conversion.go have been migrated to defaults.go, where the defaulting functions are added to the decoder. The defaulting functions are tied to the specific API version, and will be called during decoding.

alex-mohr · 2015-01-28T01:22:05Z

@bgrant0607 Brian, you were pretty active in the referenced #1502 discussion -- do you have time to review?

yujuhong · 2015-01-28T17:21:27Z

@thockin, this PR will need a rebase to fix all recently modified tests once it is reviewed.

bgrant0607 · 2015-01-28T17:35:06Z

@alex-mohr Sorry, I had a massive backlog yesterday. I'll look at this today.

@smarterclayton May also want a look.

bgrant0607 · 2015-01-28T21:37:12Z

pkg/api/defaults.go

+			if util.AllPtrFieldsNil(obj) {
+				obj.Always = &RestartPolicyAlways{}
+			}
+		},


DNSPolicy default should be set here:
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/api/v1beta1/types.go#L65
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/api/validation/validation.go#L378

These comments apply to all the versioned instances of defaults.go.

smarterclayton · 2015-01-28T22:51:07Z

Good changes in general.

bgrant0607 · 2015-01-28T23:42:54Z

pkg/conversion/converter.go

+//         func(in *v1beta1.Pod) {
+//                 // defaulting logic...
+//          })
+func (c *Converter) RegisterDefaultingFunc(defaultingFunc interface{}) error {


We need a test explicitly for the new defaulting code. The existing tests were implicitly relying on the ad hoc default approaches that were previously used. Now they're not.

bgrant0607 · 2015-01-29T00:37:20Z

pkg/api/defaults.go

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


In the spirit of #3914, we shouldn't be adding any defaulting functions here. Let's just eliminate this file.

thockin · 2015-01-30T00:36:33Z

Overall I am good with this. I do dislike the need to sprinkle default values around the test codebase - if we got rid of the internal defaulting logic, do es that need go away?

bgrant0607 · 2015-01-30T01:42:05Z

@thockin With this change, validation depends on defaults being set. As we discussed, the "right" thing to do would be to create all of these test objects by decoding versioned objects. As a convenience and optimization, we could create a library of creators of simple objects for common scenarios -- internally we call these "test fixtures". That would be a pretty big change, though. I don't think we should block this PR on it.

thockin · 2015-02-03T04:44:17Z

Agree no need to block on the constructors - this is strictly an improvement.

Currently, the validation logic validates fields in an object and supply default values wherever applies. This change factors out defaulting to a set of defaulting callback functions for decoding (see kubernetes#1502 for more discussion). * This change is based on pull request 2587. * Most defaulting has been migrated to defaults.go where the defaulting functions are added. * validation_test.go and converter_test.go have been adapted to not testing the default values. * Fixed all tests with that create invalid objects with the absence of defaulting logic.

yujuhong · 2015-02-03T09:07:29Z

Addressed the comments in the new patch.

Added some (not complete) tests for defaulting (defaults_test.go).
Modified DecodeInto() to always convert even if the versions are the same (e.g. v1beta1 -> v1beta1). This is desirable because we want the side-effect of applying the defaults even when converting a versioned json/yaml to the same version of object. This facilitates the testing in defaults_test.go
Add a DeepDerivative helper function in deep_equal.go. This function compares two objects, a1, and a2, but ignore the unset/empty fields in a1. This is useful because in many cases we compare the expected result (a1) to the actual result (a2), while all we care about are the set fields in a1. This is particular useful for avoiding comparing the default values in many tests.

This PR has grown bigger and bigger every time I had to rebase. Hopefully we can get it in before another major rebase is required...

bgrant0607 · 2015-02-03T23:14:01Z

Thanks! LGTM. I'm happy to start with this, then improve it. Did you run e2e?

thockin · 2015-02-03T23:25:03Z

I am concerned about yet another deep-something comparator, but we can revisit after we lock this in :)

yujuhong · 2015-02-03T23:35:59Z

@thockin , I understand the concern, and we should definitely iterate on that later. My guess is that many tests are actually redundant, as they test the same logic repeatedly. A major cleanup might make these tests more manageable, and the comparator may no longer required(?).

yujuhong · 2015-02-04T00:39:57Z

One of the test suite in e2e failed:

2015/02/03 16:31:57 Passed tests: certs.sh[1/1] guestbook.sh[1/1] liveness.sh[1/1] monitoring.sh[1/1] pd.sh[1/1] services.sh[1/1] update.sh[1/1]
2015/02/03 16:31:57 Flaky tests:
2015/02/03 16:31:57 Failed tests: goe2e.sh[0/1]
2015/02/03 16:31:57 1 test(s) failed.

goe2e.sh failed due to errors such as x509: certificate signed by unknown authority
I doubt my change caused the failure, but I am not sure how to resolve this yet.

thockin · 2015-02-04T00:49:44Z

the x509 error is the bane of my job. Nuke your cluster and start over.

On Tue, Feb 3, 2015 at 4:40 PM, Yu-Ju Hong notifications@github.com wrote:

One of the test suite in e2e failed:

2015/02/03 16:31:57 Passed tests: certs.sh[1/1] guestbook.sh[1/1] liveness.sh[1/1] monitoring.sh[1/1] pd.sh[1/1] services.sh[1/1] update.sh[1/1]
2015/02/03 16:31:57 Flaky tests:
2015/02/03 16:31:57 Failed tests: goe2e.sh[0/1]
2015/02/03 16:31:57 1 test(s) failed.

goe2e.sh failed due to errors such as x509: certificate signed by unknown
authority
I doubt my change caused the failure, but I am not sure how to resolve
this yet.

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

yujuhong · 2015-02-04T19:18:15Z

e2e finally passed. The PR should be ready to merge.

bgrant0607 · 2015-02-04T19:34:37Z

Great! Re-running travis.

bgrant0607 · 2015-02-04T19:41:19Z

Travis passed. Shippable is 2 hours behind as usual. Merging.

Migrate API defaulting to a centralized location

alex-mohr assigned bgrant0607 Jan 28, 2015

bgrant0607 mentioned this pull request Jan 28, 2015

Adding an extensible resource spec to the API. #3841

Merged

bgrant0607 reviewed Jan 28, 2015
View reviewed changes

bgrant0607 mentioned this pull request Jan 29, 2015

Eliminate api/conversion.go #3914

Closed

bgrant0607 reviewed Jan 29, 2015
View reviewed changes

bgrant0607 mentioned this pull request Jan 31, 2015

Convert load of api/examples/pod.json into native Go definition of the api.Pod #3995

Merged

thockin and others added 2 commits February 2, 2015 22:35

Sketch: a third take on defaulting values

1ddb68d

yujuhong force-pushed the defaults branch from 36db790 to 4a72add Compare February 3, 2015 08:58

bgrant0607 added a commit that referenced this pull request Feb 4, 2015

Merge pull request #3854 from yujuhong/defaults

3f28bad

Migrate API defaulting to a centralized location

bgrant0607 merged commit 3f28bad into kubernetes:master Feb 4, 2015

bgrant0607 mentioned this pull request Feb 6, 2015

Sketch: a third take on defaulting values #2587

Closed

yujuhong deleted the defaults branch March 5, 2015 18:03

yujuhong mentioned this pull request Mar 6, 2015

Test fixtures to generate valid objects for testing #5121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate API defaulting to a centralized location #3854

Migrate API defaulting to a centralized location #3854

yujuhong commented Jan 28, 2015

alex-mohr commented Jan 28, 2015

yujuhong commented Jan 28, 2015

bgrant0607 commented Jan 28, 2015

bgrant0607 Jan 28, 2015

bgrant0607 Jan 29, 2015

smarterclayton commented Jan 28, 2015

bgrant0607 Jan 28, 2015

bgrant0607 Jan 29, 2015

thockin commented Jan 30, 2015

bgrant0607 commented Jan 30, 2015

thockin commented Feb 3, 2015

yujuhong commented Feb 3, 2015

bgrant0607 commented Feb 3, 2015

thockin commented Feb 3, 2015

yujuhong commented Feb 3, 2015

yujuhong commented Feb 4, 2015

thockin commented Feb 4, 2015

yujuhong commented Feb 4, 2015

bgrant0607 commented Feb 4, 2015

bgrant0607 commented Feb 4, 2015

Migrate API defaulting to a centralized location #3854

Migrate API defaulting to a centralized location #3854

Conversation

yujuhong commented Jan 28, 2015

alex-mohr commented Jan 28, 2015

yujuhong commented Jan 28, 2015

bgrant0607 commented Jan 28, 2015

bgrant0607 Jan 28, 2015

Choose a reason for hiding this comment

bgrant0607 Jan 29, 2015

Choose a reason for hiding this comment

smarterclayton commented Jan 28, 2015

bgrant0607 Jan 28, 2015

Choose a reason for hiding this comment

bgrant0607 Jan 29, 2015

Choose a reason for hiding this comment

thockin commented Jan 30, 2015

bgrant0607 commented Jan 30, 2015

thockin commented Feb 3, 2015

yujuhong commented Feb 3, 2015

bgrant0607 commented Feb 3, 2015

thockin commented Feb 3, 2015

yujuhong commented Feb 3, 2015

yujuhong commented Feb 4, 2015

thockin commented Feb 4, 2015

yujuhong commented Feb 4, 2015

bgrant0607 commented Feb 4, 2015

bgrant0607 commented Feb 4, 2015