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

Kubernetes: Failure when codecgen is run on anonymous type, but not on wrapping type. #100

Closed
wojtek-t opened this issue Sep 15, 2015 · 19 comments

Comments

@wojtek-t
Copy link

To reproduce I'm using:
kubernetes/kubernetes@master...wojtek-t:use_ugorij#diff-22207499f9ab0d60831b9c388397f034R1

Input data:
kubernetes/kubernetes@master...wojtek-t:use_ugorij#diff-29f2371dad30835b344a6ca05b473eaeR1

I did NOT run codecgen for the file that contains definition of that jsons:
https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/scheduler/api/types.go

IIUC, this should result in falling back to encoding/json (which works fine). However this is not the case (i.e. the output is empty)

@ugorji

@ugorji
Copy link
Owner

ugorji commented Sep 15, 2015

Please write the error you get, and what you expect.

@wojtek-t
Copy link
Author

What I get is:

api.Policy{TypeMeta:api.TypeMeta{Kind:"", APIVersion:""}, Predicates:[]api.PredicatePolicy(nil), Priorities:[]api.PriorityPolicy(nil)}
api.Policy{TypeMeta:api.TypeMeta{Kind:"", APIVersion:""}, Predicates:[]api.PredicatePolicy{api.PredicatePolicy{Name:"MatchNodeSelector", Argument:(_api.PredicateArgument)(nil)}, api.PredicatePolicy{Name:"PodFitsResources", Argument:(_api.PredicateArgument)(nil)}, api.PredicatePolicy{Name:"PodFitsPorts", Argument:(_api.PredicateArgument)(nil)}, api.PredicatePolicy{Name:"NoDiskConflict", Argument:(_api.PredicateArgument)(nil)}, api.PredicatePolicy{Name:"TestServiceAffinity", Argument:(_api.PredicateArgument)(0xc8201a57c0)}, api.PredicatePolicy{Name:"TestLabelsPresence", Argument:(_api.PredicateArgument)(0xc8201a5870)}}, Priorities:[]api.PriorityPolicy{api.PriorityPolicy{Name:"LeastRequestedPriority", Weight:1, Argument:(_api.PriorityArgument)(nil)}, api.PriorityPolicy{Name:"ServiceSpreadingPriority", Weight:2, Argument:(_api.PriorityArgument)(nil)}, api.PriorityPolicy{Name:"TestServiceAntiAffinity", Weight:3, Argument:(_api.PriorityArgument)(0xc8201a59b0)}, api.PriorityPolicy{Name:"TestLabelPreference", Weight:4, Argument:(_api.PriorityArgument)(0xc8201a5a70)}}}

So encoding/json correctly parses that input, however codec doesn't parse anything.
I would expect the output to be exactly the same in both cases.

@wojtek-t
Copy link
Author

To be clear:

  • the first api.Policy is what is generated by codec (without running codecgen before).
  • the second one is what is generated by encoding/json

@ugorji
Copy link
Owner

ugorji commented Sep 15, 2015

Weird. I cannot reproduce this.

I ran the test locally and it all works fine. I didn't test codecgen, but I tested codec with []byte and io.Reader, and compared against using encoding/json. I got similar outputs.

I think there may be something environmental going on here.

BTW, codec never falls back to encoding/json.

codec works by first checking if the type implements codec.Selfer, and delegating to that. You can use codecgen to generate a static implementation of codec.Selfer (which fully bypasses reflection except where it sees an interface).

BTW, you will usually know if you are using codecgen, because you will get much better than 2X (especially for encoding). My benchmarks show results of 2X over encoding/json in reflection-mode for encoding and decoding, and 13X and 3X in codecgen-mode for encoding and decoding.

