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

Handle omitted LimitRange spec and/or spec.limits from k8s. #6928

Merged
merged 1 commit into from Feb 25, 2016

Conversation

cben
Copy link
Contributor

@cben cben commented Feb 24, 2016

[Review carefully, I'm new to MIQ, k8s, Ruby and Go. All and any criticism welcome.]

This hopefully solves crash seen in the wild: https://bugzilla.redhat.com/1305324 where spec.limits was nil.
It might be overzelous.

What I see from types.go is that LimitRange.Spec is json:"spec,omitempty" but LimitRangeSpec.Limits isn't, it should always be an array.
omitempty doc says:

The empty values are false, 0, any nil pointer or interface value, and any array, slice, map, or string of length zero.

This suggests that only spec missing entirely or :spec => {:limits => []} are the plausible scenarious;
:spec => nil, :spec => {} and :spec => {:limits => nil} shouldn't happen.
Nevertheless, limits being nil is what the traceback suggests?
But I see RecursiveOpenStruct returns nil for missing attributes, so I suspect what happenned in the traceback is that limits wasn't there. However shouldn't then spec have been omitempty'd and spec.limits crashed instead of spec.limits.each? I'm still confused...

  • Since ROS returns nil for missing attributes, limit_range.spec.try(:limits) || [] would also work (it passes the 5 test scenarios). IMHO try on spec helps readability clarifyng we expect spec to be missing, but perhaps all code here relies on nil-for-missing anyway?
  • Also I wasn't sure if || [], .to_a or an explicit condition skipping the loop is best.

This hopefully solves crash seen in the wild: https://bugzilla.redhat.com/1305324
where spec.limits was nil.
@cben
Copy link
Contributor Author

cben commented Feb 25, 2016

@miq-bot add_label providers/containers, bug

@cben
Copy link
Contributor Author

cben commented Feb 25, 2016

cc @moolitayer

chessbyte added a commit that referenced this pull request Feb 25, 2016
Handle omitted LimitRange spec and/or spec.limits from k8s.
@chessbyte chessbyte merged commit d8e649d into ManageIQ:master Feb 25, 2016
@chessbyte chessbyte added this to the Sprint 37 Ending Mar 7, 2016 milestone Feb 25, 2016
@cben cben deleted the handle-nil-limits branch June 23, 2016 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants