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.

Sign up for GitHub

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

Jump to bottom

[OCPCLOUD-1706] Migrate test utils to actuator-pkg repository #159

Merged
merged 1 commit into from
Feb 10, 2023
Merged

[OCPCLOUD-1706] Migrate test utils to actuator-pkg repository #159

merged 1 commit into from
Feb 10, 2023

Conversation

RadekManak
Copy link
Contributor

@RadekManak RadekManak commented Jan 10, 2023 •
edited
Loading

This PR moves the test and resourcebuilder package to openshift/cluster-api-actuator-pkg#265

The test package is renamed to testutils.
The machineInfo resourcebuilder is left in this repository and is imported as localresourcebuilder.

RadekManak
RadekManak commented Jan 10, 2023
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 10, 2023
@RadekManak RadekManak changed the title Migrate test utils to actuator-pkg repository [OCPCLOUD-1706] Migrate test utils to actuator-pkg repository Jan 23, 2023
@RadekManak RadekManak force-pushed the test-utils branch 2 times, most recently from a781bd6 to 295aeca Compare February 1, 2023 13:13
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2023
@JoelSpeed
Copy link
Contributor

In the test

With a running controller with updated machines and the instance size is changed should perform a rolling update
/go/src/github.com/openshift/cluster-control-plane-machine-set-operator/pkg/controllers/controlplanemachineset/controller_test.go:1051

At step (Some steps run in parallel, this may be a red herring)

STEP: Checking the number of control plane machines never goes above 4 replicas @ 02/01/23 13:18:13.765

We are seeing the following race

==================
WARNING: DATA RACE
Read at 0x00c0001fe048 by goroutine 15994:
  github.com/onsi/ginkgo/v2/reporters.(*DefaultReporter).emitBlock()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go:631 +0x56
  github.com/onsi/ginkgo/v2/reporters.(*DefaultReporter).emitSpecEvent()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go:553 +0x504
  github.com/onsi/ginkgo/v2/reporters.(*DefaultReporter).EmitSpecEvent()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go:540 +0x1e4
  github.com/onsi/ginkgo/v2/internal.(*Suite).handleSpecEvent()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/suite.go:264 +0x2f4
  github.com/onsi/ginkgo/v2/internal.(*Suite).By()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/suite.go:285 +0x197
  github.com/onsi/ginkgo/v2.By()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/core_dsl.go:524 +0x69
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers.CheckRolloutForIndex()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers/rollingupdate.go:37 +0x92
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers.checkRolloutProgress()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers/rollingupdate.go:85 +0x5d
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers.ItShouldPerformARollingUpdate.func1.3()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers/cases.go:95 +0x5d
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework.Async.func1()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework/utils.go:85 +0xc4
Previous write at 0x00c0001fe048 by goroutine 15992:
  github.com/onsi/ginkgo/v2/reporters.(*DefaultReporter).emit()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go:623 +0x56
  github.com/onsi/ginkgo/v2/reporters.(*DefaultReporter).emitBlock()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go:636 +0xf7
  github.com/onsi/ginkgo/v2/reporters.(*DefaultReporter).emitSpecEvent()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go:553 +0x504
  github.com/onsi/ginkgo/v2/reporters.(*DefaultReporter).EmitSpecEvent()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go:540 +0x1e4
  github.com/onsi/ginkgo/v2/internal.(*Suite).handleSpecEvent()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/suite.go:264 +0x2f4
  github.com/onsi/ginkgo/v2/internal.(*Suite).By()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/suite.go:285 +0x197
  github.com/onsi/ginkgo/v2.By()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/core_dsl.go:524 +0x69
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers.CheckReplicasDoesNotExceedSurgeCapacity()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers/rollingupdate.go:72 +0x64
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers.ItShouldPerformARollingUpdate.func1.1()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers/cases.go:87 +0x3e
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework.Async.func1()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework/utils.go:85 +0xc4
Goroutine 15994 (running) created at:
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework.Async()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework/utils.go:81 +0x12e
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers.ItShouldPerformARollingUpdate.func1()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers/cases.go:94 +0x784
  github.com/onsi/ginkgo/v2/internal.extractBodyFunction.func3()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/node.go:459 +0x30
  github.com/onsi/ginkgo/v2/internal.(*Suite).runNode.func3()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/suite.go:854 +0x114
Goroutine 15992 (running) created at:
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework.Async()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework/utils.go:81 +0x12e
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers.ItShouldPerformARollingUpdate.func1()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers/cases.go:86 +0x5c4
  github.com/onsi/ginkgo/v2/internal.extractBodyFunction.func3()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/node.go:459 +0x30
  github.com/onsi/ginkgo/v2/internal.(*Suite).runNode.func3()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/suite.go:854 +0x114
==================

And then again in step:

STEP: Waiting for the index 0 to be replaced @ 02/01/23 13:18:13.765

We are seeing this race

