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

[3.4] member replace e2e test #17173

Merged
merged 1 commit into from
Jan 12, 2024
Merged

[3.4] member replace e2e test #17173

merged 1 commit into from
Jan 12, 2024

Conversation

ZhouJianMS
Copy link
Contributor

Follow up: #17079 (comment)

@k8s-ci-robot
Copy link

Hi @ZhouJianMS. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@ZhouJianMS
Copy link
Contributor Author

Does anyone know why 3.4 branch always show that "no tests to run" in local test? I have not encountered similar issue in 3.5 and main branch.

Running tool: /usr/bin/go test -timeout 30m -tags e2e -run ^TestMemberReplace$ go.etcd.io/etcd/tests/v3/e2e -count=1

testing: warning: no tests to run
PASS
ok      go.etcd.io/etcd/tests/v3/e2e    0.014s [no tests to run]

@fuweid
Copy link
Member

fuweid commented Dec 29, 2023

@ZhouJianMS I think there is no go.etcd.io/etcd/tests/v3. You should use go.etcd.io/etcd/tests/e2e.

@ZhouJianMS ZhouJianMS force-pushed the member-replace-3.4 branch 2 times, most recently from 3da1a1f to bf9d6bf Compare January 5, 2024 08:13
@ZhouJianMS ZhouJianMS marked this pull request as ready for review January 5, 2024 08:52
fuweid
fuweid reviewed Jan 9, 2024
View reviewed changes
go.mod Outdated
golang.org/x/time v0.0.0-20180412165947-fbb02b2291d2
google.golang.org/grpc v1.58.3
google.golang.org/grpc v1.59.0
Copy link
Member

Choose a reason for hiding this comment

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

this pull request requires to update deps? If not, please move the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The deps updates were introduced by running "make" command locally. I have removed them but found that TestWatchDelayForEvent always timeout. I think my change does not have any impact on it.
Failed jobs: https://github.com/etcd-io/etcd/actions/runs/7469684403/job/20327175916?pr=17173

@ZhouJianMS ZhouJianMS force-pushed the member-replace-3.4 branch 2 times, most recently from b253b15 to c25b338 Compare January 10, 2024 02:46
@ZhouJianMS
member replace e2e test
2f8d7d0 
Signed-off-by: ZhouJianMS <zhoujian@microsoft.com>
ahrtr
ahrtr approved these changes Jan 11, 2024
View reviewed changes
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

lgtm

Thanks

@serathius
Copy link
Member

/retest

@fuweid
Copy link
Member

fuweid commented Jan 11, 2024 •
edited
Loading

I think it's about timeout. We need to increase TIMEOUT value for test-e2e-release from 30m to 60m

@serathius
Copy link
Member

I think it's about timeout. We need to increase TIMEOUT value for test-e2e-release from 30m to 60m

I would be opposed from blindly increasing the test timeout on pull request testing. I think the time of test execution is important part of user experience. For me even the 30 minutes is not great. If this is not an issue with this PR, but a problem of consistently hitting the limit, we should consider analysing the e2e test scenario and reducing the time.

@fuweid
Copy link
Member

fuweid commented Jan 12, 2024

I would be opposed from blindly increasing the test timeout on pull request testing.

Agree. But checked out all the failure cases and last 4 rounds were timeout issue.
Last run was ok go.etcd.io/etcd/tests/e2e 1796.555s. It's close to 30m.
It still impacts user experience. It's kind of flakey.
We can consider to increase the timeout value, do some investigation, fix it and turn value back.
We can't ignore the fact that we run e2e cases one by one. More cases might increase runtime.
And Github CI VM looks unstable, at least to me.

fuweid
fuweid approved these changes Jan 12, 2024
View reviewed changes
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@ahrtr ahrtr merged commit 148ba41 into etcd-io:release-3.4 Jan 12, 2024
12 checks passed
fuweid added a commit to fuweid/etcd that referenced this pull request Jan 23, 2024
@fuweid
fix: linux-amd64-e2e job timeout issue
3c4f9ff 
Assume etcd-io#16916 as baseline. The E2E takes `1395.082s`.

* etcd-io#16988

It introduced `TestAuthority` which takes `18.39s`.
And after etcd-io#16997, it takes `50.05s`.

* etcd-io#16995

It introduced `TestInPlaceRecovery` which takes `17.37s`.

* etcd-io#17144

  - New `TestHTTPHealthHandler` takes `29.9s`
  - New `TestHTTPLivezReadyzHandler` takes `35.20s`

* etcd-io#17173

  - New `TestMemberReplace` takes `7.55s`.

Ideally, it should increase `140.07s`. It's not larger than `1800s`
timeout value.

However, we run E2E cases 3 times. By default, we run E2E cases with
`-cpu 1,2,4`. That means that we run 3 times.

```bash
$ go help testflag

 -count n
            Run each test, benchmark, and fuzz seed n times (default 1).

            If -cpu is set, run n times for each GOMAXPROCS value.
            Examples are always run once. -count does not apply to
            fuzz tests matched by -fuzz.
```

I don't think we should run E2E with different GOMAXPROCS value. All the
`TestXYZ` are used to control etcd process and we don't set GOMAXPROCS
env to etcd process. So, we don't need `-cpu` setting for E2E.

Closes: etcd-io#17241

