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

Add a helper function to decode scheduler plugin args #80696

Merged
merged 1 commit into from Aug 27, 2019

Conversation

@hex108
Copy link
Member

commented Jul 29, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Most plugins need to decode their args. Then plugin authors need to figure out what is runtime.Unkonw and how to decode it. This PR adds a helper function DecodeInto to decode scheduler plugin args whose type is runtime.Unkown, it will be more convenient.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add a helper function to decode scheduler plugin args.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


/sig scheduling

@k8s-ci-robot k8s-ci-robot requested review from ahg-g and wgliang Jul 29, 2019
@hex108 hex108 force-pushed the hex108:plugin_args branch 2 times, most recently from bd4ec59 to cafda7c Jul 29, 2019
@hex108

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

It will be useful for plugin developers to decode plugin's args. cc @bsalamat @ahg-g

@hex108 hex108 force-pushed the hex108:plugin_args branch from cafda7c to 3c9d8dc Aug 2, 2019
@hex108

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

/retest

data = configuration.Raw
case runtime.ContentTypeYAML:
var err error
if data, err = yaml.ToJSON(configuration.Raw); err != nil {

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Aug 2, 2019

Member

why not decode directly?

This comment has been minimized.

Copy link
@hex108

hex108 Aug 5, 2019

Author Member

I think either one is OK. No strong tendency for it. Just follow the pattern in https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/yaml/yaml.go#L40.

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Aug 6, 2019

Member

TBH, I would change the code in apimachinery. It adds unnecessary processing.

This comment has been minimized.

Copy link
@hex108

hex108 Aug 6, 2019

Author Member

I tried to find the answer, and found it at http://ghodss.com/2014/the-right-way-to-handle-yaml-in-golang/. And https://github.com/kubernetes-sigs/yaml exists for this reason, this library first converts YAML to JSON using go-yaml and then uses json.Marshal and json.Unmarshal to convert to or from the struct. .

This comment has been minimized.

Copy link
@hex108

hex108 Aug 6, 2019

Author Member

It seems better to use k8s.sigs.io/yaml to unmashal yaml directly. Updated.

This comment has been minimized.

Copy link
@alculquicondor

alculquicondor Aug 6, 2019

Member

Thanks. Much better to delegate this pattern to a single library.

@hex108 hex108 force-pushed the hex108:plugin_args branch from 3c9d8dc to f657e69 Aug 6, 2019
@alculquicondor

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

/lgtm

@wgliang
wgliang approved these changes Aug 7, 2019
@hex108

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

/assign @bsalamat @ahg-g

@hex108

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

/assign @Huang-Wei
for another review.

- name: foo
args:
foo_test: "test decode"
`,

This comment has been minimized.

Copy link
@ahg-g

ahg-g Aug 26, 2019

Member

nit: can you please fix the indentation here and the one above.

This comment has been minimized.

Copy link
@hex108

hex108 Aug 27, 2019

Author Member

YAML is a special case, if adding indentation, an error(found character that cannot start any token) will occur when decoding, so other places also declare a YAML string like this, e.g. https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/yaml/decoder_test.go#L34

This comment has been minimized.

Copy link
@hex108

hex108 Aug 27, 2019

Author Member

Fixed the above one. PTAL

@hex108 hex108 force-pushed the hex108:plugin_args branch from f657e69 to 6324285 Aug 27, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 27, 2019
@hex108

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

/retest

@ahg-g

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 27, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, hex108

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 9aa76b2 into kubernetes:master Aug 27, 2019
24 checks passed
24 checks passed
cla/linuxfoundation hex108 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
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-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 27, 2019
@hex108 hex108 deleted the hex108:plugin_args branch Aug 28, 2019
@Adirio

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

Is this function expected to be used here @hex108?

func New(config *runtime.Unknown, _ framework.FrameworkHandle) (framework.Plugin, error) {

Providing an example YAML or JSON in a comment in that example and modifying the example to use it would be useful.

@hex108

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2019

Is this function expected to be used here

@Adirio Yes, it is. I'll add it to one of examples. Thanks!

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.