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.

Sign up for GitHub

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

Jump to bottom

🐛 Fix informer cache creating pointers to pointers #796

Merged
merged 3 commits into from
Feb 26, 2020
Merged

🐛 Fix informer cache creating pointers to pointers #796

merged 3 commits into from
Feb 26, 2020

Conversation

alvaroaleman
Copy link
Member

Makes the cache cope with list types that have a slice of pointers as Items field.

Fixes #656
Supersedes and thereby closes #667

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 13, 2020
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 13, 2020
alvaroaleman
alvaroaleman commented Feb 13, 2020
View reviewed changes
pkg/controller/controller_suite_test.go Outdated
Spec string `json:"spec,omitempty"`
}

// DeepCopyObject implements runtime.Object
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I handwrote these mostly because I was too lazy to figure out how to plug deepcopy-gen into a hackscript and have it verified by CI. Also it may not be able to cope with a folder that contains multiple packages (controller and controller_test). Since the correctness of the DeepCopies is not really relevant for the test, thats IMHO good enough

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment with the above? :)

@DewaldV
Copy link
Contributor

DewaldV commented Feb 14, 2020 •
edited
Loading

Awesome job on the test @alvaroaleman! I got about 50% of the way there when I attempted it last year but had brought a lot more code along so this looks much cleaner than my test implementation. 😄

@alvaroaleman
Copy link
Member Author

Awesome job on the test @alvaroaleman! I got about 50% of the way there when I attempted it last year but had brought a lot more code along so this looks much cleaner than my test implementation. smile

Yeah it actually turned out to be a lot more involved than anticipated, initially I thought I could directly test the informerCache but that isn't really possible as it embedds a *InformersMap, so mocking that would require some more refactorings. So I ended up just writing an integration test 🙃

@alvaroaleman
Copy link
Member Author

@vincepri Mind having a look?

vincepri
vincepri reviewed Feb 18, 2020
View reviewed changes
pkg/controller/controller_suite_test.go Outdated
Spec string `json:"spec,omitempty"`
}

// DeepCopyObject implements runtime.Object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment with the above? :)

pkg/cache/informer_cache.go
@@ -93,7 +93,11 @@ func (ip *informerCache) List(ctx context.Context, out runtime.Object, opts ...c
}
// http://knowyourmeme.com/memes/this-is-fine
elemType := reflect.Indirect(reflect.ValueOf(itemsPtr)).Type().Elem()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity, can we also add a comment here explaining what we're trying to do and why? I checked out the code and understood we're pretty much trying to figure out the type of the list passed in, but that might be a lot to ask for folks not familiar with the codebase.

Let's also move the logic to a local function and add some unit tests for it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, PTAL. I had to move the UnconventionalListType into a different package, because its now used by both unit and integration tests and would otherwise have caused an import cycle.

@vincepri
Copy link
Member

/assign

@vincepri vincepri added this to the v0.5.x milestone Feb 21, 2020
@alvaroaleman alvaroaleman force-pushed the construct-pointer-for-non-pointers-only-with-test branch from aa7cb59 to c35cff4 Compare February 23, 2020 11:42
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 23, 2020
DewaldV and others added 2 commits February 23, 2020 12:44
@DewaldV @alvaroaleman
Construct pointer to cache type for non-pointers only.
f024e45 
When the list type is already a pointer this code results in a cache type of **Type which can't be cast to runtime.Object.
This adds a check to the function to only grab a pointer to a type if it is not already a ptr type.
@alvaroaleman
Test list type handlings that have a slice of pointers as items field
0b6855d
@alvaroaleman alvaroaleman force-pushed the construct-pointer-for-non-pointers-only-with-test branch from c35cff4 to e66416c Compare February 23, 2020 11:49
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2020
@alvaroaleman alvaroaleman force-pushed the construct-pointer-for-non-pointers-only-with-test branch from e66416c to bd6a4b5 Compare February 23, 2020 11:51
vincepri
vincepri approved these changes Feb 24, 2020
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/assign @DirectXMan12 @gerred

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2020
@DirectXMan12
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, DirectXMan12

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2020
@k8s-ci-robot k8s-ci-robot merged commit 0374b8c into kubernetes-sigs:master Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincepri vincepri vincepri approved these changes

@mengqiy mengqiy Awaiting requested review from mengqiy

Assignees

@DirectXMan12 DirectXMan12

@gerred gerred

@vincepri vincepri

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.5.x
Development

Successfully merging this pull request may close these issues.

Unable to List Objects with pointer slices
6 participants
@alvaroaleman @DewaldV @vincepri @DirectXMan12 @k8s-ci-robot @gerred