TestConfigMapController test flake #41419
Comments
Seems like a different failure mode |
|
@kubernetes/sig-federation-bugs who owns this test? |
|
Also a different failure mode again in #39567 |
|
@nikhiljindal @Kargakis @csbell is this issue release blocking? If not, please remove from 1.6 milestone |
|
I think this can be moved out of 1.6. It's a unit test problem, not a regression in functionality. There is an integration test pending that verifies the same functionality in a more realistic way (#43125). |
|
@perotinus you had also been looking into this. Can you give your opinion by 1PM PDT so we can triage it before the 1.6 burndown? |
|
I think this can be removed from 1.6; the flakiness that I've been tracking down is similar to this, so it doesn't appear to be a regression. Note that the flake described specifically by this issue is different from the ones I've been debugging, though #39567 seems similar to one of them. It looks like there are three classes of flakes happening:
|
|
Moving from 1.6 as appears known flake. |
|
@perotinus I think the configmap unit test should be removed in favor of the integration test proposed by #42025. I don't see the benefit to debugging and/or rewriting a bad unit test when the functionality in question could be better tested in integration anyway. Also, I'm intending the generic controller refactor (#41050) to include unit testing of corner cases not covered by the existing unit test or the proposed integration test. |
|
I am not entirely sure how integration tests have a greater benefit than unit tests. They all serve different purposes. Unit tests test the controller code paths in the absence of other components or faked components. Integration tests test the same code paths but with the other components actually running. More room for flakiness. Unit tests don't need etcd, don't need underlying clusters' API servers to be running whereas integration tests do. One of the real benefits in having unit tests is to have a faster debug cycles and more predictability. Unit tests should be less flaky than integration tests. If they are not, we should invest time in fixing them rather than deleting them. Integration tests can fail for reasons not in our control. Example, etcd failure or some such, but unit tests are immune to those types of failures. |
|
I agree with @madhusudancs that there is definite value in unit tests. I'm not familiar enough with the project to really have a good sense for whether the current suites of unit tests for the federation controllers are really the right tests. I prefer unit tests to integration tests (and integration tests to e2e tests), and I believe that these flaky unit tests are helping to reveal issues with the way our controllers work and mismatches between what we think they do and what they actually do, which is valuable. |
|
@madhusudancs @perotinus I'm not sure how my statement about the configmap unit test gave you the impression that I think https://testing.googleblog.com/2015/04/just-say-no-to-more-end-to-end-tests.html I do not think the current 'unit' tests for the federation controllers - including the one in question for the ConfigMap controller - typify good unit testing. They are brittle and prone to race conditions due to the way they attempt to fake out the api server. I think the flakiness is more a reflection of the test design than the functionality under test. There's also the issue of the monolithic nature of the existing tests - 100-200 lines each but providing relatively poor coverage due to targeting the controller as a whole rather than individual code paths. So yes, I think most of the coverage provided by the existing unit tests for the federation controllers can be better provided by the integration testing I'm proposing. And yes, I think better unit tests need to be written for the controllers. That will require refactoring the controllers to make such an effort tractable. The reconcile methods have too many dependencies and too many branches. And since the current controllers duplicate code instead of reusing it, the test surface is too large to justify providing good coverage for all of them individually. My proposal (starting with #41050) is to factor out the comment elements and ensure that the bulk of functionality can be targeted once rather than per-controller. Then type-specific functionality (e.g. ingress ip assignment, replicaset and deployment scheduling) can be targeted separately. |
Honestly, I don't know how else to interpret #41419 (comment) even after reading your clarification above. But anyway, without digressing, I agree with your point that our current testing of controllers as a whole in unit tests is sub-optimal given the way we setup fakes. But if you look at the general design of federated controllers, the bulk of the work is just replicating the objects in different API servers with some additional logic in some controllers. You can't just ignore the bulk of the work. But if that functionality is tested in a generic controller unit tests then that's fine. |
Automatic merge from submit-queue Fix federated config map unit tests Fixes #41419 and #42847 and possibly other issues in this area. cc: @nikhiljindal @csbell @perotinus
https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/41412/pull-kubernetes-unit/17829/#k8siokubernetesfederationpkgfederation-controllerconfigmap-testconfigmapcontroller
The text was updated successfully, but these errors were encountered: