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.

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

tests: Add SIGKILL functional test #13924

Closed
wants to merge 1 commit into from
Closed

Conversation

serathius
Copy link
Member

Splitting #13838 into mergable code
cc @ptabor @ahrtr

@codecov-commenter
Copy link

Codecov Report

Merging #13924 (9e4313a) into main (b018742) will increase coverage by 0.07%.
The diff coverage is n/a.

❗ Current head 9e4313a differs from pull request most recent head be34ec9. Consider uploading reports for the commit be34ec9 to get more accurate results

@@            Coverage Diff             @@
##             main   #13924      +/-   ##
==========================================
+ Coverage   72.41%   72.48%   +0.07%     
==========================================
  Files         469      469              
  Lines       38411    38411              
==========================================
+ Hits        27814    27841      +27     
+ Misses       8798     8783      -15     
+ Partials     1799     1787      -12     
Flag Coverage Δ
all 72.48% <ø> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
server/etcdserver/api/rafthttp/peer_status.go 87.87% <0.00%> (-12.13%) ⬇️
server/etcdserver/api/rafthttp/peer.go 87.01% <0.00%> (-8.45%) ⬇️
server/etcdserver/api/rafthttp/pipeline.go 93.50% <0.00%> (-3.90%) ⬇️
server/etcdserver/api/v3election/election.go 66.66% <0.00%> (-2.78%) ⬇️
client/v3/maintenance.go 54.71% <0.00%> (-1.26%) ⬇️
server/proxy/grpcproxy/watch.go 95.37% <0.00%> (-1.16%) ⬇️
server/etcdserver/api/v3rpc/interceptor.go 76.56% <0.00%> (-1.05%) ⬇️
client/v3/client.go 80.06% <0.00%> (-0.95%) ⬇️
server/etcdserver/apply.go 89.05% <0.00%> (+0.27%) ⬆️
pkg/proxy/server.go 61.01% <0.00%> (+0.33%) ⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b018742...be34ec9. Read the comment docs.

@serathius
Copy link
Member Author

@ahrtr @ptabor PTAL

@@ -185,6 +185,8 @@ tester-config:
- SIGTERM_ALL
- SIGQUIT_AND_REMOVE_ONE_FOLLOWER
- SIGQUIT_AND_REMOVE_ONE_FOLLOWER_UNTIL_TRIGGER_SNAPSHOT
- SIGKILL_FOLLOWER
Copy link
Member

Choose a reason for hiding this comment

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

SIGKILL_ONE_FOLLOWER so as to keep the naming consistent as the existing SIGTERM_ONE_FOLLOWER and SIGQUIT_AND_REMOVE_ONE_FOLLOWER ?

@@ -426,6 +429,8 @@ enum Case {
// each member must be able to process client requests.
SIGQUIT_AND_REMOVE_QUORUM_AND_RESTORE_LEADER_SNAPSHOT_FROM_SCRATCH = 14;

SIGKILL_FOLLOWER = 15;
Copy link
Member

Choose a reason for hiding this comment

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

SIGKILL_ONE_FOLLOWER? The same as above.

@ahrtr
Copy link
Member

ahrtr commented Apr 13, 2022

Basically looks good to me with one minor comment.

Another comment is there are some duplicate code, such as handler.go#L555-L565. We may get them wrapped into a common function or method. But of course we can address it in a separate PR. I can also do it once this one is merged.

@@ -174,6 +174,30 @@ func recover_SIGQUIT_ETCD_AND_REMOVE_DATA(clus *Cluster, idx1 int) error {
return err
}

func inject_SIGKILL(clus *Cluster, index int) error {
clus.lg.Info(
"disastrous machine failure START",
Copy link
Contributor

Choose a reason for hiding this comment

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

(SIGKILL)

)
err := clus.sendOp(index, rpcpb.Operation_SIGKILL_ETCD)
clus.lg.Info(
"disastrous machine failure END",
Copy link
Contributor

Choose a reason for hiding this comment

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

SIGKILL

Copy link
Contributor

@ptabor ptabor left a comment

Choose a reason for hiding this comment

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

nits, but marking as requiring changing for attention.

@stale
Copy link

stale bot commented Aug 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 12, 2022
@ahrtr ahrtr added stage/tracked and removed stale labels Aug 12, 2022
@serathius serathius closed this Jan 16, 2023
@serathius serathius deleted the sigkill branch June 15, 2023 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants