kubernetes/util: fix ResourceContains method and add unit test #49
Conversation
ghost
mentioned this pull request
|
Hi. While we appreciate a now-improved PR, there is a fundamental problem with ResourceContains: it is veeeery, very, very slow. Running Seiso against an image stream with 100 tags and a frontend/backend application stack could easily take 3 minutes, because it queries the same resources again and again. As you can see in https://github.com/appuio/seiso/blob/master/pkg/kubernetes/util.go#L34, there's a client listing the resource with every call. There is no caching at all. And since the business logic calls this method with every single image tag, the number of kubernetes API calls becomes almost exponential. If you would extend your PR (or create a new one) that restructures the method in a way that the kubernetes objects to be queried are passed in as parameters or another solution that lists the resources only at first access, we would be grateful. Let me know if you intend to address this also in this PR, otherwise we can also merge this and address the performance problem another time. |
You are welcome :)
I understand the concerns, although I think I don't have either enough knowledge in go / kubernetes, neither time, to resolve this caching issue. I'm sorry I think I won't be able to help on this subject, so it would be better to merge this PR to fix the current behavior :) |
|
Also, it's fine to have one single commit, not really worth to have two in this case. More importantly, we would prefer to not have commit messages that are cut off after 70 characters. So we usually use commit title to summarize the change in an easy and human-understandable sentence, and then the longer commit description for its reasoning (if any). Using commit prefixes isn't necessary, so you gain some more characters :) |
Trim
force-pushed
the
test-kubernetes-util
branch
from
23741af
to
a6dccbf
Compare
|
I've just made the requested changes and rebased on current master. |
The ResourceContains method was checking only if first resource were containing the requested value. For example, if first element did not have the value but the second did, ResourceContains returned false, because it did not check the second element. The loop is moved outside to easily unit test ResourceContains: otherwise it would require to mock kubernetes client and its methods. As the new unit test is very similar to the one for ObjectContains, we removed the later and extended metadata field in mock object values to cover all ObjectContains case too.
Trim
force-pushed
the
test-kubernetes-util
branch
from
a6dccbf
to
e870fe1
Compare
|
I've just fixed a copy/paste issue for first test case. |
Hello,
The idea of this PR is the same I proposed in #46, but with the addition of unit testing the value lookup of the original
ResourceContainsmethod.First, I wanted to unit test directly the
ResourceContainsmethod, but I wasn't able to correctly create mocks for theclientInit()method and thek.client.Resource(resource).Namespace(namespace).List(metav1.ListOptions{})call.Then, I found it was simpler to split the list lookup from ResourceContains method and unit test it directly. I think that's a good enough way to unit test ResourceContains algorithm, because the same way was used for the
ObjectContains()method.The first commit of this PR creates the method
UnstructuredListContainswith original code fromResourceContainsand add unit tests for it. So, you can confirm with this commit alone the unit test is not passing actually if the value looked for is contained in the second element.The second commit apply the fix from #46 and it fix correctly the unit test.
I didn't had back the logging line from #46 saying which active image stream was found. If you think it's interesting to add it, I can add a third commit to this PR.