Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support both JSON and YAML for scheduler configuration #75857

Merged
merged 3 commits into from Mar 31, 2019

Conversation

@danielqsj
Copy link
Member

commented Mar 29, 2019

What type of PR is this?

/kind feature
/sig scheduling

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #75852

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Support both JSON and YAML for scheduler configuration.

@danielqsj danielqsj force-pushed the danielqsj:sc1 branch from 59c3dc8 to 697ed2c Mar 29, 2019

@danielqsj

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

@k8s-ci-robot k8s-ci-robot requested a review from Huang-Wei Mar 29, 2019

@Huang-Wei

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Could you help add an unit test?

@danielqsj

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

@Huang-Wei added test cases for yaml format scheduler config, PTAL

@danielqsj

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

/retest

@@ -162,6 +162,70 @@ func TestSchedulerCreationFromConfigMap(t *testing.T) {
),
expectedPrioritizers: sets.NewString(),
},
{

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Mar 30, 2019

Member

This actually doesn't cover the changes this PR introduced - it's creating a ConfigMap through apiserver.

What the test needs to cover is:

// initPolicyFromFile initialize policy from file
func initPolicyFromFile(policyFile string, policy *schedulerapi.Policy) error {

Writing a simple unit test should be good enough.

This comment has been minimized.

Copy link
@danielqsj

danielqsj Mar 31, 2019

Author Member

These test cases used for yaml format config in ConfigMap which is not supported before this PR due to the codec is same as initPolicyFromFile.

Thanks for your tips, I added a unit test for initPolicyFromFile. PTAL

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Mar 31, 2019

@Huang-Wei
Copy link
Member

left a comment

/lgtm
/approve

Thanks @danielqsj !

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielqsj, Huang-Wei

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 2792f1a into kubernetes:master Mar 31, 2019

16 of 17 checks passed

tide Not mergeable. Needs approved, lgtm labels.
Details
cla/linuxfoundation danielqsj authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
@@ -44,10 +45,11 @@ var Codec runtime.Codec

func init() {
jsonSerializer := json.NewSerializer(json.DefaultMetaFactory, schedulerapi.Scheme, schedulerapi.Scheme, true)

This comment has been minimized.

Copy link
@misterikkit

misterikkit Apr 1, 2019

Contributor

I know I'm late to this PR, but according to the godoc, json.NewSerializer returns an object that can parse JSON or YAML bytes. Are those docs wrong?

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Apr 1, 2019

Member

Probably, it turns out to be working for both JSON and YAML.

This comment has been minimized.

Copy link
@misterikkit

misterikkit Apr 2, 2019

Contributor

I guess I'm just surprised that it wasn't working before this PR, because the docs imply that it should have worked. Clearly @danielqsj would not have sent this unless it was broken.

This comment has been minimized.

Copy link
@danielqsj

danielqsj Apr 2, 2019

Author Member

I checked it again.
After remove yaml.NewDecodingSerializer, unit test failed:

=== RUN   TestInitPolicyFromFile
--- FAIL: TestInitPolicyFromFile (0.00s)
    scheduler_test.go:1011: Failed writing a policy config file: invalid policy: couldn't get version/kind; json parse error: invalid character 'a' looking for beginning of value
FAIL

And the doc of json.NewSerializer is

// NewSerializer creates a JSON serializer that handles encoding versioned objects into the proper JSON form. If typer
// is not nil, the object has the group, version, and kind fields set.

It seems the return object can only parse JSON, right?

So I turned to yaml.NewDecodingSerializer whose purpose is to support YAML decoding.

// NewDecodingSerializer adds YAML decoding support to a serializer that supports JSON.
func NewDecodingSerializer(jsonSerializer runtime.Serializer) runtime.Serializer {
	return &yamlSerializer{jsonSerializer}
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.