PASS
Benchmark__Std_Json___Encode        2000        350409 ns/op       13848 B/op         97 allocs/op
Benchmark__Std_Json___Decode         300       1936979 ns/op       16120 B/op        485 allocs/op
ok      ugorji.net/codec    1.569s
prebuild done successfully
PASS
Benchmark__Json_______Encode        3000        178794 ns/op        9424 B/op         52 allocs/op
Benchmark__Json_______Decode        1000       1082549 ns/op       13328 B/op        320 allocs/op
ok      ugorji.net/codec    1.795s
prebuild done successfully
PASS
Benchmark__Json_______Encode       30000         27368 ns/op         832 B/op          4 allocs/op
Benchmark__Json_______Decode        1000        634929 ns/op        9544 B/op        194 allocs/op
ok      ugorji.net/codec    1.827s

@ugorji
Copy link
Owner

ugorji commented Sep 15, 2015

@wojtek-t

@wojtek-t
Copy link
Author

@ugorji - did you generated the files? I'm reproducing it 100% times.

@wojtek-t
Copy link
Author

I've added "ErrorIfNoField = true" and what I get now is:

panic: no matching struct field found when decoding stream map with key predicates

source:
kubernetes/kubernetes@master...wojtek-t:test#diff-22207499f9ab0d60831b9c388397f034R1
test file:
kubernetes/kubernetes@master...wojtek-t:test#diff-29f2371dad30835b344a6ca05b473eaeR1

@wojtek-t
Copy link
Author

I think I've got something. I had the following stacktrace:

goroutine 1 [running]:
github.com/ugorji/go/codec.(*Decoder).errorf(0xc82018f9e0, 0xe229a0, 0x43, 0xc8200a7910, 0x1, 0x1)
/usr/local/google/home/wojtekt/go/src/github.com/ugorji/go/codec/decode.go:1486 +0x85
github.com/ugorji/go/codec.(*Decoder).structFieldNotFound(0xc82018f9e0, 0xffffffffffffffff, 0xc8201e27a0, 0xa)
/usr/local/google/home/wojtekt/go/src/github.com/ugorji/go/codec/decode.go:1450 +0x215
github.com/ugorji/go/codec.genHelperDecoder.DecStructFieldNotFound(0xc82018f9e0, 0xa, 0xffffffffffffffff, 0xc8201e27a0, 0xa)
/usr/local/google/home/wojtekt/go/src/github.com/ugorji/go/codec/gen-helper.generated.go:151 +0x3f
k8s.io/kubernetes/pkg/api.(*TypeMeta).codecDecodeSelfFromMap(0xc82015f900, 0xffffffffffffffff, 0xc82018f9e0)
/usr/local/google/home/wojtekt/go/src/k8s.io/kubernetes/pkg/api/types.generated.go:227 +0x4d4
k8s.io/kubernetes/pkg/api.(*TypeMeta).CodecDecodeSelf(0xc82015f900, 0xc82018f9e0)
/usr/local/google/home/wojtekt/go/src/k8s.io/kubernetes/pkg/api/types.generated.go:173 +0x177
github.com/ugorji/go/codec.(*Decoder).decode(0xc82018f9e0, 0xc39800, 0xc82015f900)
/usr/local/google/home/wojtekt/go/src/github.com/ugorji/go/codec/decode.go:1182 +0x8d0
github.com/ugorji/go/codec.(*Decoder).MustDecode(0xc82018f9e0, 0xc39800, 0xc82015f900)
/usr/local/google/home/wojtekt/go/src/github.com/ugorji/go/codec/decode.go:1117 +0x35
main.main()
/usr/local/google/home/wojtekt/go/src/k8s.io/kubernetes/reproduce.go:24 +0x362

It suggests, that Codec is trying to decode TypeMeta. However, there are no Typemeta-related fields in the input, so it should just parse predicated &pririties fields.

@ugorji
Copy link
Owner

ugorji commented Sep 16, 2015

