You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/10/24 06:47:05 UTC
[GitHub] [apisix-dashboard] bzp2010 opened a new pull request, #2650: feat: add etcd store auto re-initialize
bzp2010 opened a new pull request, #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650
Please answer these questions before submitting a pull request, **or your PR will get closed**.
**Why submit this pull request?**
- [x] Bugfix
- [ ] New feature provided
- [ ] Improve performance
- [ ] Backport patches
**What changes will this PR take into?**
Resynchronize when the etcd local cache does not match the data source.
**Related issues**
fix #2461 #2360 and more same problem
**Checklist:**
- [x] Did you explain what problem does this PR solve? Or what new features have been added?
- [x] Have you added corresponding test cases?
- [ ] Have you modified the corresponding document?
- [x] Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on a diff in pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#discussion_r1006380498
##########
api/cmd/root.go:
##########
@@ -81,3 +89,57 @@ func manageAPI() error {
}
return nil
}
+
+func etcdConnectionChecker() context.CancelFunc {
+ ctx, cancel := context.WithCancel(context.TODO())
+ unavailableTimes := 0
+
+ go func() {
+ etcdClient := storage.GenEtcdStorage().GetClient()
+ for {
+ select {
+ case <-time.Tick(10 * time.Second):
+ sCtx, sCancel := context.WithTimeout(context.TODO(), 5*time.Second)
+ err := etcdClient.Sync(sCtx)
Review Comment:
@nic-chen
The connection can be automatically restored, but the behavior of watch is unobservable, and sometimes watch is incapacitated, but it still does not actively quit. There is no API to actively know which watchers are currently available and what is the state of those watchers.
This is a complete black box for its applicators, the entire documentation of etcd mentions that its connections are self-healing and watch will not lose any data, but it does happen in our environment.
I think this is really necessary, and if you don't think it's appropriate, we can use a less expensive way than sync requests, which is to implement it using etcd client's internal connection state monitor, client.getActiveConnection(), which will not initiate the request, but still get the connection state.
**IMHO, for the uncertain behavior in the etcd client (it is indeed a black box), where all the grpc stream details are hidden and the documentation does not match the actual behavior, an active health checking mechanism independent of the internal mechanism is a must.**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on a diff in pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#discussion_r1007850689
##########
api/test/shell/cli_test.sh:
##########
@@ -478,7 +477,39 @@ stop_dashboard() {
# check response header with custom header
run curl -i http://127.0.0.1:9000
-[ $(echo "$output" | grep -c "X-Frame-Options: test") -eq '1' ]
+ [ $(echo "$output" | grep -c "X-Frame-Options: test") -eq '1' ]
+
+ stop_dashboard 6
+}
+
+#16
+@test "Check etcd auto re-watch" {
+ recover_conf
+ clean_logfile
+
+ # change log level
+ if [[ $KERNEL = "Darwin" ]]; then
+ sed -i "" 's/level: warn/level: info/' ${CONF_FILE}
+ else
+ sed -i 's/level: warn/level: info/' ${CONF_FILE}
+ fi
+
+ start_dashboard 15
+
+ [ "$(grep -c "etcd connection is fine" ${LOG_FILE})" -ge '1' ]
+
+ run docker stop etcd
+
+ sleep 30
+
+ [ "$(grep -c "etcd connection loss detected" ${LOG_FILE})" -ge '1' ]
+
+ run docker start etcd
+
+ sleep 20
+
+ [ "$(grep -c "etcd connection recovered" ${LOG_FILE})" -ge '1' ]
+ [ "$(grep -c "etcd store reinitializing" ${LOG_FILE})" -ge '1' ]
Review Comment:
Again, the timer will still detect the fault at this point and do it again after recovery.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#issuecomment-1295679655
@nic-chen Thanks to your support, I have added a new use case to the CLI test to ensure that data changes during a connection outage will be resynchronized after the data is reinitialized. We can merge and continue to see the impact of this fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on a diff in pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#discussion_r1006381673
##########
api/cmd/root.go:
##########
@@ -81,3 +89,57 @@ func manageAPI() error {
}
return nil
}
+
+func etcdConnectionChecker() context.CancelFunc {
+ ctx, cancel := context.WithCancel(context.TODO())
+ unavailableTimes := 0
+
+ go func() {
+ etcdClient := storage.GenEtcdStorage().GetClient()
+ for {
+ select {
+ case <-time.Tick(10 * time.Second):
+ sCtx, sCancel := context.WithTimeout(context.TODO(), 5*time.Second)
+ err := etcdClient.Sync(sCtx)
+ sCancel()
+ if err != nil {
+ unavailableTimes++
+ log.Errorf("etcd connection loss detected, times: %d", unavailableTimes)
+ continue
+ }
+
+ // After multiple failures, the connection is restored
+ if unavailableTimes >= 1 {
+ log.Warnf("etcd connection recovered, but after several connection losses, reinitializing stores, times: %d", unavailableTimes)
+ unavailableTimes = 0
+
+ // When this happens, force a full re-initialization of the store
+ store.RangeStore(func(key store.HubKey, store *store.GenericStore) bool {
+ log.Warnf("etcd store reinitializing: resource: %s", key)
+ _ = store.Init()
Review Comment:
@nic-chen fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#issuecomment-1294283014
ping @nic-chen @starsz again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on a diff in pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#discussion_r1007838534
##########
api/test/shell/cli_test.sh:
##########
@@ -478,7 +477,39 @@ stop_dashboard() {
# check response header with custom header
run curl -i http://127.0.0.1:9000
-[ $(echo "$output" | grep -c "X-Frame-Options: test") -eq '1' ]
+ [ $(echo "$output" | grep -c "X-Frame-Options: test") -eq '1' ]
+
+ stop_dashboard 6
+}
+
+#16
+@test "Check etcd auto re-watch" {
+ recover_conf
+ clean_logfile
+
+ # change log level
+ if [[ $KERNEL = "Darwin" ]]; then
+ sed -i "" 's/level: warn/level: info/' ${CONF_FILE}
+ else
+ sed -i 's/level: warn/level: info/' ${CONF_FILE}
+ fi
+
+ start_dashboard 15
+
+ [ "$(grep -c "etcd connection is fine" ${LOG_FILE})" -ge '1' ]
+
+ run docker stop etcd
+
+ sleep 30
+
+ [ "$(grep -c "etcd connection loss detected" ${LOG_FILE})" -ge '1' ]
+
+ run docker start etcd
+
+ sleep 20
+
+ [ "$(grep -c "etcd connection recovered" ${LOG_FILE})" -ge '1' ]
+ [ "$(grep -c "etcd store reinitializing" ${LOG_FILE})" -ge '1' ]
Review Comment:
If the load fails, an error will be output to the console and log.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] nic-chen commented on a diff in pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
nic-chen commented on code in PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#discussion_r1006371114
##########
api/cmd/root.go:
##########
@@ -81,3 +89,57 @@ func manageAPI() error {
}
return nil
}
+
+func etcdConnectionChecker() context.CancelFunc {
+ ctx, cancel := context.WithCancel(context.TODO())
+ unavailableTimes := 0
+
+ go func() {
+ etcdClient := storage.GenEtcdStorage().GetClient()
+ for {
+ select {
+ case <-time.Tick(10 * time.Second):
+ sCtx, sCancel := context.WithTimeout(context.TODO(), 5*time.Second)
+ err := etcdClient.Sync(sCtx)
Review Comment:
oh, I think it is unnecessary. Because the client has a retry mechanism, even if the connection is interrupted during operation, reusing the client will automatically recover, only the `watch` cannot recover automatically in some cases
##########
api/cmd/root.go:
##########
@@ -81,3 +89,57 @@ func manageAPI() error {
}
return nil
}
+
+func etcdConnectionChecker() context.CancelFunc {
+ ctx, cancel := context.WithCancel(context.TODO())
+ unavailableTimes := 0
+
+ go func() {
+ etcdClient := storage.GenEtcdStorage().GetClient()
+ for {
+ select {
+ case <-time.Tick(10 * time.Second):
+ sCtx, sCancel := context.WithTimeout(context.TODO(), 5*time.Second)
+ err := etcdClient.Sync(sCtx)
+ sCancel()
+ if err != nil {
+ unavailableTimes++
+ log.Errorf("etcd connection loss detected, times: %d", unavailableTimes)
+ continue
+ }
+
+ // After multiple failures, the connection is restored
+ if unavailableTimes >= 1 {
+ log.Warnf("etcd connection recovered, but after several connection losses, reinitializing stores, times: %d", unavailableTimes)
+ unavailableTimes = 0
+
+ // When this happens, force a full re-initialization of the store
+ store.RangeStore(func(key store.HubKey, store *store.GenericStore) bool {
+ log.Warnf("etcd store reinitializing: resource: %s", key)
+ _ = store.Init()
Review Comment:
should also log the err
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on a diff in pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#discussion_r1006380498
##########
api/cmd/root.go:
##########
@@ -81,3 +89,57 @@ func manageAPI() error {
}
return nil
}
+
+func etcdConnectionChecker() context.CancelFunc {
+ ctx, cancel := context.WithCancel(context.TODO())
+ unavailableTimes := 0
+
+ go func() {
+ etcdClient := storage.GenEtcdStorage().GetClient()
+ for {
+ select {
+ case <-time.Tick(10 * time.Second):
+ sCtx, sCancel := context.WithTimeout(context.TODO(), 5*time.Second)
+ err := etcdClient.Sync(sCtx)
Review Comment:
@nic-chen
The connection can be automatically restored, but the behavior of watch is unobservable, and sometimes watch is incapacitated, but it still does not actively quit. There is no API to actively know which watchers are currently available and what is the state of those watchers.
This is a complete black box for its applicators, the entire documentation of etcd mentions that its connections are self-healing and watch will not lose any data, but it does happen in our environment.
I think this is really necessary, and if you don't think it's appropriate, we can use a less expensive way than sync requests, which is to implement it using etcd client's internal connection state monitor, client.getActiveConnection(), which will not initiate the request, but still get the connection state.
**IMHO, for the uncertain behavior in the etcd client (it is indeed a black box), where all the grpc stream details are hidden and the documentation does not match the actual behavior, an active health checking mechanism independent of the internal mechanism is a must.**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on a diff in pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#discussion_r1007837541
##########
api/test/shell/cli_test.sh:
##########
@@ -478,7 +477,39 @@ stop_dashboard() {
# check response header with custom header
run curl -i http://127.0.0.1:9000
-[ $(echo "$output" | grep -c "X-Frame-Options: test") -eq '1' ]
+ [ $(echo "$output" | grep -c "X-Frame-Options: test") -eq '1' ]
+
+ stop_dashboard 6
+}
+
+#16
+@test "Check etcd auto re-watch" {
+ recover_conf
+ clean_logfile
+
+ # change log level
+ if [[ $KERNEL = "Darwin" ]]; then
+ sed -i "" 's/level: warn/level: info/' ${CONF_FILE}
+ else
+ sed -i 's/level: warn/level: info/' ${CONF_FILE}
+ fi
+
+ start_dashboard 15
+
+ [ "$(grep -c "etcd connection is fine" ${LOG_FILE})" -ge '1' ]
+
+ run docker stop etcd
+
+ sleep 30
+
+ [ "$(grep -c "etcd connection loss detected" ${LOG_FILE})" -ge '1' ]
+
+ run docker start etcd
+
+ sleep 20
+
+ [ "$(grep -c "etcd connection recovered" ${LOG_FILE})" -ge '1' ]
+ [ "$(grep -c "etcd store reinitializing" ${LOG_FILE})" -ge '1' ]
Review Comment:
This is not possible in principle, because each connection exception triggers a re-initialization of the entire abstract storage layer, which involves reloading the "list" in full, and creating a new watcher. As soon as the full load succeeds, the existing data in the cache will be overwritten by the new data, and what is not there will be added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on a diff in pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#discussion_r1007837541
##########
api/test/shell/cli_test.sh:
##########
@@ -478,7 +477,39 @@ stop_dashboard() {
# check response header with custom header
run curl -i http://127.0.0.1:9000
-[ $(echo "$output" | grep -c "X-Frame-Options: test") -eq '1' ]
+ [ $(echo "$output" | grep -c "X-Frame-Options: test") -eq '1' ]
+
+ stop_dashboard 6
+}
+
+#16
+@test "Check etcd auto re-watch" {
+ recover_conf
+ clean_logfile
+
+ # change log level
+ if [[ $KERNEL = "Darwin" ]]; then
+ sed -i "" 's/level: warn/level: info/' ${CONF_FILE}
+ else
+ sed -i 's/level: warn/level: info/' ${CONF_FILE}
+ fi
+
+ start_dashboard 15
+
+ [ "$(grep -c "etcd connection is fine" ${LOG_FILE})" -ge '1' ]
+
+ run docker stop etcd
+
+ sleep 30
+
+ [ "$(grep -c "etcd connection loss detected" ${LOG_FILE})" -ge '1' ]
+
+ run docker start etcd
+
+ sleep 20
+
+ [ "$(grep -c "etcd connection recovered" ${LOG_FILE})" -ge '1' ]
+ [ "$(grep -c "etcd store reinitializing" ${LOG_FILE})" -ge '1' ]
Review Comment:
This is not possible in theory, because each connection exception triggers a re-initialization of the entire abstract storage layer, which involves reloading the "list" in full, and creating a new watcher. As soon as the full load succeeds, the existing data in the cache will be overwritten by the new data, and what is not there will be added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on a diff in pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
starsz commented on code in PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#discussion_r1007878568
##########
api/cmd/root.go:
##########
@@ -81,3 +89,59 @@ func manageAPI() error {
}
return nil
}
+
+func etcdConnectionChecker() context.CancelFunc {
+ ctx, cancel := context.WithCancel(context.TODO())
+ unavailableTimes := 0
+
+ go func() {
+ etcdClient := storage.GenEtcdStorage().GetClient()
+ for {
+ select {
+ case <-time.Tick(10 * time.Second):
+ sCtx, sCancel := context.WithTimeout(context.TODO(), 5*time.Second)
Review Comment:
What I mean is that you can use ctx as the parent of sCtx rather than context.TODO().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] codecov-commenter commented on pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#issuecomment-1288918099
# [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/2650?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2650](https://codecov.io/gh/apache/apisix-dashboard/pull/2650?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (defc203) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/7cbdc482f741276ddd80ecaec41ee78598d18b55?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7cbdc48) will **decrease** coverage by `6.06%`.
> The diff coverage is `70.87%`.
```diff
@@ Coverage Diff @@
## master #2650 +/- ##
==========================================
- Coverage 76.49% 70.43% -6.07%
==========================================
Files 139 200 +61
Lines 3748 7796 +4048
Branches 872 872
==========================================
+ Hits 2867 5491 +2624
- Misses 881 2006 +1125
- Partials 0 299 +299
```
| Flag | Coverage Δ | |
|---|---|---|
| backend-e2e-test-ginkgo | `65.14% <62.13%> (?)` | |
| backend-unit-test | `50.54% <55.95%> (?)` | |
| frontend-e2e-test | `68.62% <ø> (-7.88%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/2650?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [api/internal/core/storage/etcd.go](https://codecov.io/gh/apache/apisix-dashboard/pull/2650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmFnZS9ldGNkLmdv) | `52.67% <0.00%> (ø)` | |
| [api/cmd/root.go](https://codecov.io/gh/apache/apisix-dashboard/pull/2650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXBpL2NtZC9yb290Lmdv) | `74.46% <73.33%> (ø)` | |
| [api/internal/core/store/store.go](https://codecov.io/gh/apache/apisix-dashboard/pull/2650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmUuZ28=) | `85.77% <73.80%> (ø)` | |
| [web/src/components/LabelsfDrawer/LabelsDrawer.tsx](https://codecov.io/gh/apache/apisix-dashboard/pull/2650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-d2ViL3NyYy9jb21wb25lbnRzL0xhYmVsc2ZEcmF3ZXIvTGFiZWxzRHJhd2VyLnRzeA==) | `3.92% <0.00%> (-80.40%)` | :arrow_down: |
| [...b/src/pages/Route/components/DataLoader/Import.tsx](https://codecov.io/gh/apache/apisix-dashboard/pull/2650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-d2ViL3NyYy9wYWdlcy9Sb3V0ZS9jb21wb25lbnRzL0RhdGFMb2FkZXIvSW1wb3J0LnRzeA==) | `4.91% <0.00%> (-78.69%)` | :arrow_down: |
| [...es/Route/components/DataLoader/loader/OpenAPI3.tsx](https://codecov.io/gh/apache/apisix-dashboard/pull/2650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-d2ViL3NyYy9wYWdlcy9Sb3V0ZS9jb21wb25lbnRzL0RhdGFMb2FkZXIvbG9hZGVyL09wZW5BUEkzLnRzeA==) | `33.33% <0.00%> (-66.67%)` | :arrow_down: |
| [...pages/Route/components/Step1/MatchingRulesView.tsx](https://codecov.io/gh/apache/apisix-dashboard/pull/2650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-d2ViL3NyYy9wYWdlcy9Sb3V0ZS9jb21wb25lbnRzL1N0ZXAxL01hdGNoaW5nUnVsZXNWaWV3LnRzeA==) | `25.00% <0.00%> (-54.17%)` | :arrow_down: |
| [web/src/pages/Route/transform.ts](https://codecov.io/gh/apache/apisix-dashboard/pull/2650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-d2ViL3NyYy9wYWdlcy9Sb3V0ZS90cmFuc2Zvcm0udHM=) | `52.91% <0.00%> (-27.52%)` | :arrow_down: |
| [.../src/pages/Route/components/Step1/ProxyRewrite.tsx](https://codecov.io/gh/apache/apisix-dashboard/pull/2650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-d2ViL3NyYy9wYWdlcy9Sb3V0ZS9jb21wb25lbnRzL1N0ZXAxL1Byb3h5UmV3cml0ZS50c3g=) | `70.68% <0.00%> (-24.14%)` | :arrow_down: |
| [...b/src/components/Plugin/UI/referer-restriction.tsx](https://codecov.io/gh/apache/apisix-dashboard/pull/2650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-d2ViL3NyYy9jb21wb25lbnRzL1BsdWdpbi9VSS9yZWZlcmVyLXJlc3RyaWN0aW9uLnRzeA==) | `69.69% <0.00%> (-21.22%)` | :arrow_down: |
| ... and [73 more](https://codecov.io/gh/apache/apisix-dashboard/pull/2650/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on a diff in pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
starsz commented on code in PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#discussion_r1007508802
##########
api/cmd/root.go:
##########
@@ -81,3 +89,59 @@ func manageAPI() error {
}
return nil
}
+
+func etcdConnectionChecker() context.CancelFunc {
+ ctx, cancel := context.WithCancel(context.TODO())
+ unavailableTimes := 0
+
+ go func() {
+ etcdClient := storage.GenEtcdStorage().GetClient()
+ for {
+ select {
+ case <-time.Tick(10 * time.Second):
+ sCtx, sCancel := context.WithTimeout(context.TODO(), 5*time.Second)
Review Comment:
```suggestion
sCtx, sCancel := context.WithTimeout(ctx, 5*time.Second)
```
##########
api/cmd/root.go:
##########
@@ -81,3 +89,59 @@ func manageAPI() error {
}
return nil
}
+
+func etcdConnectionChecker() context.CancelFunc {
+ ctx, cancel := context.WithCancel(context.TODO())
+ unavailableTimes := 0
+
+ go func() {
+ etcdClient := storage.GenEtcdStorage().GetClient()
+ for {
+ select {
+ case <-time.Tick(10 * time.Second):
+ sCtx, sCancel := context.WithTimeout(context.TODO(), 5*time.Second)
+ err := etcdClient.Sync(sCtx)
+ sCancel()
+ if err != nil {
+ unavailableTimes++
+ log.Errorf("etcd connection loss detected, times: %d", unavailableTimes)
+ continue
+ }
+
+ // After multiple failures, the connection is restored
+ if unavailableTimes >= 1 {
+ log.Warnf("etcd connection recovered, but after several connection losses, reinitializing stores, times: %d", unavailableTimes)
+ unavailableTimes = 0
+
+ // When this happens, force a full re-initialization of the store
+ store.RangeStore(func(key store.HubKey, store *store.GenericStore) bool {
+ log.Warnf("etcd store reinitializing: resource: %s", key)
+ if err := store.Init(); err != nil {
+ log.Errorf("etcd store reinitialize failed: resource: %s", key)
Review Comment:
```suggestion
log.Errorf("etcd store reinitialize failed: resource: %s, error: %s", key, err)
```
##########
api/test/shell/cli_test.sh:
##########
@@ -478,7 +477,39 @@ stop_dashboard() {
# check response header with custom header
run curl -i http://127.0.0.1:9000
-[ $(echo "$output" | grep -c "X-Frame-Options: test") -eq '1' ]
+ [ $(echo "$output" | grep -c "X-Frame-Options: test") -eq '1' ]
+
+ stop_dashboard 6
+}
+
+#16
+@test "Check etcd auto re-watch" {
+ recover_conf
+ clean_logfile
+
+ # change log level
+ if [[ $KERNEL = "Darwin" ]]; then
+ sed -i "" 's/level: warn/level: info/' ${CONF_FILE}
+ else
+ sed -i 's/level: warn/level: info/' ${CONF_FILE}
+ fi
+
+ start_dashboard 15
+
+ [ "$(grep -c "etcd connection is fine" ${LOG_FILE})" -ge '1' ]
+
+ run docker stop etcd
+
+ sleep 30
+
+ [ "$(grep -c "etcd connection loss detected" ${LOG_FILE})" -ge '1' ]
+
+ run docker start etcd
+
+ sleep 20
+
+ [ "$(grep -c "etcd connection recovered" ${LOG_FILE})" -ge '1' ]
+ [ "$(grep -c "etcd store reinitializing" ${LOG_FILE})" -ge '1' ]
Review Comment:
Better to add e2e test to ensure the data is not loss.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on a diff in pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#discussion_r1007833043
##########
api/cmd/root.go:
##########
@@ -81,3 +89,59 @@ func manageAPI() error {
}
return nil
}
+
+func etcdConnectionChecker() context.CancelFunc {
+ ctx, cancel := context.WithCancel(context.TODO())
+ unavailableTimes := 0
+
+ go func() {
+ etcdClient := storage.GenEtcdStorage().GetClient()
+ for {
+ select {
+ case <-time.Tick(10 * time.Second):
+ sCtx, sCancel := context.WithTimeout(context.TODO(), 5*time.Second)
Review Comment:
This means applying a separate timeout for each request instead of using that global one ctx, which is used to turn off the timer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#issuecomment-1292918916
### Updated
- Update etcd dependencies to v3.5.5
- Add passive live check for etcd watch
- Add active connection check for all etcd stream
- Add cli test for active check
- Improve some codes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#issuecomment-1292916186
Updated:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on a diff in pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#discussion_r1006380361
##########
api/cmd/root.go:
##########
@@ -81,3 +89,57 @@ func manageAPI() error {
}
return nil
}
+
+func etcdConnectionChecker() context.CancelFunc {
+ ctx, cancel := context.WithCancel(context.TODO())
+ unavailableTimes := 0
+
+ go func() {
+ etcdClient := storage.GenEtcdStorage().GetClient()
+ for {
+ select {
+ case <-time.Tick(10 * time.Second):
+ sCtx, sCancel := context.WithTimeout(context.TODO(), 5*time.Second)
+ err := etcdClient.Sync(sCtx)
Review Comment:
The connection can be automatically restored, but the behavior of watch is unobservable, and sometimes watch is incapacitated, but it still does not actively quit. There is no API to actively know which watchers are currently available and what is the state of those watchers.
This is a complete black box for its applicators, the entire documentation of etcd mentions that its connections are self-healing and watch will not lose any data, but it does happen in our environment.
I think this is really necessary, and if you don't think it's appropriate, we can use a less expensive way than sync requests, which is to implement it using etcd client's internal connection state monitor, client.getActiveConnection(), which will not initiate the request, but still get the connection state.
**IMHO, for the uncertain behavior in the etcd client (it is indeed a black box), where all the grpc stream details are hidden and the documentation does not match the actual behavior, an active health checking mechanism independent of the internal mechanism is a must.**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 commented on a diff in pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650#discussion_r1006380839
##########
api/cmd/root.go:
##########
@@ -81,3 +89,57 @@ func manageAPI() error {
}
return nil
}
+
+func etcdConnectionChecker() context.CancelFunc {
+ ctx, cancel := context.WithCancel(context.TODO())
+ unavailableTimes := 0
+
+ go func() {
+ etcdClient := storage.GenEtcdStorage().GetClient()
+ for {
+ select {
+ case <-time.Tick(10 * time.Second):
+ sCtx, sCancel := context.WithTimeout(context.TODO(), 5*time.Second)
+ err := etcdClient.Sync(sCtx)
+ sCancel()
+ if err != nil {
+ unavailableTimes++
+ log.Errorf("etcd connection loss detected, times: %d", unavailableTimes)
+ continue
+ }
+
+ // After multiple failures, the connection is restored
+ if unavailableTimes >= 1 {
+ log.Warnf("etcd connection recovered, but after several connection losses, reinitializing stores, times: %d", unavailableTimes)
+ unavailableTimes = 0
+
+ // When this happens, force a full re-initialization of the store
+ store.RangeStore(func(key store.HubKey, store *store.GenericStore) bool {
+ log.Warnf("etcd store reinitializing: resource: %s", key)
+ _ = store.Init()
Review Comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bzp2010 merged pull request #2650: feat: add etcd store auto re-initialize
Posted by GitBox <gi...@apache.org>.
bzp2010 merged PR #2650:
URL: https://github.com/apache/apisix-dashboard/pull/2650
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org