Signed-off-by: Wei Fu <fuweid89@gmail.com>
@fuweid fuweid mentioned this pull request Jan 23, 2024
Merged
fuweid added a commit to fuweid/etcd that referenced this pull request Jan 23, 2024
@fuweid
fix: linux-amd64-e2e job timeout issue
1d47674 
Assume etcd-io#16916 as baseline. The E2E takes `1395.082s`.

* etcd-io#16988

It introduced `TestAuthority` which takes `18.39s`.
And after etcd-io#16997, it takes `50.05s`.

* etcd-io#16995

It introduced `TestInPlaceRecovery` which takes `17.37s`.

* etcd-io#17144

  - New `TestHTTPHealthHandler` takes `29.9s`
  - New `TestHTTPLivezReadyzHandler` takes `35.20s`

* etcd-io#17173

  - New `TestMemberReplace` takes `7.55s`.

Ideally, it should increase `140.07s`. It's not larger than `1800s`
timeout value.

However, we run E2E cases 3 times. By default, we run E2E cases with
`-cpu 1,2,4`. That means that we run 3 times.

`1395.082s` + `140.07s * 3` = `1815.292s` > `1800s`

```bash
$ go help testflag

 -count n
            Run each test, benchmark, and fuzz seed n times (default 1).

            If -cpu is set, run n times for each GOMAXPROCS value.
            Examples are always run once. -count does not apply to
            fuzz tests matched by -fuzz.
```

I don't think we should run E2E with different GOMAXPROCS value. All the
`TestXYZ` are used to control etcd process and we don't set GOMAXPROCS
env to etcd process. So, we don't need `-cpu` setting for E2E.

Closes: etcd-io#17241

Signed-off-by: Wei Fu <fuweid89@gmail.com>
fuweid added a commit to fuweid/etcd that referenced this pull request Jan 23, 2024
@fuweid
fix: linux-amd64-e2e job timeout issue
a511d63 
Assume etcd-io#16916 as baseline. The E2E takes `1395.082s`.

* etcd-io#16988

It introduced `TestAuthority` which takes `18.39s`.
And after etcd-io#16997, it takes `50.05s`.

* etcd-io#16995

It introduced `TestInPlaceRecovery` which takes `17.37s`.

* etcd-io#17144

  - New `TestHTTPHealthHandler` takes `29.9s`
  - New `TestHTTPLivezReadyzHandler` takes `35.20s`

* etcd-io#17173

  - New `TestMemberReplace` takes `7.55s`.

Ideally, it should increase `140.07s`. It's not larger than `1800s`
timeout value.

However, we run E2E cases 3 times. By default, we run E2E cases with
`-cpu 1,2,4`. That means that we run 3 times.

`1395.082s` + `140.07s * 3` = `1815.292s` > `1800s`

```bash
$ go help testflag

 -count n
            Run each test, benchmark, and fuzz seed n times (default 1).

            If -cpu is set, run n times for each GOMAXPROCS value.
            Examples are always run once. -count does not apply to
            fuzz tests matched by -fuzz.
```

I don't think we should run E2E with different GOMAXPROCS value. All the
`TestXYZ` are used to control etcd process and we don't set GOMAXPROCS
env to etcd process.

Set `CPU=4` to align with main and release/3.5.

Closes: etcd-io#17241

Signed-off-by: Wei Fu <fuweid89@gmail.com>
fuweid added a commit to fuweid/etcd that referenced this pull request Jan 23, 2024
@fuweid
fix: linux-amd64-e2e job timeout issue
c74cc42 
Assume etcd-io#16916 as baseline. The E2E takes `1395.082s`.

* etcd-io#16988

It introduced `TestAuthority` which takes `18.39s`.
And after etcd-io#16997, it takes `50.05s`.

* etcd-io#16995

It introduced `TestInPlaceRecovery` which takes `17.37s`.

* etcd-io#17144

  - New `TestHTTPHealthHandler` takes `29.9s`
  - New `TestHTTPLivezReadyzHandler` takes `35.20s`

* etcd-io#17173

  - New `TestMemberReplace` takes `7.55s`.

Ideally, it should increase `140.07s`. It's not larger than `1800s`
timeout value.

However, we run E2E cases 3 times. By default, we run E2E cases with
`-cpu 1,2,4`. That means that we run 3 times.

`1395.082s` + `140.07s * 3` = `1815.292s` > `1800s`

```bash
$ go help testflag

 -count n
            Run each test, benchmark, and fuzz seed n times (default 1).

            If -cpu is set, run n times for each GOMAXPROCS value.
            Examples are always run once. -count does not apply to
            fuzz tests matched by -fuzz.
```

I don't think we should run E2E with different GOMAXPROCS value. All the
`TestXYZ` are used to control etcd process and we don't set GOMAXPROCS
env to etcd process.

Set `CPU=4` to align with main and release/3.5.

Closes: etcd-io#17241

Signed-off-by: Wei Fu <fuweid89@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serathius serathius serathius approved these changes

@fuweid fuweid fuweid approved these changes

@ahrtr ahrtr ahrtr approved these changes

Assignees
No one assigned
Labels
needs-ok-to-test stage/merge-when-tests-green
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ZhouJianMS @k8s-ci-robot @fuweid @serathius @ahrtr