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] Backport e2e tests for livez/readyz. #17144

Merged
merged 2 commits into from
Dec 21, 2023
Merged

[3.4] Backport e2e tests for livez/readyz. #17144

merged 2 commits into from
Dec 21, 2023

Conversation

siyuanfoundation
Copy link
Contributor

@siyuanfoundation siyuanfoundation marked this pull request as draft December 18, 2023 23:02
@siyuanfoundation siyuanfoundation force-pushed the livez-bp-3.4-e2e branch 6 times, most recently from 379f192 to f4fd1ef Compare December 19, 2023 17:40
@siyuanfoundation siyuanfoundation marked this pull request as ready for review December 19, 2023 17:54
fuweid
fuweid reviewed Dec 20, 2023
View reviewed changes
Makefile Outdated Show resolved Hide resolved
serathius
serathius reviewed Dec 21, 2023
View reviewed changes
tests/e2e/etcd_process.go Outdated
@@ -105,6 +125,15 @@ func (ep *etcdServerProcess) Start() error {
if ep.proc != nil {
panic("already started")
}
if ep.cfg.proxy != nil && ep.proxy == nil {
// ep.cfg.lg.Info("starting proxy...", zap.String("name", ep.cfg.name), zap.String("from", ep.cfg.proxy.From.String()), zap.String("to", ep.cfg.proxy.To.String()))
Copy link
Member

@serathius serathius Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Any reason to leave this here? Any problems with uncommenting this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no log in 3.4 implementation of etcd process. Removed the line.

@serathius
Copy link
Member

/retest

ahrtr
ahrtr reviewed Dec 21, 2023
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.

Usually we don't proactively bump dependency in a stable release unless there is any CVE or critical/major issue. Also we should do it in a separate PR if we decided to do it.

But I won't insist on it this time if there is no any objection from other maintainers and grpc-go expert/maintainer. @dfawley

Also interesting .... I see that the grpc-go's tag v1.59.0 was created on Oct 17, 2023, but I do not see grpc-go team explicitly releases it, as I do not see the release in https://github.com/grpc/grpc-go/releases @dfawley is there any reason that you intentionally did not release grpc-go v1.59.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the change since it is not necessary for this PR.

@siyuanfoundation
[3.4] backport health check e2e tests.
c43530c 
Signed-off-by: Siyuan Zhang <sizhang@google.com>
@siyuanfoundation
[3.4] enable gofail in e2e test.
5ca4d8c 
Signed-off-by: Siyuan Zhang <sizhang@google.com>
ahrtr
ahrtr approved these changes Dec 21, 2023
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

@ahrtr ahrtr merged commit f06c6e6 into etcd-io:release-3.4 Dec 21, 2023
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>
@siyuanfoundation siyuanfoundation deleted the livez-bp-3.4-e2e branch April 3, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fuweid fuweid fuweid left review comments

@serathius serathius serathius approved these changes

@ahrtr ahrtr ahrtr approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

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