[release-4.12] OCPBUGS-6841: Update etcd scaling test for CPMS supported platforms #27702

Elbehery · 2023-02-02T18:26:31Z

This is a cherry-pick of #27497

cc @hasbro17

openshift-ci-robot · 2023-02-02T18:26:38Z

@Elbehery: This pull request references Jira Issue OCPBUGS-6841, which is valid.

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.12.z) matches configured target version for branch (4.12.z)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)
dependent bug Jira Issue OCPBUGS-6844 is in the state Verified, which is one of the valid states (VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE))
dependent Jira Issue OCPBUGS-6844 targets the "4.13.0" version, which is one of the valid target versions: 4.13.0
bug has dependents

Requesting review from QA contact:
/cc @geliu2016

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This is a cherry-pick of #27497

cc @hasbro17

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 · 2023-02-02T18:26:39Z

@Elbehery: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

[release-4.12] OCPBUGS-6841: Update etcd scaling test for CPMS supported platforms

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.

Elbehery · 2023-02-02T18:27:35Z

/assign @tjungblu
/assign @hasbro17

Elbehery · 2023-02-02T19:48:35Z

verify job fails on make update-gofmt

i ran it locally && make verify and nothing changed, no error !!

/retest-required

geliu2016

/label cherry-pick-approved

openshift-ci · 2023-02-03T02:43:11Z

@geliu2016: Can not set label cherry-pick-approved: Must be member in one of these teams: [openshift-staff-engineers]

In response to this:

/label cherry-pick-approved

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.

tjungblu · 2023-02-03T09:35:32Z

test/extended/testdata/bindata.go

-//	    a.png
-//	    b.png
-//
+//     data/


not sure how that got added, but you should probably remove this file from the changeset. Seems like a difference in go version

i used go1.18, it is the version used in go.mod for 4.12 branch

I will remove this 👍🏽

For platforms where the ControlPlaneMachineSet is active and being reconciled by the CPMSO, the vertical scaling test should rely on the CPMSO to remove and add new machines, otherwise there is a race between the test removing a machine and the CPMSO adding a new one.

openshift-ci · 2023-02-03T16:27:41Z

@Elbehery: 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-vsphere-ovn-etcd-scaling	`4d9cf6a`	link	false	`/test e2e-vsphere-ovn-etcd-scaling`
ci/prow/e2e-aws-ovn-single-node-serial	`4d9cf6a`	link	false	`/test e2e-aws-ovn-single-node-serial`

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.

Elbehery · 2023-02-04T12:42:28Z

/retest-required

Elbehery · 2023-02-07T17:26:44Z

/retest-required

Elbehery · 2023-02-07T17:27:26Z

@geliu2016 can we label this please
@hasbro17 any job needs to be overriden ?

geliu2016

/label cherry-pick-approved

openshift-ci · 2023-02-08T06:46:07Z

@geliu2016: Can not set label cherry-pick-approved: Must be member in one of these teams: [openshift-staff-engineers]

In response to this:

/label cherry-pick-approved

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.

hasbro17 · 2023-02-08T19:28:59Z

Test is passing on vsphere, the job is failing due to unrelated monitoring issues:

: [sig-instrumentation][Late] Alerts shouldn't report any unexpected alerts in firing or pending state [apigroup:config.openshift.io] [Suite:openshift/conformance/parallel] expand_less | 32s
-- | --
{  fail [github.com/openshift/origin/test/extended/prometheus/prometheus.go:331]: unable to check firing alerts during test Unexpected error:     <*v1.Error \| 0xc005915500>: {         Type: "server_error",         Msg: "server error: 504",         Detail: "<html><body><h1>504 Gateway Time-out</h1>\nThe server didn't respond in time.\n</body></html>\n",     }     server_error: server error: 504 occurred Ginkgo exit error 1: exit with code 1}

/approve
/label backport-risk-assessed

openshift-ci · 2023-02-08T19:29:10Z

@hasbro17: Can not set label backport-risk-assessed: Must be member in one of these teams: [openshift-staff-engineers]

In response to this:

Test is passing on vsphere, the job is failing due to unrelated monitoring issues:

: [sig-instrumentation][Late] Alerts shouldn't report any unexpected alerts in firing or pending state [apigroup:config.openshift.io] [Suite:openshift/conformance/parallel] expand_less | 32s
-- | --
{  fail [github.com/openshift/origin/test/extended/prometheus/prometheus.go:331]: unable to check firing alerts during test Unexpected error:     <*v1.Error \| 0xc005915500>: {         Type: "server_error",         Msg: "server error: 504",         Detail: "<html><body><h1>504 Gateway Time-out</h1>\nThe server didn't respond in time.\n</body></html>\n",     }     server_error: server error: 504 occurred Ginkgo exit error 1: exit with code 1}

