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

fatal error: concurrent map read and map write #5317

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

tdiesler
Copy link
Contributor

@tdiesler tdiesler commented Apr 3, 2024

No description provided.

@tdiesler tdiesler force-pushed the ghi5315 branch 3 times, most recently from 1901bc7 to 9a20589 Compare April 3, 2024 13:52
@christophd
Copy link
Contributor

I tried to solve this in the E2E test setup (with mutex lock) where the clients get instantiated (see https://github.com/apache/camel-k/blob/main/e2e/support/test_support.go#L165).

But looking closer at the code I see that the kamel install command will always use a new client instance and this can lead to the concurrent map write. So the proposed solution here makes sense to me. I do not know if this synchronization has some other side effects though.

How about synchronizing the NewOutOfClusterClient func which is used by kamel CLI so we do not harm any operator client usage? Also I would remove the mutex lock that I have introduced in the test support then to avoid duplicate locking.

@tdiesler
Copy link
Contributor Author

tdiesler commented Apr 8, 2024

There are (currently) three usages of apis.AddToScheme

  • when there is an error on operator startup
  • with some "fake" client while testing
  • whenever a client with config is needed

I ignored the first two and believe it is correct to sync in NewClientWithConfig which IMHO is close enough to runtime.SchemeBuilder.AddToScheme where the problem actually is. The idea is, that it is probably ok to sync the creation of a non-trivial k8s client, which apparently does work sensitive to concurrency issues. At the same time, I assume that creation of such a client is fast enough that it does not need to happen in parallel on the same Go runtime.

Sure, lets try to remove that other mutex

@tdiesler
Copy link
Contributor Author

tdiesler commented Apr 9, 2024

PR validation fails in kamel-build-bundle/build-index-image.sh for install/olm and install/upgrade

Constructing index image ...
time="2024-04-09T07:25:46Z" level=fatal msg="package \"camel-k\" has duplicate bundle \"camel-k-operator.v2.3.0\""
Error: Process completed with exit code 1.

@tdiesler
Copy link
Contributor Author

tdiesler commented Apr 9, 2024

This is now failing with https://issues.redhat.com/browse/CMLK-1928

@squakez
Copy link
Contributor

squakez commented Apr 9, 2024

This is now failing with https://issues.redhat.com/browse/CMLK-1928

It was already reported in #5326

squakez
squakez previously requested changes Apr 9, 2024
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

Sorry. We are not going to ignore any test. You can restart it or fix it, as you prefer. Thanks.

@squakez squakez dismissed their stale review April 9, 2024 15:18

commit amended

@christophd christophd merged commit 6b1b5f6 into apache:main Apr 9, 2024
15 checks passed
@tdiesler tdiesler deleted the ghi5315 branch April 9, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants