Skipped failing test and added race condition check to CI #1217

samiib · 2024-05-15T07:22:28Z

Skipped the aci_rest_managed importWithIpv6 test from CI to stop random failures in the acceptance test.
Issue #1218 is created to fix this single test case.

The race check should be enabled in CI to detect any future issues.

codecov-commenter · 2024-05-17T03:45:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (master@0b33a73). Learn more about missing BASE report.

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #1217   +/-   ##
=========================================
  Coverage          ?   86.94%           
=========================================
  Files             ?       43           
  Lines             ?    11258           
  Branches          ?        0           
=========================================
  Hits              ?     9788           
  Misses            ?     1005           
  Partials          ?      465

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ixed

shrsr · 2024-05-17T13:58:45Z

.github/workflows/checks.yml

@@ -76,7 +76,9 @@ jobs:
          terraform_version: '1.7.*'
          terraform_wrapper: false
      - name: Terraform Acceptance Test (APIC ${{ matrix.apic_host.name }})
-        run: go test github.com/CiscoDevNet/terraform-provider-aci/v2/internal/provider -v -timeout 300m -coverprofile=coverage.out -covermode=atomic
+        # TODO: Fix TestAccAciRestManaged_importWithIpv6 to resolve data race failures occurring in the CI


This should be an interesting fix.

Authenticate() has c.skipLoggingPayload whose value is being changed and reset in the end.
Now Authenticate() is called from multiple go-routines, one being MakeRestRequestRaw() via DoRestRequestEscapeHtml and the other go-routine handling MakeRestRequestRaw() within the Authenticate() function itself. This might intermittently be creating a race condition due to c.skipLoggingPayload state being read by one and written by the other at the same time.
I think this is what's causing it.

It would be great if this PR is updated with the fix.

Thanks @shrsr, yeah this sounds like what could be happening based on the stack trace error of the data race warnings. Its odd that its only happening consistently in this one test. It appears to be quite a complex fix that will effect every other module, hence I created another issue for it. #1218
For now this PR will turn on the race condition check in the CI and skip this one test to stop PR's failing randomly.

It's only happening for this test because of the way the config. is specified in the test. As seen below, two BDs are being created in parallel. This is only showing up during the CI because more than one cpu is utilized by the go test environment in the CI workflow enabling parallelism. The utilization of more than one CPU also causes parallel refreshes in TF acceptance tests further adding to race conditions.

resource "aci_rest_managed" "tenant_%[2]s" { dn = "uni/tn-%[1]s" class_name = "fvTenant" content = { name = "%[1]s" } child { rn = "ctx-VRF1" class_name = "fvCtx" content = { name = "VRF1" } } child { rn = "ctx-VRF2" class_name = "fvCtx" content = { name = "VRF2" } } } resource "aci_rest_managed" "bd_%[2]s" { dn = "${aci_rest_managed.tenant_%[2]s.id}/BD-%[1]s" class_name = "fvBD" content = { name = "%[1]s" } child { rn = "rsctx" class_name = "fvRsCtx" content = { tnFvCtxName = "VRF1" } } } resource "aci_rest_managed" "subnet_%[2]s" { dn = "${aci_rest_managed.bd_%[2]s.id}/subnet-[%[3]s]" class_name = "fvSubnet" content = { ip = "%[3]s" scope = "private" ipDPLearning = "enabled" ctrl = "nd" } } resource "aci_rest_managed" "bd_%[2]s_2" { dn = "${aci_rest_managed.tenant_%[2]s.id}/BD-%[1]s_2" class_name = "fvBD" content = { name = "%[1]s_2" } child { rn = "rsctx" class_name = "fvRsCtx" content = { tnFvCtxName = "VRF2" } } child { rn = "subnet-[%[4]s]" class_name = "fvSubnet" content = { ip = "%[4]s" scope = "private" ipDPLearning = "enabled" ctrl = "nd" } } }

Instead of skipping the test, we could enforce the CI to use 1 cpu for now while the issue is being fixed.
What do you think?

go test github.com/CiscoDevNet/terraform-provider-aci/v2/internal/provider -v -race -cpu 1 -timeout 300m -coverprofile=coverage.out -covermode=atomic

I like it! I'll try it out now. 😸

@shrsr Unfortunately with -cpu 1 the data race is still detected. I'll revert that commit and skip the test for now. 😞

Yeah looks like there are other factors that come into play in containers and simply setting the core to 1 is not enough to mask the race.

shrsr · 2024-05-20T21:22:17Z

.github/workflows/checks.yml

@@ -76,7 +76,9 @@ jobs:
          terraform_version: '1.7.*'
          terraform_wrapper: false
      - name: Terraform Acceptance Test (APIC ${{ matrix.apic_host.name }})
-        run: go test github.com/CiscoDevNet/terraform-provider-aci/v2/internal/provider -v -timeout 300m -coverprofile=coverage.out -covermode=atomic
+        # TODO: Fix TestAccAciRestManaged_importWithIpv6 to resolve data race failures occurring in the CI


It's only happening for this test because of the way the config. is specified in the test. As seen below, two BDs are being created in parallel. This is only showing up during the CI because more than one cpu is utilized by the go test environment in the CI workflow enabling parallelism. The utilization of more than one CPU also causes parallel refreshes in TF acceptance tests further adding to race conditions.

resource "aci_rest_managed" "tenant_%[2]s" { dn = "uni/tn-%[1]s" class_name = "fvTenant" content = { name = "%[1]s" } child { rn = "ctx-VRF1" class_name = "fvCtx" content = { name = "VRF1" } } child { rn = "ctx-VRF2" class_name = "fvCtx" content = { name = "VRF2" } } } resource "aci_rest_managed" "bd_%[2]s" { dn = "${aci_rest_managed.tenant_%[2]s.id}/BD-%[1]s" class_name = "fvBD" content = { name = "%[1]s" } child { rn = "rsctx" class_name = "fvRsCtx" content = { tnFvCtxName = "VRF1" } } } resource "aci_rest_managed" "subnet_%[2]s" { dn = "${aci_rest_managed.bd_%[2]s.id}/subnet-[%[3]s]" class_name = "fvSubnet" content = { ip = "%[3]s" scope = "private" ipDPLearning = "enabled" ctrl = "nd" } } resource "aci_rest_managed" "bd_%[2]s_2" { dn = "${aci_rest_managed.tenant_%[2]s.id}/BD-%[1]s_2" class_name = "fvBD" content = { name = "%[1]s_2" } child { rn = "rsctx" class_name = "fvRsCtx" content = { tnFvCtxName = "VRF2" } } child { rn = "subnet-[%[4]s]" class_name = "fvSubnet" content = { ip = "%[4]s" scope = "private" ipDPLearning = "enabled" ctrl = "nd" } } }

shrsr · 2024-05-20T21:25:24Z

.github/workflows/checks.yml

@@ -76,7 +76,9 @@ jobs:
          terraform_version: '1.7.*'
          terraform_wrapper: false
      - name: Terraform Acceptance Test (APIC ${{ matrix.apic_host.name }})
-        run: go test github.com/CiscoDevNet/terraform-provider-aci/v2/internal/provider -v -timeout 300m -coverprofile=coverage.out -covermode=atomic
+        # TODO: Fix TestAccAciRestManaged_importWithIpv6 to resolve data race failures occurring in the CI


Instead of skipping the test, we could enforce the CI to use 1 cpu for now while the issue is being fixed.
What do you think?

go test github.com/CiscoDevNet/terraform-provider-aci/v2/internal/provider -v -race -cpu 1 -timeout 300m -coverprofile=coverage.out -covermode=atomic

shrsr

LGTM

anvitha-jain

LGTM

gmicol

LGTM

samiib · 2024-06-14T05:41:20Z

Long term fix is ready here #1236. Cancelling this PR.

samiib changed the title ~~[ignore] Added race condition check to tests~~ WIP - [ignore] Added race condition check to tests May 15, 2024

[ignore] Added race condition check to tests

300b919

samiib force-pushed the test-race-check branch from 655e47c to 300b919 Compare May 16, 2024 03:27

[ignore] Split TestAccAciRestManaged_import into seperate tests

0190fa0

samiib force-pushed the test-race-check branch from e6a138a to 0190fa0 Compare May 17, 2024 01:59

[ignore] Skip troublesome aci_rest_managed test in CI until it gets f…

f118801

…ixed

samiib force-pushed the test-race-check branch from d9430e0 to f118801 Compare May 17, 2024 04:09

samiib changed the title ~~WIP - [ignore] Added race condition check to tests~~ Skipped failing test and added race condition check to CI May 17, 2024

samiib requested review from lhercot, akinross, shrsr, anvitha-jain, sajagana and gmicol May 17, 2024 05:20

samiib self-assigned this May 17, 2024

samiib added the Test Used for all testing label May 17, 2024

shrsr requested changes May 17, 2024

View reviewed changes

shrsr requested changes May 20, 2024

View reviewed changes

shrsr previously approved these changes May 21, 2024

View reviewed changes

samiib dismissed shrsr’s stale review via f118801 May 21, 2024 21:14

samiib force-pushed the test-race-check branch from 3860bda to f118801 Compare May 21, 2024 21:14

samiib requested a review from shrsr May 21, 2024 21:15

anvitha-jain previously approved these changes May 22, 2024

View reviewed changes

samiib dismissed anvitha-jain’s stale review via 8065077 May 22, 2024 22:12

samiib force-pushed the test-race-check branch from 8065077 to f118801 Compare May 23, 2024 04:15

gmicol approved these changes May 23, 2024

View reviewed changes

samiib requested a review from anvitha-jain May 23, 2024 23:38

anvitha-jain approved these changes May 31, 2024

View reviewed changes

samiib closed this Jun 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skipped failing test and added race condition check to CI #1217

Skipped failing test and added race condition check to CI #1217

samiib commented May 15, 2024 •

edited

Loading

codecov-commenter commented May 17, 2024 •

edited

Loading

shrsr May 17, 2024

samiib May 19, 2024

shrsr May 20, 2024

shrsr May 20, 2024

samiib May 20, 2024

samiib May 21, 2024

shrsr May 22, 2024

shrsr May 20, 2024

shrsr May 20, 2024

shrsr left a comment

anvitha-jain left a comment

gmicol left a comment

samiib commented Jun 14, 2024

Skipped failing test and added race condition check to CI #1217

Skipped failing test and added race condition check to CI #1217

Conversation

samiib commented May 15, 2024 • edited Loading

codecov-commenter commented May 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrsr left a comment

Choose a reason for hiding this comment

anvitha-jain left a comment

Choose a reason for hiding this comment

gmicol left a comment

Choose a reason for hiding this comment

samiib commented Jun 14, 2024

samiib commented May 15, 2024 •

edited

Loading

codecov-commenter commented May 17, 2024 •

edited

Loading