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

feat: controller leader election #173

Merged
merged 3 commits into from
Jan 14, 2021
Merged

feat: controller leader election #173

merged 3 commits into from
Jan 14, 2021

Conversation

tokers
Copy link
Contributor

@tokers tokers commented Jan 12, 2021

Please answer these questions before submitting a pull request

  • Why submit this pull request?

  • Bugfix

  • New feature provided

  • Improve performance

  • Backport patches

  • Related issues

#85

Bugfix

  • Description

  • How to fix?

New feature or improvement

  • Describe the details and related test reports.

Backport patches

  • Why need to backport?

  • Source branch

  • Related commits and pull requests

  • Target branch

@codecov-io
Copy link

Codecov Report

Merging #173 (b9668f2) into master (1669bba) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #173      +/-   ##
==========================================
- Coverage   48.71%   48.65%   -0.07%     
==========================================
  Files          30       30              
  Lines        1480     1482       +2     
==========================================
  Hits          721      721              
- Misses        673      675       +2     
  Partials       86       86
Impacted Files Coverage Δ
cmd/ingress/ingress.go 70.76% <100.00%> (+0.45%) ⬆️
pkg/config/config.go 83.33% <100.00%> (+0.47%) ⬆️
pkg/seven/state/solver.go 1.69% <0.00%> (-1.70%) ⬇️

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 1669bba...b9668f2. Read the comment docs.

@gxthrj
Copy link
Contributor

gxthrj commented Jan 12, 2021

e2e test failed.

	Error:
  	            		            	expected status equal to:
  	            		            	 "200 OK"

  	            		            	but got:
  	            		            	 "502 Bad Gateway"
  	Test:       	namespacing filtering resources in other namespaces should be ignored


  /Users/jinwei/go_git/go/pkg/mod/github.com/gavv/httpexpect/[email protected]/reporter.go:23
... ...
Summarizing 1 Failure:

[Fail] namespacing filtering [It] resources in other namespaces should be ignored
/Users/jinwei/go_git/go/pkg/mod/github.com/gavv/httpexpect/[email protected]/reporter.go:23

Ran 7 of 7 Specs in 156.243 seconds
FAIL! -- 6 Passed | 1 Failed | 0 Pending | 0 Skipped

@tokers
Copy link
Contributor Author

tokers commented Jan 13, 2021

@gxthrj It's due to the controller didn't push the resources quickly to apisix.

membphis
membphis reviewed Jan 13, 2021
View reviewed changes
Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

it seems that we need to add some E2E test case for this PR.

maybe the https://github.com/chaos-mesh/chaos-mesh is useful for us.

@tokers
Copy link
Contributor Author

tokers commented Jan 13, 2021

it seems that we need to add some E2E test case for this PR.

maybe the https://github.com/chaos-mesh/chaos-mesh is useful for us.

Already mimics the behavior in e2e cases.

What i did is killing the leader controller, and wait for a while until another leader was elected.

membphis
membphis approved these changes Jan 13, 2021
View reviewed changes
Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

LGTM

@membphis
Copy link
Member

@gxthrj ping

gxthrj
gxthrj reviewed Jan 14, 2021
View reviewed changes
test/e2e/scaffold/httpbin.go
@@ -112,3 +115,32 @@ func (s *Scaffold) ScaleHTTPBIN(desired int) error {
}
return nil
}

// WaitAllHTTPBINPods waits until all httpbin pods ready.
func (s *Scaffold) WaitAllHTTPBINPoddsAvailable() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use k8s.WaitUntilNumPodsCreatedE(...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just checks number of Pods are created, not number of Pods are ready.

What we need is the latter, since only a Pod is ready, can it exists in the Endpoints ready addresses.

@gxthrj
Copy link
Contributor

gxthrj commented Jan 14, 2021

We'd better link related issues.

@tokers
Copy link
Contributor Author

tokers commented Jan 14, 2021

We'd better link related issues.

Already linked it. See the PR's overview.

@gxthrj
Copy link
Contributor

gxthrj commented Jan 14, 2021

We'd better link related issues.

Already linked it. See the PR's overview.

image
Should be reflected here.

This was linked to issues Jan 14, 2021
leaderelection #85
Closed
Add more use examples in apisix-ingress-controller ingress --help #111
Closed
@gxthrj gxthrj removed a link to an issue Jan 14, 2021
Add more use examples in apisix-ingress-controller ingress --help #111
Closed
@gxthrj gxthrj linked an issue Jan 14, 2021 that may be closed by this pull request
[TODO] Active / standby deployment to improve availability #11
Closed
@gxthrj gxthrj merged commit 396cae2 into apache:master Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@membphis membphis membphis approved these changes

@gxthrj gxthrj gxthrj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

leaderelection [TODO] Active / standby deployment to improve availability
4 participants
@tokers @codecov-io @gxthrj @membphis