/approve
/label backport-risk-assessed

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.

Elbehery · 2023-02-14T12:39:26Z

/retest-required

Elbehery · 2023-02-14T12:40:14Z

@soltysh hello ✋🏽

Can we get this merged ? .. it is just bumping timeout :)

openshift-ci · 2023-02-14T13:46:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Elbehery, geliu2016, hasbro17, mfojtik

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/etcd/OWNERS~~ [hasbro17,mfojtik]

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

deads2k · 2023-02-14T15:51:50Z

test/extended/etcd/helpers/helpers.go

@@ -180,6 +183,91 @@ func recoverClusterToInitialStateIfNeeded(ctx context.Context, t TestingT, machi
 	})
 }

+func DeleteSingleMachine(ctx context.Context, t TestingT, machineClient machinev1beta1client.MachineInterface) (string, error) {
+	waitPollInterval := 15 * time.Second
+	waitPollTimeout := 5 * time.Minute


you probably want to check this every 30s or so. The load is minimal and it helps latency. Just fixing in master is ok with me.

thanks @deads2k

This is a backport, maybe @hasbro17 we need a new PR with these changes to 4.13 before backporting ?

deads2k · 2023-02-14T15:52:21Z

test/extended/etcd/helpers/helpers.go

+			return isTransientAPIError(t, err)
+		}
+
+		// Machine names are suffixed with an index number (e.g "ci-op-xlbdrkvl-6a467-qcbkh-master-0")


not all names are ordered by number, but ordering for consistency is still ok.

deads2k · 2023-02-14T15:57:01Z

test/extended/etcd/helpers/helpers.go

+
+	machineToDelete := ""
+	err := wait.Poll(waitPollInterval, waitPollTimeout, func() (bool, error) {
+		machineList, err := machineClient.List(ctx, metav1.ListOptions{LabelSelector: masterMachineLabelSelector})


Why does is this listing repeated instead of identifying the machine-to-be-deleted once and then retrying the Delete until you get a successful call?

That's to retry the List if it errors out due to a transient API error. Would we not want to retry in that case?

That's to retry the List if it errors out due to a transient API error. Would we not want to retry in that case?

The client retries automatically. Failures at this level aren't expected and even in controller code aren't generally handled.

Elbehery · 2023-02-14T17:14:40Z

@deads2k can we marge this to be aligned with master and 4.13, then we make the required changes in a new PR against master then backport ?

deads2k · 2023-02-14T17:59:33Z

test/extended/etcd/helpers/helpers.go

+// EnsureReadyReplicasOnCPMS checks if status.readyReplicas on the cluster CPMS is n
+// this effectively counts the number of control-plane machines with the provider state as running
+func EnsureReadyReplicasOnCPMS(ctx context.Context, t TestingT, expectedReplicaCount int, cpmsClient machinev1client.ControlPlaneMachineSetInterface) error {
+	waitPollInterval := 5 * time.Second


poll should be faster.

deads2k · 2023-02-14T17:59:50Z

test/extended/etcd/helpers/helpers.go

+			// The machine we just listed should be present but if not, error out
+			if apierrors.IsNotFound(err) {
+				t.Logf("machine %q was listed but not found or already deleted", machineToDelete)
+				return false, fmt.Errorf("machine %q was listed but not found or already deleted", machineToDelete)


this error will exist the loop as an error, rigth?

Yes, it will break the polling loop and exit the function with an error.

Yes, it will break the polling loop and exit the function with an error.

Since the item was deleted and the function is called DeleteSingleMachine, why return the error?

deads2k · 2023-02-14T18:00:53Z

test/extended/etcd/helpers/helpers.go

+			return isTransientAPIError(t, err)
+		}
+
+		if cpms.Status.ReadyReplicas != int32(expectedReplicaCount) {


do you care about this condition or do you care this AND that you have exactly the right number of total nodes AND that the desired replicas is also expectedReplicaCount?

We care about this condition because we want the test to ensure that a new machine has successfully been created and is ready, which means the associated has also been created and is running.
IIUC ReadyReplicas should cover both conditions:

https://github.com/openshift/cluster-control-plane-machine-set-operator/blob/main/pkg/controllers/controlplanemachineset/status.go#L95-L96

https://github.com/openshift/cluster-control-plane-machine-set-operator/blob/main/pkg/machineproviders/types.go#L38-L40

I think the desired status.replicas will only tell us if the machine was created, and not whether it became running with a node attached.

if expected does not match the desired (spec), the return further down looks incorrect.

also, this looks like the correct spot to double check that the number of nodes is actually correct.

geliu2016 · 2023-02-21T00:56:29Z

/label cherry-pick-approved

openshift-ci · 2023-02-21T00:56:38Z

@geliu2016: Can not set label cherry-pick-approved: Must be member in one of these teams: [openshift-staff-engineers]

In response to this:

/label cherry-pick-approved

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.

hasbro17 · 2023-02-21T07:19:24Z

@deads2k I think we can put up a new PR to address the review comments from here and make those changes in master since this is just a backport.
However I think merging this first is still useful in fixing the test where it's broken on the platforms that have CPMS enabled.

deads2k · 2023-02-27T22:41:59Z

test/extended/etcd/helpers/helpers.go

+				t.Logf("machine %q was listed but not found or already deleted", machineToDelete)
+				return false, fmt.Errorf("machine %q was listed but not found or already deleted", machineToDelete)
+			}
+			return isTransientAPIError(t, err)


if you retry this, you are no longer guaranteeing that you're deleting a single item. You can be deleting many.

damdo · 2023-04-07T15:48:22Z

Looks like etcd vertical scaling for 4.12 is broken until this backport merges (which is a blocking job for the control-plane-machine-set-operator).
Their 4.14 and 4.13 counterparts are working well for us, it would be nice to get this one in too.
Thanks!

Elbehery · 2023-04-07T17:24:19Z

Thanks @damdo for your review 👍🏽

There is another on going PR #27788

Elbehery · 2023-05-05T08:13:46Z

resolved by #27907

/close

openshift-ci-robot · 2023-05-05T08:13:49Z

@Elbehery: This pull request references Jira Issue OCPBUGS-6841. The bug has been updated to no longer refer to the pull request using the external bug tracker.

In response to this:

This is a cherry-pick of #27497

cc @hasbro17

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 requested a review from geliu2016 February 2, 2023 18:26

openshift-ci bot assigned hasbro17 and tjungblu Feb 2, 2023

openshift-ci bot requested review from csrwng and soltysh February 2, 2023 18:28

geliu2016 approved these changes Feb 3, 2023

View reviewed changes

tjungblu reviewed Feb 3, 2023

View reviewed changes

Elbehery force-pushed the backport_etcd_scaling_test_for_CPMS branch from b9dae82 to 4d9cf6a Compare February 3, 2023 13:01

Elbehery mentioned this pull request Feb 4, 2023

[release-4.12] OCPBUGS-6841: Update etcd scaling test for CPMS supported platforms #27692

Closed

geliu2016 approved these changes Feb 8, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2023

openshift-ci bot assigned mfojtik Feb 14, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2023

mfojtik added the backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. label Feb 14, 2023

openshift-ci bot assigned dgoodwin, gangwgr, jadhaj, jitendar-singh, kasturinarra, pamoedom and prietyc123 Feb 14, 2023

deads2k reviewed Feb 14, 2023

View reviewed changes

deads2k reviewed Feb 27, 2023

View reviewed changes

Elbehery mentioned this pull request Mar 9, 2023

update-etcd-scaling-test #27788

Merged

Elbehery closed this May 5, 2023

[release-4.12] OCPBUGS-6841: Update etcd scaling test for CPMS supported platforms #27702

[release-4.12] OCPBUGS-6841: Update etcd scaling test for CPMS supported platforms #27702

Conversation

Elbehery commented Feb 2, 2023

openshift-ci-robot commented Feb 2, 2023

openshift-ci bot commented Feb 2, 2023

Elbehery commented Feb 2, 2023

Elbehery commented Feb 2, 2023

geliu2016 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 3, 2023 • edited

Elbehery commented Feb 4, 2023

Elbehery commented Feb 7, 2023

Elbehery commented Feb 7, 2023

geliu2016 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 8, 2023

hasbro17 commented Feb 8, 2023

openshift-ci bot commented Feb 8, 2023

Elbehery commented Feb 14, 2023

Elbehery commented Feb 14, 2023

openshift-ci bot commented Feb 14, 2023

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

Elbehery commented Feb 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k Feb 27, 2023 • edited

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

geliu2016 commented Feb 21, 2023

openshift-ci bot commented Feb 21, 2023

hasbro17 commented Feb 21, 2023

Choose a reason for hiding this comment

damdo commented Apr 7, 2023

Elbehery commented Apr 7, 2023

Elbehery commented May 5, 2023

openshift-ci-robot commented May 5, 2023

openshift-ci bot commented Feb 3, 2023 •

edited

deads2k Feb 27, 2023 •

edited