@wojtek-1 your bug report and comments said you DID NOT run codecgen. But this stack trace is from codecgen. That's why I always ask you for your output and expected output. I've been looking at standard reflection+based mode (ie without codecgen generated Selfer implementation.

Will run codecgen and look again.

@wojtek-t
Copy link
Author

@ugorji - no i did NOT run codecgen

@wojtek-t
Copy link
Author

or maybe to be more clear:

  • i did run codecgen for pkg/api/types.go
  • i did NOT run codecgen for plugin/pkg/scheduler/api/types.go

@ugorji
Copy link
Owner

ugorji commented Sep 16, 2015

@wojtek-t Thanks. I can reproduce it now. I was previously running codecgen on go-get folder of github.com/kubernetes/kubernetes/pkg/api/ instead of k8s.io/kubernetes/pkg/api .

The issue has to do with anonymous fields. I'm looking into it now.

@wojtek-t
Copy link
Author

Awesome - thanks!

@wojtek-t
Copy link
Author

FYI - I will be on vacation for a week from starting tomorrow and it's still not clear if someone picks it up from me or I will get back to it once I'm back.

@ugorji
Copy link
Owner

ugorji commented Sep 16, 2015

Ok. Well, let's try to wrap it up before you leave then ;)

ugorji added a commit that referenced this issue Sep 16, 2015
This effectively calls itself recursively and leads to an expected stackoverflow.
To fix, check if the variable name == the constant variable name of the type in the
Selfer implementation (x). If it is, then do not attempt to call the Selfer method.

In addition, the following fixes were applied:

When encoding extensions, use an address if the kind is not Struct or Array.
This way, we don't copy unnecessarily.

When encoding based on the known interfaces which may be implemented
i.e. encoding.(Binary|Text)(M|Unm)arshaler, json.(M|Unm)arshaler, codec.Selfer,
the value to encode may not be addressable but the implementation is on a pointer.
In that situation, copy the value to an addressable value and call the implementation there.

Updates #100
@ugorji
Copy link
Owner

ugorji commented Sep 16, 2015

@wojtek-t

The problem was that you ran codecgen on a value (e.g. api.TypeMeta) which you then embedded,
causing the value to be a codec.Selfer.
This effectively made the "wrapping" type (ie api.Policy) a codec.Selfer also.
This is how go works.

To fix, you MUST also run codecgen on the containing types, so that they have their own
implementation of codec.Selfer, and that gets used.

Note: You will have the same issue if you embed anything that is a
encoding.(Binary|Text)(M|Unm)arshaler, json.(M|Unm)arshaler, codec.Selfer .
The "wrapping/containing" type will then have that implementation, and that will be delegated to.

The key thing to note here is that, when using anonymous fields, the methods are inherited.
So you have to manage that by ensuring all "containing" types have the methods specified or they
will delegate to the child which may be a problem.

Please update the source and run codecgen on k8s.io/kubernetes/plugin/pkg/scheduler/api .

@wojtek-t
Copy link
Author

yes - generating this file to the main types helps (also removing generated types for pkg/api/types.go helps).

That's sad that it works like that, but I understand the explanation. Thanks a lot for help!

@ugorji ugorji changed the title Codec doesn't work without generated methods Kubernetes: Failure when codecgen is run on anonymous type, but not on wrapping type. Sep 16, 2015
@ugorji
Copy link
Owner

ugorji commented Sep 16, 2015

@wojtek-t closing this as there isn't anything that can be done; this is the way "go" works.

@ugorji ugorji closed this as completed Sep 16, 2015
@ugorji
Copy link
Owner

ugorji commented Sep 16, 2015

@wojtek-t Another thing to note:

Given a type T, T's method set does not include methods bound to *T. So if you embed api.TypeMeta, then the Selfer methods are not "inherited". However, since you embed *api.TypeMeta, then its Selfer methods are "inherited".

Go's embedded method inheritance is "wonky" :( and is good when it's useful, but it's bad at times like these :(

ugorji added a commit that referenced this issue Sep 16, 2015
Some helper methods have changed. Consequently, formerly
generated code will no longer work.

Update GenVersion to reflect that, and force users to re-generate.

Update #101 #100 ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants