Bug 2015624: Refactor IBM Cloud skips to External #26537

cjschaef · 2021-10-21T18:21:43Z

Refactor code related to skipping tests on IBM Cloud to skip based
on External ControlPlaneTopology, or Managed offering. IBM Cloud
will be offering an OCP, unmanaged, cluster that should not skip
such tests.

openshift-ci · 2021-10-21T18:21:47Z

@cjschaef: This pull request references Bugzilla bug 2015624, which is invalid:

expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 2015624: Refactor IBM Cloud skips to External

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.

alvaroaleman · 2021-10-28T14:37:10Z

test/extended/authorization/authorization.go

@@ -250,16 +252,19 @@ func (test resourceAccessReviewTest) run() {
 				}
 			}

+			infra, err := test.ocClient.AdminConfigClient().ConfigV1().Infrastructures().Get(context.Background(), "cluster", metav1.GetOptions{})


Would it be possible to have some kind of global somewhere so we can store the controlplanetopology instead of fetching it in every test that needs to switch behavior based on its value?

I can certainly look into that

I hopefully have something sufficient to do that now

alvaroaleman · 2021-11-12T14:00:08Z

test/extended/util/framework.go

+
+// GetControlPlaneTopology retrieves the cluster infrastructure TopologyMode
+func GetControlPlaneTopology(ocClient *CLI) (*configv1.TopologyMode, error) {
+	if ControlPlaneTopology == nil {


I think this needs to be threadsafe/protected by a sync.Mutex as tests could end up calling this in parallel

Added a mutex lock to the function

the mutex already needs to be used for reading ( if ControlPlaneTopology == nil) as reading/writing to a variable in parallel is a datarace.

Will the additional locking be acceptable, if we always perform it when this function is called, compared with the additional check after receiving the lock, prior to attempting to set the value?

I considered using the builtin once, but was not confident that that would work properly if an error was raised while attempting to get the Infrastructure.Status.ControlPlaneTopology.

If we just lock in the very beginning of the function prior to reading we are good. An additional check prior to setting the value would only be needed if we used a RWLock, but I don't think we need that.

okay, I'll make that change, thanks!

ironcladlou · 2021-11-15T14:48:12Z

/approve

Refactor code related to skipping tests on IBM Cloud to skip based on External ControlPlaneTopology, or Managed offering. IBM Cloud will be offering an OCP, unmanaged, cluster that should not skip such tests.

alvaroaleman

thanks for your work!

openshift-ci · 2021-11-17T17:40:29Z

@cjschaef: 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-gcp-csi	`41d9d55`	link	false	`/test e2e-gcp-csi`
ci/prow/e2e-aws-cgroupsv2	`41d9d55`	link	false	`/test e2e-aws-cgroupsv2`
ci/prow/e2e-aws-csi	`41d9d55`	link	false	`/test e2e-aws-csi`
ci/prow/e2e-aws-single-node	`41d9d55`	link	false	`/test e2e-aws-single-node`
ci/prow/e2e-agnostic-cmd	`41d9d55`	link	false	`/test e2e-agnostic-cmd`

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.

alvaroaleman · 2021-11-17T18:00:43Z

/lgtm
/retest-required

alvaroaleman · 2021-11-17T18:01:57Z

/bugzilla refresh

openshift-ci · 2021-11-17T18:02:00Z

@alvaroaleman: This pull request references Bugzilla bug 2015624, which is invalid:

expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

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.

openshift-ci · 2021-11-17T18:02:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, cjschaef, ironcladlou

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:

~~test/extended/OWNERS~~ [ironcladlou]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alvaroaleman · 2021-11-17T18:05:25Z

/bugzilla refresh

openshift-ci · 2021-11-17T18:07:34Z

@alvaroaleman: This pull request references Bugzilla bug 2015624, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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.

openshift-bot · 2021-11-17T19:36:37Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-11-17T19:44:35Z

@cjschaef: All pull requests linked via external trackers have merged:

openshift/origin#26537

Bugzilla bug 2015624 has been moved to the MODIFIED state.

In response to this:

Bug 2015624: Refactor IBM Cloud skips to External

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.

openshift-ci bot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Oct 21, 2021

openshift-ci bot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Oct 21, 2021

openshift-ci bot requested review from adambkaplan and dgrisonnet October 21, 2021 18:22

cjschaef force-pushed the bz_2015624 branch 5 times, most recently from a87cae2 to c6114a0 Compare October 21, 2021 21:50

alvaroaleman reviewed Oct 28, 2021

View reviewed changes

alvaroaleman mentioned this pull request Oct 28, 2021

Add initial support for controlplanetopology=external #26549

Closed

cjschaef force-pushed the bz_2015624 branch 11 times, most recently from 3b1030e to c44b654 Compare November 11, 2021 16:56

alvaroaleman reviewed Nov 12, 2021

View reviewed changes

cjschaef force-pushed the bz_2015624 branch 2 times, most recently from e7167da to 5671723 Compare November 15, 2021 14:33

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2021

cjschaef force-pushed the bz_2015624 branch from 5671723 to 2dbd582 Compare November 15, 2021 16:03

Bug 2015624: Refactor IBM Cloud skips to External

41d9d55

Refactor code related to skipping tests on IBM Cloud to skip based on External ControlPlaneTopology, or Managed offering. IBM Cloud will be offering an OCP, unmanaged, cluster that should not skip such tests.

cjschaef force-pushed the bz_2015624 branch from 2dbd582 to 41d9d55 Compare November 17, 2021 16:07

alvaroaleman approved these changes Nov 17, 2021

View reviewed changes

openshift-ci bot assigned alvaroaleman Nov 17, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2021

openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Nov 17, 2021

openshift-ci bot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Nov 17, 2021

openshift-merge-robot merged commit f2d6080 into openshift:master Nov 17, 2021

cjschaef deleted the bz_2015624 branch November 17, 2021 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 2015624: Refactor IBM Cloud skips to External #26537

Bug 2015624: Refactor IBM Cloud skips to External #26537

cjschaef commented Oct 21, 2021

openshift-ci bot commented Oct 21, 2021

alvaroaleman Oct 28, 2021

cjschaef Oct 29, 2021

cjschaef Nov 10, 2021

alvaroaleman Nov 12, 2021

cjschaef Nov 12, 2021

alvaroaleman Nov 15, 2021

cjschaef Nov 17, 2021

alvaroaleman Nov 17, 2021

cjschaef Nov 17, 2021

cjschaef Nov 17, 2021

ironcladlou commented Nov 15, 2021

alvaroaleman left a comment

openshift-ci bot commented Nov 17, 2021 •

edited

Loading

alvaroaleman commented Nov 17, 2021

alvaroaleman commented Nov 17, 2021

openshift-ci bot commented Nov 17, 2021

openshift-ci bot commented Nov 17, 2021

alvaroaleman commented Nov 17, 2021

openshift-ci bot commented Nov 17, 2021

openshift-bot commented Nov 17, 2021

openshift-ci bot commented Nov 17, 2021

Bug 2015624: Refactor IBM Cloud skips to External #26537

Bug 2015624: Refactor IBM Cloud skips to External #26537

Conversation

cjschaef commented Oct 21, 2021

openshift-ci bot commented Oct 21, 2021

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

Choose a reason for hiding this comment

ironcladlou commented Nov 15, 2021

alvaroaleman left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 17, 2021 • edited Loading

alvaroaleman commented Nov 17, 2021

alvaroaleman commented Nov 17, 2021

openshift-ci bot commented Nov 17, 2021

openshift-ci bot commented Nov 17, 2021

alvaroaleman commented Nov 17, 2021

openshift-ci bot commented Nov 17, 2021

openshift-bot commented Nov 17, 2021

openshift-ci bot commented Nov 17, 2021

openshift-ci bot commented Nov 17, 2021 •

edited

Loading