==================
WARNING: DATA RACE
Write at 0x00c0001fe058 by goroutine 15993:
  github.com/onsi/ginkgo/v2/reporters.(*DefaultReporter).emit()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go:624 +0x9a
  github.com/onsi/ginkgo/v2/reporters.(*DefaultReporter).emitBlock()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go:634 +0xaf
  github.com/onsi/ginkgo/v2/reporters.(*DefaultReporter).emitSpecEvent()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go:553 +0x504
  github.com/onsi/ginkgo/v2/reporters.(*DefaultReporter).EmitSpecEvent()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go:540 +0x1e4
  github.com/onsi/ginkgo/v2/internal.(*Suite).handleSpecEvent()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/suite.go:264 +0x2f4
  STEP: Checking that other indexes (not 0) do not have 2 replicas @ 02/01/23 13:18:13.766  github.com/onsi/ginkgo/v2/internal.(*Suite).By()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/suite.go:285 +0x197
  github.com/onsi/ginkgo/v2.By()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/core_dsl.go:524 +0x69
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers.WaitForControlPlaneMachineSetDesiredReplicas()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers/controlplanemachineset.go:178 +0x178
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers.ItShouldPerformARollingUpdate.func1.2()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers/cases.go:91 +0x84
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework.Async.func1()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework/utils.go:85 +0xc4
Previous write at 0x00c0001fe058 by goroutine 15992:
  github.com/onsi/ginkgo/v2/reporters.(*DefaultReporter).emit()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go:624 +0x9a
  github.com/onsi/ginkgo/v2/reporters.(*DefaultReporter).emitBlock()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go:636 +0xf7
  github.com/onsi/ginkgo/v2/reporters.(*DefaultReporter).emitSpecEvent()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go:553 +0x504
  github.com/onsi/ginkgo/v2/reporters.(*DefaultReporter).EmitSpecEvent()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go:540 +0x1e4
  github.com/onsi/ginkgo/v2/internal.(*Suite).handleSpecEvent()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/suite.go:264 +0x2f4
  github.com/onsi/ginkgo/v2/internal.(*Suite).By()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/suite.go:285 +0x197
  github.com/onsi/ginkgo/v2.By()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/core_dsl.go:524 +0x69
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers.CheckReplicasDoesNotExceedSurgeCapacity()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers/rollingupdate.go:72 +0x64
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers.ItShouldPerformARollingUpdate.func1.1()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers/cases.go:87 +0x3e
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework.Async.func1()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework/utils.go:85 +0xc4
Goroutine 15993 (running) created at:
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework.Async()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework/utils.go:81 +0x12e
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers.ItShouldPerformARollingUpdate.func1()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers/cases.go:90 +0x6a4
  github.com/onsi/ginkgo/v2/internal.extractBodyFunction.func3()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/node.go:459 +0x30
  github.com/onsi/ginkgo/v2/internal.(*Suite).runNode.func3()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/suite.go:854 +0x114
Goroutine 15992 (running) created at:
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework.Async()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework/utils.go:81 +0x12e
  github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers.ItShouldPerformARollingUpdate.func1()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers/cases.go:86 +0x5c4
  github.com/onsi/ginkgo/v2/internal.extractBodyFunction.func3()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/node.go:459 +0x30
  github.com/onsi/ginkgo/v2/internal.(*Suite).runNode.func3()
      /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/vendor/github.com/onsi/ginkgo/v2/internal/suite.go:854 +0x114
==================

I can see notes in there about this being a race within framework.Async so it could be an issue with how we are testing the various pieces of code in the waitgroup, will need to review the changes in ginkgo between 2.4 and 2.7 to identify if there's something obvious changed

@JoelSpeed
Copy link
Contributor

By running just the single racing test locally (modify hack/test.sh):

#31 -  ${GINKGO} ${GINKGO_ARGS} ${GINKGO_EXTRA_ARGS} ${TEST_PACKAGES}
#31 +  ${GINKGO} ${GINKGO_ARGS} ${GINKGO_EXTRA_ARGS} ./pkg/controllers/controlplanemachineset

And then focusing the test's parent context (pkg/controllers/controlplanemachineset/controller_test.go):

#1036 - Context("and the instance size is changed", func() {
#1036 + FContext("and the instance size is changed", func() {

And then using go modules replace statements to force the ginkgo version, I can see that the race isn't present in v2.4.0 (current version on main) but is present on v2.5.0.

So the issue should be introduced within this diff

@RadekManak RadekManak mentioned this pull request Feb 8, 2023
Closed
@JoelSpeed JoelSpeed mentioned this pull request Feb 8, 2023
Closed
@damdo
Copy link
Member

damdo commented Feb 8, 2023

@RadekManak #164 merged so if you want to rebase on top of main you should be good to go 👍

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2023
JoelSpeed
JoelSpeed reviewed Feb 9, 2023
View reviewed changes
Makefile Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@JoelSpeed
Copy link
Contributor

/hold cancel
/lgtm
/approve
/retest-required

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 9, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2023
damdo
damdo reviewed Feb 9, 2023
View reviewed changes
Copy link
Member

@damdo damdo left a comment

Choose a reason for hiding this comment

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

Thanks!
/lgtm

@damdo
Copy link
Member

damdo commented Feb 9, 2023

/retest

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 2a90938 and 2 for PR HEAD d6d70e5 in total

@JoelSpeed
Copy link
Contributor

/retest-required

@damdo
Copy link
Member

damdo commented Feb 10, 2023

/retest

@openshift-merge-robot openshift-merge-robot merged commit 90b0cef into openshift:main Feb 10, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 10, 2023

@RadekManak: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-upgrade e88d98e link true /test e2e-aws-upgrade
ci/prow/e2e-azure-operator d6d70e5 link false /test e2e-azure-operator

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damdo damdo damdo left review comments

@JoelSpeed JoelSpeed JoelSpeed left review comments

@lobziik lobziik Awaiting requested review from lobziik

@odvarkadaniel odvarkadaniel Awaiting requested review from odvarkadaniel

Assignees

@damdo damdo

@JoelSpeed JoelSpeed

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RadekManak @JoelSpeed @damdo @openshift-ci-robot @openshift-merge-robot