Skip to content

Fixing some data races exposed during unit testing#319

Merged
lukeatdell merged 5 commits intomainfrom
usr/lukeatdell/unittestracefix
Sep 9, 2025
Merged

Fixing some data races exposed during unit testing#319
lukeatdell merged 5 commits intomainfrom
usr/lukeatdell/unittestracefix

Conversation

@lukeatdell
Copy link
Contributor

@lukeatdell lukeatdell commented Sep 5, 2025

Description

Intermittent data race warnings were being reported when running unit tests from the GitHub Actions.
Investigation showed there are several instances where InitLabelsAndAnnotations() is called from test code while other routines are running that concurrently access these label vars, causing the data race.

With the current implementation, a data race here should not be a concern, because the InitLabelsAndAnnotations() func is called once from main to set/build the label values, then they are only ever read, never written-to again, until it comes to the test code as mentioned above.

Changes allow the Replication Group monitor loop goroutine to be canceled using the provided context. Tests that are finished using the monitor loop should defer a call to the context's cancel() func for the context provided to the (*ReplicationGroupMonitor).Monitor() function.

GitHub Issues

GitHub Issue #
https://github.com/dell/csm/issues/1896

Checklist:

  • I have performed a self-review of my own code to ensure there are no formatting, vetting, linting, or security issues
  • I have verified that new and existing unit tests pass locally with my changes
  • I have not allowed coverage numbers to degenerate
  • I have maintained at least 90% code coverage
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have maintained backward compatibility

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

  • Replication e2e
    5 scenarios (5 passed)
    22 steps (22 passed)
    6m55.963946931s
    --- PASS: TestReplication (415.97s)
        --- PASS: TestReplication/Execute_repctl_replication_actions. (64.06s)
        --- PASS: TestReplication/Execute_repctl_replication_actions.#01 (113.62s)
        --- PASS: TestReplication/Execute_repctl_replication_actions.#02 (59.18s)
        --- PASS: TestReplication/Reprotect_at_the_target_cluster. (119.71s)
        --- PASS: TestReplication/Execute_an_unplanned_failover. (59.39s)
    PASS
    status 0
    ok      karavi-testing/karavi-replication/replication-test      552.903s
    

lukeatdell and others added 4 commits September 5, 2025 17:06
- utilizes a `sync.Once` struct to make sure a set of global vars is initialized only once.
- adds context done monitor to monitor goroutine to allow canceling the routine from tests.
r.Log.V(logger.InfoLevel).Info("Start monitoring replication-group")

dellCSIReplicationGroupsList := new(repv1.DellCSIReplicationGroupList)
select {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Annotation: this whole function's indentation was decreased because the ticker was pulled from here, to the Monitor() function above, but the diff makes it look more complicated than that, unfortunately.

Pulling that ticker up allows us to use a select statement on the ticker and the ctx.Done channels, allowing us to exit as soon as ctx.Done is selected, instead of pre-loading or queuing this function by default and waiting for the ticker to tick, running the function and then finding out ctx.Done is done.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2025

Merging this branch will increase overall coverage

Impacted Packages Coverage Δ 🤖
github.com/dell/csm-replication/controllers 91.75% (ø)
github.com/dell/csm-replication/controllers/csi-replicator 85.06% (+0.02%) 👍

Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/dell/csm-replication/controllers/csi-replicator/monitoring.go 75.64% (+0.32%) 78 (+1) 59 (+1) 19 👍

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

  • github.com/dell/csm-replication/controllers/common_test.go
  • github.com/dell/csm-replication/controllers/csi-replicator/monitoring_test.go

@lukeatdell lukeatdell merged commit 94f13c5 into main Sep 9, 2025
6 checks passed
@lukeatdell lukeatdell deleted the usr/lukeatdell/unittestracefix branch September 9, 2025 19:46
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.

4 participants