component-base/logs: improve handling of re-applying a configuration #117108
Conversation
|
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Wed Apr 5 10:36:42 UTC 2023. |
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
|
/retest |
|
/triage accepted |
1 similar comment
|
/triage accepted |
|
/assign @liggitt Can you perhaps take a look? You were involved in designing the |
| default: | ||
| return fmt.Errorf("invalid value %d for ReapplyHandling", ReapplyHandling) | ||
| } | ||
| } |
There was a problem hiding this comment.
would it make more sense to actually detect if the config being applied differed, and always error if so?
that would avoid false positives where identical config was specified, and avoid silently ignoring config changes in tests
There was a problem hiding this comment.
I think it should continue to be an error in applications (iReapplyHandlingError) f they apply twice, even if the config is the same: the config might only be the same under the current circumstances and different under others. I think it's better to fail under all circumstances because it makes obvious that something is wrong.
Comparing configs might make sense for ReapplyHandlingIgnore: if an integration test tries to run with different configs, then it's better to raise that as an error instead of silently ignoring it.
For ReapplyHandlingAllow it's most likely a different config, so we can compare and skip if equal, it just won't change much.
There was a problem hiding this comment.
collapsing the test-caller cases to ignore a reapply if identical and error if different seems good... if a test is reapplying a different config it is not testing what it thinks it is, and I would want to know so I could move my test to a different package or something to get an isolated test process
There was a problem hiding this comment.
The unit tests where ReapplyHandlingAllow is used do need to apply different configs - that's their purpose 😅
So combining both cases doesn't work. I've pushed a version where the config get compared for the ReapplyHandlingIgnore case.
There was a problem hiding this comment.
I'm saying we should not have ReapplyHandlingAllow, only ReapplyHandlingIgnoreIfEqual and ReapplyHandlingError
There was a problem hiding this comment.
Some of those unit tests are elsewhere:
There was a problem hiding this comment.
Some of those unit tests are elsewhere:
I would move the unit tests, either into the package that can reset the state, or into separate integration packages, rather than enable an "ignore ineffective config" option
There was a problem hiding this comment.
either into the package that can reset the state
That would be k8s.io/component-base/logs/api/v1. Then that package inherits new dependencies (like zapr) that are undesirable and/or we get circular dependencies.
separate integration packages
Isn't that what test/integration/logs/benchmark/load_test.go is? How would such a package reset the state in k8s.io/component-base/logs/api/v1 if we don't offer an API for that?
Hmm, perhaps we can add a k8s.io/component-base/logs/api/v1.Reset? That has to come with a big warning that it should only be used when no goroutines are running and is only meant for such unit tests - basically the same semantic as ReapplyHandlingAllow, except that it is a separate call.
There was a problem hiding this comment.
that seems ok. we've done things like that before:
// SetServiceResolverForTests allows the service resolver to be overridden during tests.
// Tests using this function must run serially as this function is not safe to call concurrently with server start.
func SetServiceResolverForTests(resolver webhook.ServiceResolver) func() {// SetForTests sets capabilities for tests. Convenience method for testing. This should only be called from tests.
func SetForTests(c Capabilities) {There was a problem hiding this comment.
0bbac15
to
47b086e
Compare
47b086e
to
f9334fe
Compare
|
Rebased due to a conflict. |
f9334fe
to
64e1273
Compare
|
This PR is getting tested in combination with enabled race detection over in #117108. |
64e1273
to
4c953f5
Compare
4c953f5
to
c031805
Compare
| // Reset restores the default settings. This is not thread-safe and should only | ||
| // be used when there are no goroutines running. The intented users are unit | ||
| // tests in other packages. | ||
| func Reset(featureGate featuregate.FeatureGate) error { |
There was a problem hiding this comment.
| // Reset restores the default settings. This is not thread-safe and should only | |
| // be used when there are no goroutines running. The intented users are unit | |
| // tests in other packages. | |
| func Reset(featureGate featuregate.FeatureGate) error { | |
| // ResetForTest restores the default settings. This is not thread-safe and should only | |
| // be used when there are no goroutines running. The intended users are unit | |
| // tests in other packages. | |
| func ResetForTest(featureGate featuregate.FeatureGate) error { |
c031805
to
bd4b902
Compare
Normal binaries should never have to do this. It's not safe when there are already some goroutines running which might do logging. Therefore the new default is to return an error when a binary accidentally re-applies. A few unit ensure that there are no goroutines and have to call the functions more then once. The new ResetForTest API gets used by those to enable changing the logging settings more than once in the same process. Integration tests use the same code as the normal binaries. To make reuse of that code safe, component-base/logs can be configured to silently ignore any additional calls. This addresses data races that were found when enabling -race for integration tests. To catch cases where the integration test does want to modify the config, the old and new config get compared and an error is raised when it's not the same. To avoid having to modify all integration tests which start test servers, reconfiguring component-base/logs is done by the test server packages.
bd4b902
to
02efe09
Compare
|
/lgtm |
|
LGTM label has been added. Git tree hash: ce34b562b69c93999bd078d18803ceaffbb46fdb
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, pohly 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 |
|
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
2 similar comments
|
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
|
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
What type of PR is this?
/kind bug
/kind failing-test
What this PR does / why we need it:
Normal binaries should never have re-apply the configuration. It's not safe when there are already some goroutines running which might do logging. Therefore the new default is to return an error when a binary accidentally re-applies.
The unit tests of component-base/logs ensure that there are no goroutines and have to call the functions more than once.
Integration tests use the same code as the normal binaries. To make reuse of that code safe, component-base/logs can be configured to silently ignore any additional calls. This addresses data races that were found when enabling -race for integration tests.
To avoid having to modify all integration tests which start test servers, reconfiguring component-base/logs is done by the test server packages.
Which issue(s) this PR fixes:
Data race in k8s.io/kubernetes/test/integration/serving.TestComponentSecureServingAndAuth.
Does this PR introduce a user-facing change?