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 2021/05/15 08:16:48 UTC
[GitHub] [apisix-ingress-controller] slene opened a new pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
slene opened a new pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453
Please answer these questions before submitting a pull request
- Why submit this pull request?
- [x] Bugfix
- [ ] New feature provided
- [ ] Improve performance
- [ ] Backport patches
- Related issues
#448
#449
___
### 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
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633171064
##########
File path: pkg/ingress/apisix_cluster_config.go
##########
@@ -62,7 +64,6 @@ func (c *apisixClusterConfigController) run(ctx context.Context) {
go c.runWorker(ctx)
}
<-ctx.Done()
- c.workqueue.ShutDown()
Review comment:
Did you mean the case that cache sync failure?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633025408
##########
File path: pkg/apisix/cluster.go
##########
@@ -290,6 +303,39 @@ func (c *cluster) GlobalRule() GlobalRule {
return c.globalRules
}
+// HealthCheck implements Cluster.HealthCheck method.
+func (c *cluster) HealthCheck(ctx context.Context, backoff wait.Backoff) (err error) {
+ if c.cacheSyncErr != nil {
+ err = c.cacheSyncErr
+ return
+ }
+ if atomic.LoadInt32(&c.cacheState) == _cacheSyncing {
+ return
+ }
+ var lastCheckErr error
+ err = wait.ExponentialBackoffWithContext(ctx, backoff, func() (done bool, _ error) {
+ if lastCheckErr = c.healthCheck(ctx); lastCheckErr != nil {
+ log.Warnf("failed to HealthCheck for cluster %s: %s, will retry", c.name, lastCheckErr)
+ return
+ }
+ done = true
+ return
+ })
+ if err != nil {
+ // if ErrWaitTimeout then set lastSyncErr
+ c.cacheSyncErr = lastCheckErr
+ }
+ return err
+}
+
+func (c *cluster) healthCheck(ctx context.Context) (err error) {
+ // TODO
Review comment:
Just use TCP socket probe is OK.
##########
File path: pkg/ingress/controller.go
##########
@@ -423,3 +449,26 @@ func (c *Controller) syncSSL(ctx context.Context, ssl *apisixv1.Ssl, event types
}
return err
}
+
+func (c *Controller) checkClusterHealthy(ctx context.Context) {
+ for {
+ select {
+ case <-ctx.Done():
+ case <-time.After(5 * time.Second):
+ }
+
+ // Retry three times in a row, and exit if all of them fail.
+ backoff := wait.Backoff{
+ Duration: 5 * time.Second,
+ Factor: 1,
+ Steps: 3,
+ }
+ err := c.apisix.Cluster(c.cfg.APISIX.DefaultClusterName).HealthCheck(ctx, backoff)
+ if err != nil {
+ // Finally failed health check, then give up leader.
+ c.leaderContextCancelFunc()
+ log.Warnf("failed to HealthCheck for default cluster: %s, give up leader", err)
Review comment:
```suggestion
log.Warnf("failed to check health for default cluster: %s, give up leader", err)
```
##########
File path: pkg/kube/init.go
##########
@@ -25,16 +25,12 @@ import (
// KubeClient contains some objects used to communicate with Kubernetes API Server.
type KubeClient struct {
+ cfg *config.Config
+
// Client is the object used to operate Kubernetes builtin resources.
Client kubernetes.Interface
// APISIXClient is the object used to operate resources under apisix.apache.org group.
APISIXClient clientset.Interface
- // SharedIndexInformerFactory is the index informer factory object used to watch and
Review comment:
Why change this?
##########
File path: pkg/config/config.go
##########
@@ -85,6 +85,8 @@ type APISIXConfig struct {
// DefaultClusterAdminKey is the admin key for the default cluster.
// TODO: Obsolete the plain way to specify admin_key, which is insecure.
DefaultClusterAdminKey string `json:"default_cluster_admin_key" yaml:"default_cluster_admin_key"`
+ // DefaultClusterClientTimeout is the request timeout for default cluster client.
+ DefaultClusterClientTimeout types.TimeDuration `json:"default_cluster_client_timeout" yaml:"default_cluster_client_timeout"`
Review comment:
We will introduce `ApisixClusterConfig` to set these options, so don't add it here.
##########
File path: pkg/apisix/cluster.go
##########
@@ -50,6 +50,19 @@ var (
ErrDuplicatedCluster = errors.New("duplicated cluster")
_errReadOnClosedResBody = errors.New("http: read on closed response body")
+
+ // Default shared transport if apisix client
+ defaultTransport = &http.Transport{
Review comment:
```suggestion
_defaultTransport = &http.Transport{
```
##########
File path: pkg/apisix/apisix.go
##########
@@ -19,6 +19,7 @@ import (
"sync"
v1 "github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
+ "k8s.io/apimachinery/pkg/util/wait"
Review comment:
By convention, we put the 3-party package at the middle of `import`.
```go
import (
std
......
"k8s.io/apimachinery/pkg/util/wait"
v1 "github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
)
```
##########
File path: pkg/ingress/apisix_cluster_config.go
##########
@@ -62,7 +64,6 @@ func (c *apisixClusterConfigController) run(ctx context.Context) {
go c.runWorker(ctx)
}
<-ctx.Done()
- c.workqueue.ShutDown()
Review comment:
It's not related to PR change. If you wanna change them to use `defer`, just in another PR.
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +326,42 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller is start leading ...",
Review comment:
```suggestion
log.Infow("controller tries to leading ...",
```
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +326,42 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller is start leading ...",
zap.String("namespace", c.namespace),
zap.String("pod", c.name),
)
+
defer c.leaderContextCancelFunc()
c.metricsCollector.ResetLeader(true)
- err := c.apisix.AddCluster(&apisix.ClusterOptions{
+ clusterOpts := &apisix.ClusterOptions{
Name: c.cfg.APISIX.DefaultClusterName,
AdminKey: c.cfg.APISIX.DefaultClusterAdminKey,
BaseURL: c.cfg.APISIX.DefaultClusterBaseURL,
- })
+ Timeout: c.cfg.APISIX.DefaultClusterClientTimeout.Duration,
+ }
+ err := c.apisix.AddCluster(clusterOpts)
if err != nil && err != apisix.ErrDuplicatedCluster {
- // TODO give up the leader role.
Review comment:
Don't remove the TODO, unless you really achieve this.
##########
File path: pkg/apisix/nonexistentclient.go
##########
@@ -20,6 +20,7 @@ import (
"github.com/apache/apisix-ingress-controller/pkg/apisix/cache"
v1 "github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
+ "k8s.io/apimachinery/pkg/util/wait"
Review comment:
Ditto with the `import` convention.
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +326,42 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller is start leading ...",
zap.String("namespace", c.namespace),
zap.String("pod", c.name),
)
+
defer c.leaderContextCancelFunc()
c.metricsCollector.ResetLeader(true)
- err := c.apisix.AddCluster(&apisix.ClusterOptions{
+ clusterOpts := &apisix.ClusterOptions{
Name: c.cfg.APISIX.DefaultClusterName,
AdminKey: c.cfg.APISIX.DefaultClusterAdminKey,
BaseURL: c.cfg.APISIX.DefaultClusterBaseURL,
- })
+ Timeout: c.cfg.APISIX.DefaultClusterClientTimeout.Duration,
+ }
+ err := c.apisix.AddCluster(clusterOpts)
if err != nil && err != apisix.ErrDuplicatedCluster {
- // TODO give up the leader role.
log.Errorf("failed to add default cluster: %s", err)
return
}
if err := c.apisix.Cluster(c.cfg.APISIX.DefaultClusterName).HasSynced(ctx); err != nil {
- // TODO give up the leader role.
Review comment:
Ditto.
##########
File path: pkg/ingress/controller.go
##########
@@ -180,40 +158,71 @@ func NewController(cfg *config.Config) (*Controller, error) {
watchingNamespace: watchingNamespace,
secretSSLMap: new(sync.Map),
recorder: eventBroadcaster.NewRecorder(scheme.Scheme, v1.EventSource{Component: _component}),
-
- epInformer: kubeClient.SharedIndexInformerFactory.Core().V1().Endpoints().Informer(),
- epLister: kubeClient.SharedIndexInformerFactory.Core().V1().Endpoints().Lister(),
- svcInformer: kubeClient.SharedIndexInformerFactory.Core().V1().Services().Informer(),
- svcLister: kubeClient.SharedIndexInformerFactory.Core().V1().Services().Lister(),
- ingressLister: ingressLister,
- ingressInformer: ingressInformer,
- secretInformer: kubeClient.SharedIndexInformerFactory.Core().V1().Secrets().Informer(),
- secretLister: kubeClient.SharedIndexInformerFactory.Core().V1().Secrets().Lister(),
- apisixRouteInformer: apisixRouteInformer,
- apisixRouteLister: apisixRouteLister,
- apisixUpstreamInformer: kubeClient.APISIXSharedIndexInformerFactory.Apisix().V1().ApisixUpstreams().Informer(),
- apisixUpstreamLister: kubeClient.APISIXSharedIndexInformerFactory.Apisix().V1().ApisixUpstreams().Lister(),
- apisixTlsInformer: kubeClient.APISIXSharedIndexInformerFactory.Apisix().V1().ApisixTlses().Informer(),
- apisixTlsLister: kubeClient.APISIXSharedIndexInformerFactory.Apisix().V1().ApisixTlses().Lister(),
- apisixClusterConfigInformer: kubeClient.APISIXSharedIndexInformerFactory.Apisix().V2alpha1().ApisixClusterConfigs().Informer(),
- apisixClusterConfigLister: kubeClient.APISIXSharedIndexInformerFactory.Apisix().V2alpha1().ApisixClusterConfigs().Lister(),
}
+ return c, nil
+}
+
+func (c *Controller) initWhenStartLeader() {
Review comment:
```suggestion
func (c *Controller) initWhenStartLeading() {
```
##########
File path: pkg/kube/apisix/apis/config/v2alpha1/types.go
##########
@@ -17,6 +17,7 @@ package v2alpha1
import (
"encoding/json"
+ "github.com/apache/apisix-ingress-controller/pkg/types"
Review comment:
Put our own packages at the bottom of `import`.
##########
File path: pkg/ingress/controller.go
##########
@@ -423,3 +449,26 @@ func (c *Controller) syncSSL(ctx context.Context, ssl *apisixv1.Ssl, event types
}
return err
}
+
+func (c *Controller) checkClusterHealthy(ctx context.Context) {
Review comment:
```suggestion
func (c *Controller) checkClusterHealth(ctx context.Context) {
```
##########
File path: pkg/apisix/apisix.go
##########
@@ -50,6 +51,8 @@ type Cluster interface {
String() string
// HasSynced checks whether all resources in APISIX cluster is synced to cache.
HasSynced(context.Context) error
+ // HealthCheck checks apisix cluster health in realtime.
+ HealthCheck(context.Context, wait.Backoff) error
Review comment:
IMHO We shall not accept a `wait.Backoff` object from the caller. What we try to do is hide some details and just tell the caller whether the cluster is healthy or not.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#issuecomment-848584906
https://github.com/apache/apisix-ingress-controller/pull/499 See this @slene , Corrupted this branch by accident 😂, but don't worry you are still the contributor.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r638406500
##########
File path: pkg/apisix/cluster.go
##########
@@ -63,6 +77,7 @@ type ClusterOptions struct {
type cluster struct {
name string
baseURL string
+ baseURLHost string
Review comment:
OK, agree with you :), we can change them in subsequent PRs.
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +325,47 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller tries to leading ...",
zap.String("namespace", c.namespace),
zap.String("pod", c.name),
)
+
+ var cancelFunc context.CancelFunc
Review comment:
I didn't say we should call `leaderContextCancelFun` in the health check. Just call it before the `c.run` return. Or the leader role cannot be given up.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r634325674
##########
File path: pkg/apisix/cluster.go
##########
@@ -290,6 +310,51 @@ func (c *cluster) GlobalRule() GlobalRule {
return c.globalRules
}
+// HealthCheck implements Cluster.HealthCheck method.
+func (c *cluster) HealthCheck(ctx context.Context) (err error) {
+ if c.cacheSyncErr != nil {
Review comment:
```
func (c *cluster) HealthCheck(ctx context.Context) (err error) {
if c.cacheSyncErr != nil {
err = c.cacheSyncErr
return
}
// _cacheSyncing means the first time cache sync
if atomic.LoadInt32(&c.cacheState) == _cacheSyncing {
return
}
...
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] gxthrj commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633098741
##########
File path: pkg/apisix/cluster.go
##########
@@ -132,13 +144,20 @@ func (c *cluster) syncCache() {
}()
backoff := wait.Backoff{
- Duration: time.Second,
- Factor: 2,
- Steps: 6,
- }
- err := wait.ExponentialBackoff(backoff, c.syncCacheOnce)
+ Duration: 2 * time.Second,
+ Factor: 1,
+ Steps: 5,
+ }
+ var lastSyncErr error
+ err := wait.ExponentialBackoff(backoff, func() (done bool, _ error) {
+ // not possible return: false, nil
Review comment:
```suggestion
// impossibly return: false, nil
```
##########
File path: pkg/ingress/controller.go
##########
@@ -423,3 +449,26 @@ func (c *Controller) syncSSL(ctx context.Context, ssl *apisixv1.Ssl, event types
}
return err
}
+
+func (c *Controller) checkClusterHealthy(ctx context.Context) {
+ for {
+ select {
+ case <-ctx.Done():
+ case <-time.After(5 * time.Second):
+ }
+
+ // Retry three times in a row, and exit if all of them fail.
+ backoff := wait.Backoff{
Review comment:
Put logic into the `case <-time.After(5 * time.Second)`
##########
File path: pkg/ingress/controller.go
##########
@@ -423,3 +455,21 @@ func (c *Controller) syncSSL(ctx context.Context, ssl *apisixv1.Ssl, event types
}
return err
}
+
+func (c *Controller) checkClusterHealth(ctx context.Context, cancelFunc context.CancelFunc) {
+ for {
+ select {
+ case <-ctx.Done():
+ case <-time.After(5 * time.Second):
+ }
+
+ err := c.apisix.Cluster(c.cfg.APISIX.DefaultClusterName).HealthCheck(ctx)
+ if err != nil {
+ // Finally failed health check, then give up leader.
+ log.Warnf("failed to check health for default cluster: %s, give up leader", err)
+ cancelFunc()
Review comment:
```suggestion
defer cancelFunc()
```
##########
File path: pkg/ingress/controller.go
##########
@@ -423,3 +455,21 @@ func (c *Controller) syncSSL(ctx context.Context, ssl *apisixv1.Ssl, event types
}
return err
}
+
+func (c *Controller) checkClusterHealth(ctx context.Context, cancelFunc context.CancelFunc) {
+ for {
+ select {
+ case <-ctx.Done():
+ case <-time.After(5 * time.Second):
+ }
+
+ err := c.apisix.Cluster(c.cfg.APISIX.DefaultClusterName).HealthCheck(ctx)
+ if err != nil {
+ // Finally failed health check, then give up leader.
+ log.Warnf("failed to check health for default cluster: %s, give up leader", err)
+ cancelFunc()
+ return
Review comment:
Should we set the state to `_cacheSyncing` before return?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers edited a comment on pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
tokers edited a comment on pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#issuecomment-849251017
> @tokers Bad news. You performed an incorrect git operation after force push. So my collaboration record is lost. 😞😔😣
It's my bad. But don't worry your collaboration is still counted, and now you are the contributor.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r634322130
##########
File path: pkg/apisix/cluster.go
##########
@@ -290,6 +310,51 @@ func (c *cluster) GlobalRule() GlobalRule {
return c.globalRules
}
+// HealthCheck implements Cluster.HealthCheck method.
+func (c *cluster) HealthCheck(ctx context.Context) (err error) {
+ if c.cacheSyncErr != nil {
Review comment:
You mean health check should wait first time cache sync finished ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633090335
##########
File path: pkg/kube/init.go
##########
@@ -25,16 +25,12 @@ import (
// KubeClient contains some objects used to communicate with Kubernetes API Server.
type KubeClient struct {
+ cfg *config.Config
+
// Client is the object used to operate Kubernetes builtin resources.
Client kubernetes.Interface
// APISIXClient is the object used to operate resources under apisix.apache.org group.
APISIXClient clientset.Interface
- // SharedIndexInformerFactory is the index informer factory object used to watch and
Review comment:
```
// in controller
// re-execute this code will make panic `close of closed channel` when context cancelled
// unless you re-create Informer
**Informer.Run(ctx.Done())
```
So should re-create **Informer in every controller.run. It need re-create InformerFactory.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633088542
##########
File path: pkg/apisix/cluster.go
##########
@@ -290,6 +303,39 @@ func (c *cluster) GlobalRule() GlobalRule {
return c.globalRules
}
+// HealthCheck implements Cluster.HealthCheck method.
+func (c *cluster) HealthCheck(ctx context.Context, backoff wait.Backoff) (err error) {
+ if c.cacheSyncErr != nil {
+ err = c.cacheSyncErr
+ return
+ }
+ if atomic.LoadInt32(&c.cacheState) == _cacheSyncing {
+ return
+ }
+ var lastCheckErr error
+ err = wait.ExponentialBackoffWithContext(ctx, backoff, func() (done bool, _ error) {
+ if lastCheckErr = c.healthCheck(ctx); lastCheckErr != nil {
+ log.Warnf("failed to HealthCheck for cluster %s: %s, will retry", c.name, lastCheckErr)
+ return
+ }
+ done = true
+ return
+ })
+ if err != nil {
+ // if ErrWaitTimeout then set lastSyncErr
+ c.cacheSyncErr = lastCheckErr
+ }
+ return err
+}
+
+func (c *cluster) healthCheck(ctx context.Context) (err error) {
+ // TODO
Review comment:
ok, later.
##########
File path: pkg/ingress/apisix_cluster_config.go
##########
@@ -62,7 +64,6 @@ func (c *apisixClusterConfigController) run(ctx context.Context) {
go c.runWorker(ctx)
}
<-ctx.Done()
- c.workqueue.ShutDown()
Review comment:
If not change them. Wiil cause goroutine leak in this pr. When re-create *controller.
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +326,42 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller is start leading ...",
zap.String("namespace", c.namespace),
zap.String("pod", c.name),
)
+
defer c.leaderContextCancelFunc()
c.metricsCollector.ResetLeader(true)
- err := c.apisix.AddCluster(&apisix.ClusterOptions{
+ clusterOpts := &apisix.ClusterOptions{
Name: c.cfg.APISIX.DefaultClusterName,
AdminKey: c.cfg.APISIX.DefaultClusterAdminKey,
BaseURL: c.cfg.APISIX.DefaultClusterBaseURL,
- })
+ Timeout: c.cfg.APISIX.DefaultClusterClientTimeout.Duration,
+ }
+ err := c.apisix.AddCluster(clusterOpts)
if err != nil && err != apisix.ErrDuplicatedCluster {
- // TODO give up the leader role.
Review comment:
Do heath check, give up leader if failed. Not enough ?
##########
File path: pkg/kube/init.go
##########
@@ -25,16 +25,12 @@ import (
// KubeClient contains some objects used to communicate with Kubernetes API Server.
type KubeClient struct {
+ cfg *config.Config
+
// Client is the object used to operate Kubernetes builtin resources.
Client kubernetes.Interface
// APISIXClient is the object used to operate resources under apisix.apache.org group.
APISIXClient clientset.Interface
- // SharedIndexInformerFactory is the index informer factory object used to watch and
Review comment:
```
// in controller
// re-execute this code will make panic `close of closed channel` when context cancelled
// unless you re-create Informer
**Informer.Run(ctx.Done())
```
So should re-create **Informer in every controller.run. It need re-create InformerFactory.
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +326,42 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller is start leading ...",
zap.String("namespace", c.namespace),
zap.String("pod", c.name),
)
+
defer c.leaderContextCancelFunc()
c.metricsCollector.ResetLeader(true)
- err := c.apisix.AddCluster(&apisix.ClusterOptions{
+ clusterOpts := &apisix.ClusterOptions{
Name: c.cfg.APISIX.DefaultClusterName,
AdminKey: c.cfg.APISIX.DefaultClusterAdminKey,
BaseURL: c.cfg.APISIX.DefaultClusterBaseURL,
- })
+ Timeout: c.cfg.APISIX.DefaultClusterClientTimeout.Duration,
+ }
+ err := c.apisix.AddCluster(clusterOpts)
if err != nil && err != apisix.ErrDuplicatedCluster {
- // TODO give up the leader role.
Review comment:
Do heath check, give up leader if failed. Not enough ?
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +326,42 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller is start leading ...",
zap.String("namespace", c.namespace),
zap.String("pod", c.name),
)
+
defer c.leaderContextCancelFunc()
c.metricsCollector.ResetLeader(true)
- err := c.apisix.AddCluster(&apisix.ClusterOptions{
+ clusterOpts := &apisix.ClusterOptions{
Name: c.cfg.APISIX.DefaultClusterName,
AdminKey: c.cfg.APISIX.DefaultClusterAdminKey,
BaseURL: c.cfg.APISIX.DefaultClusterBaseURL,
- })
+ Timeout: c.cfg.APISIX.DefaultClusterClientTimeout.Duration,
+ }
+ err := c.apisix.AddCluster(clusterOpts)
if err != nil && err != apisix.ErrDuplicatedCluster {
- // TODO give up the leader role.
Review comment:
ok
##########
File path: pkg/ingress/controller.go
##########
@@ -423,3 +455,21 @@ func (c *Controller) syncSSL(ctx context.Context, ssl *apisixv1.Ssl, event types
}
return err
}
+
+func (c *Controller) checkClusterHealth(ctx context.Context, cancelFunc context.CancelFunc) {
+ for {
+ select {
+ case <-ctx.Done():
+ case <-time.After(5 * time.Second):
+ }
+
+ err := c.apisix.Cluster(c.cfg.APISIX.DefaultClusterName).HealthCheck(ctx)
+ if err != nil {
+ // Finally failed health check, then give up leader.
+ log.Warnf("failed to check health for default cluster: %s, give up leader", err)
+ cancelFunc()
+ return
Review comment:
No, the current logic is to just discard it if it is unhealthy.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633089040
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +326,42 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller is start leading ...",
zap.String("namespace", c.namespace),
zap.String("pod", c.name),
)
+
defer c.leaderContextCancelFunc()
c.metricsCollector.ResetLeader(true)
- err := c.apisix.AddCluster(&apisix.ClusterOptions{
+ clusterOpts := &apisix.ClusterOptions{
Name: c.cfg.APISIX.DefaultClusterName,
AdminKey: c.cfg.APISIX.DefaultClusterAdminKey,
BaseURL: c.cfg.APISIX.DefaultClusterBaseURL,
- })
+ Timeout: c.cfg.APISIX.DefaultClusterClientTimeout.Duration,
+ }
+ err := c.apisix.AddCluster(clusterOpts)
if err != nil && err != apisix.ErrDuplicatedCluster {
- // TODO give up the leader role.
Review comment:
Do heath check, give up leader if failed. Not enough ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] gxthrj commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633099630
##########
File path: pkg/ingress/controller.go
##########
@@ -423,3 +455,21 @@ func (c *Controller) syncSSL(ctx context.Context, ssl *apisixv1.Ssl, event types
}
return err
}
+
+func (c *Controller) checkClusterHealth(ctx context.Context, cancelFunc context.CancelFunc) {
+ for {
+ select {
+ case <-ctx.Done():
+ case <-time.After(5 * time.Second):
+ }
+
+ err := c.apisix.Cluster(c.cfg.APISIX.DefaultClusterName).HealthCheck(ctx)
+ if err != nil {
+ // Finally failed health check, then give up leader.
+ log.Warnf("failed to check health for default cluster: %s, give up leader", err)
+ cancelFunc()
Review comment:
```suggestion
defer cancelFunc()
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r634310141
##########
File path: pkg/apisix/cluster.go
##########
@@ -63,6 +77,7 @@ type ClusterOptions struct {
type cluster struct {
name string
baseURL string
+ baseURLHost string
Review comment:
Split baseURL to pathPrefix and baseURLHost is not enough. Also need scheme. So how to change this config ?
##########
File path: pkg/ingress/controller.go
##########
@@ -423,3 +455,21 @@ func (c *Controller) syncSSL(ctx context.Context, ssl *apisixv1.Ssl, event types
}
return err
}
+
+func (c *Controller) checkClusterHealth(ctx context.Context, cancelFunc context.CancelFunc) {
+ defer cancelFunc()
+ for {
+ select {
+ case <-ctx.Done():
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633325314
##########
File path: pkg/kube/init.go
##########
@@ -25,16 +25,12 @@ import (
// KubeClient contains some objects used to communicate with Kubernetes API Server.
type KubeClient struct {
+ cfg *config.Config
+
// Client is the object used to operate Kubernetes builtin resources.
Client kubernetes.Interface
// APISIXClient is the object used to operate resources under apisix.apache.org group.
APISIXClient clientset.Interface
- // SharedIndexInformerFactory is the index informer factory object used to watch and
Review comment:
OK, got it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r634153781
##########
File path: pkg/apisix/cluster.go
##########
@@ -290,6 +310,51 @@ func (c *cluster) GlobalRule() GlobalRule {
return c.globalRules
}
+// HealthCheck implements Cluster.HealthCheck method.
+func (c *cluster) HealthCheck(ctx context.Context) (err error) {
+ if c.cacheSyncErr != nil {
+ err = c.cacheSyncErr
+ return
+ }
+ if atomic.LoadInt32(&c.cacheState) == _cacheSyncing {
+ return
+ }
+
+ // Retry three times in a row, and exit if all of them fail.
+ backoff := wait.Backoff{
+ Duration: 5 * time.Second,
+ Factor: 1,
+ Steps: 3,
+ }
+ var lastCheckErr error
+ err = wait.ExponentialBackoffWithContext(ctx, backoff, func() (done bool, _ error) {
+ if lastCheckErr = c.healthCheck(ctx); lastCheckErr != nil {
+ log.Warnf("failed to check health for cluster %s: %s, will retry", c.name, lastCheckErr)
+ return
+ }
+ done = true
+ return
+ })
+ if err != nil {
+ // if ErrWaitTimeout then set lastSyncErr
+ c.cacheSyncErr = lastCheckErr
+ }
+ return err
+}
+
+func (c *cluster) healthCheck(ctx context.Context) (err error) {
+ // tcp socket probe
+ d := net.Dialer{Timeout: 3 * time.Second}
+ conn, err := d.DialContext(ctx, "tcp", c.baseURLHost)
+ if err != nil {
+ return err
+ }
+ if er := conn.Close(); er != nil {
+ log.Warnf("failed close tcp socket: %s", er)
Review comment:
```suggestion
log.Warnw("failed to close tcp probe connection",
zap.Error(err),
zap.String("cluster", c.name),
)
```
##########
File path: pkg/apisix/cluster.go
##########
@@ -63,6 +77,7 @@ type ClusterOptions struct {
type cluster struct {
name string
baseURL string
+ baseURLHost string
Review comment:
If host is required, we can change the `baseURL` to `pathPrefix`, currently, `baseURLHost` is partial of `baseURL`.
##########
File path: pkg/apisix/cluster.go
##########
@@ -290,6 +310,51 @@ func (c *cluster) GlobalRule() GlobalRule {
return c.globalRules
}
+// HealthCheck implements Cluster.HealthCheck method.
+func (c *cluster) HealthCheck(ctx context.Context) (err error) {
+ if c.cacheSyncErr != nil {
Review comment:
Actually, I think health check should be independent with the first time cache sync.
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +325,47 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller tries to leading ...",
zap.String("namespace", c.namespace),
zap.String("pod", c.name),
)
+
+ var cancelFunc context.CancelFunc
Review comment:
It seems that you don't merge from the latest master, here we already have some methods to finalize the context passed to `run`.
Here we cannot cancel the context of `ctx` in parameters, so we cannot give up the leader role by ourselves.
##########
File path: pkg/apisix/cluster.go
##########
@@ -63,6 +77,7 @@ type ClusterOptions struct {
type cluster struct {
name string
baseURL string
+ baseURLHost string
Review comment:
If host is required, we can change the `baseURL` to `pathPrefix`, currently, `baseURLHost` is partial of `baseURL`.
##########
File path: pkg/apisix/cluster.go
##########
@@ -50,6 +51,19 @@ var (
ErrDuplicatedCluster = errors.New("duplicated cluster")
_errReadOnClosedResBody = errors.New("http: read on closed response body")
+
+ // Default shared transport if apisix client
Review comment:
```suggestion
// Default shared transport for apisix client
```
We may export options to customize this transport in the future (in command line options).
##########
File path: pkg/ingress/controller.go
##########
@@ -423,3 +455,21 @@ func (c *Controller) syncSSL(ctx context.Context, ssl *apisixv1.Ssl, event types
}
return err
}
+
+func (c *Controller) checkClusterHealth(ctx context.Context, cancelFunc context.CancelFunc) {
+ defer cancelFunc()
+ for {
+ select {
+ case <-ctx.Done():
Review comment:
Just return in this arm.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r634320904
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +325,47 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller tries to leading ...",
zap.String("namespace", c.namespace),
zap.String("pod", c.name),
)
+
+ var cancelFunc context.CancelFunc
Review comment:
```
...
election:
curCtx, cancel := context.WithCancel(rootCtx)
c.leaderContextCancelFunc = cancel
elector.Run(curCtx)
select {
case <-rootCtx.Done():
return nil
default:
goto election
}
...
```
```
func (c *Controller) run(ctx context.Context) {
...
defer c.leaderContextCancelFunc()
...
<-ctx.Done()
c.wg.Wait()
}
```
Not, you mean use one c.leaderContextCancelFunc to control the run context ? It is not enough.
I think the code should wait for all goroutine to exit in c.run before executing the next election.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633089040
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +326,42 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller is start leading ...",
zap.String("namespace", c.namespace),
zap.String("pod", c.name),
)
+
defer c.leaderContextCancelFunc()
c.metricsCollector.ResetLeader(true)
- err := c.apisix.AddCluster(&apisix.ClusterOptions{
+ clusterOpts := &apisix.ClusterOptions{
Name: c.cfg.APISIX.DefaultClusterName,
AdminKey: c.cfg.APISIX.DefaultClusterAdminKey,
BaseURL: c.cfg.APISIX.DefaultClusterBaseURL,
- })
+ Timeout: c.cfg.APISIX.DefaultClusterClientTimeout.Duration,
+ }
+ err := c.apisix.AddCluster(clusterOpts)
if err != nil && err != apisix.ErrDuplicatedCluster {
- // TODO give up the leader role.
Review comment:
Do heath check, give up leader if failed. Not enough ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633088542
##########
File path: pkg/apisix/cluster.go
##########
@@ -290,6 +303,39 @@ func (c *cluster) GlobalRule() GlobalRule {
return c.globalRules
}
+// HealthCheck implements Cluster.HealthCheck method.
+func (c *cluster) HealthCheck(ctx context.Context, backoff wait.Backoff) (err error) {
+ if c.cacheSyncErr != nil {
+ err = c.cacheSyncErr
+ return
+ }
+ if atomic.LoadInt32(&c.cacheState) == _cacheSyncing {
+ return
+ }
+ var lastCheckErr error
+ err = wait.ExponentialBackoffWithContext(ctx, backoff, func() (done bool, _ error) {
+ if lastCheckErr = c.healthCheck(ctx); lastCheckErr != nil {
+ log.Warnf("failed to HealthCheck for cluster %s: %s, will retry", c.name, lastCheckErr)
+ return
+ }
+ done = true
+ return
+ })
+ if err != nil {
+ // if ErrWaitTimeout then set lastSyncErr
+ c.cacheSyncErr = lastCheckErr
+ }
+ return err
+}
+
+func (c *cluster) healthCheck(ctx context.Context) (err error) {
+ // TODO
Review comment:
ok, later.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] gxthrj commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633099888
##########
File path: pkg/ingress/controller.go
##########
@@ -423,3 +455,21 @@ func (c *Controller) syncSSL(ctx context.Context, ssl *apisixv1.Ssl, event types
}
return err
}
+
+func (c *Controller) checkClusterHealth(ctx context.Context, cancelFunc context.CancelFunc) {
+ for {
+ select {
+ case <-ctx.Done():
+ case <-time.After(5 * time.Second):
+ }
+
+ err := c.apisix.Cluster(c.cfg.APISIX.DefaultClusterName).HealthCheck(ctx)
+ if err != nil {
+ // Finally failed health check, then give up leader.
+ log.Warnf("failed to check health for default cluster: %s, give up leader", err)
+ cancelFunc()
+ return
Review comment:
Should we set the state to `_cacheSyncing` before return?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633091469
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +326,42 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller is start leading ...",
zap.String("namespace", c.namespace),
zap.String("pod", c.name),
)
+
defer c.leaderContextCancelFunc()
c.metricsCollector.ResetLeader(true)
- err := c.apisix.AddCluster(&apisix.ClusterOptions{
+ clusterOpts := &apisix.ClusterOptions{
Name: c.cfg.APISIX.DefaultClusterName,
AdminKey: c.cfg.APISIX.DefaultClusterAdminKey,
BaseURL: c.cfg.APISIX.DefaultClusterBaseURL,
- })
+ Timeout: c.cfg.APISIX.DefaultClusterClientTimeout.Duration,
+ }
+ err := c.apisix.AddCluster(clusterOpts)
if err != nil && err != apisix.ErrDuplicatedCluster {
- // TODO give up the leader role.
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r636612657
##########
File path: pkg/apisix/cluster.go
##########
@@ -290,6 +310,51 @@ func (c *cluster) GlobalRule() GlobalRule {
return c.globalRules
}
+// HealthCheck implements Cluster.HealthCheck method.
+func (c *cluster) HealthCheck(ctx context.Context) (err error) {
+ if c.cacheSyncErr != nil {
Review comment:
> You mean health check should wait first time cache sync finished ?
The code above already implement it.
- The cluster sync have two state `syncing` `synced` only.
- When state is `synced` and has an error of health check. Just give up leader and re-create cluster to syncing.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r634951844
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +325,47 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller tries to leading ...",
zap.String("namespace", c.namespace),
zap.String("pod", c.name),
)
+
+ var cancelFunc context.CancelFunc
Review comment:
The reason why I introduced the `leaderContextCancelFunc` is, package `leaderelection` doesn't quit the lease renew loop even if `c.run` exits.
> Not, you mean use one c.leaderContextCancelFunc to control the run context ? It is not enough.
I think the code should wait for all goroutine to exit in c.run before executing the next election.
Yep, waiting is required before it exits. This is logical inside `c.run`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633227392
##########
File path: pkg/ingress/controller.go
##########
@@ -423,3 +455,21 @@ func (c *Controller) syncSSL(ctx context.Context, ssl *apisixv1.Ssl, event types
}
return err
}
+
+func (c *Controller) checkClusterHealth(ctx context.Context, cancelFunc context.CancelFunc) {
+ for {
+ select {
+ case <-ctx.Done():
+ case <-time.After(5 * time.Second):
+ }
+
+ err := c.apisix.Cluster(c.cfg.APISIX.DefaultClusterName).HealthCheck(ctx)
+ if err != nil {
+ // Finally failed health check, then give up leader.
+ log.Warnf("failed to check health for default cluster: %s, give up leader", err)
+ cancelFunc()
+ return
Review comment:
No, the current logic is to just discard it if it is unhealthy.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers closed pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
tokers closed pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] gxthrj commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r634905420
##########
File path: pkg/ingress/controller.go
##########
@@ -423,3 +449,26 @@ func (c *Controller) syncSSL(ctx context.Context, ssl *apisixv1.Ssl, event types
}
return err
}
+
+func (c *Controller) checkClusterHealthy(ctx context.Context) {
+ for {
+ select {
+ case <-ctx.Done():
+ case <-time.After(5 * time.Second):
+ }
+
+ // Retry three times in a row, and exit if all of them fail.
+ backoff := wait.Backoff{
Review comment:
Why mark this as resolved? in `case <-ctx.Done()` do not need the logic of health check.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r636611520
##########
File path: pkg/apisix/cluster.go
##########
@@ -63,6 +77,7 @@ type ClusterOptions struct {
type cluster struct {
name string
baseURL string
+ baseURLHost string
Review comment:
Replace baseURL with *url.URL object may be make more changes. Can we do that in this pr ? Or just use baseURLHost.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#issuecomment-848561231
I have merged your branch with the latest master branch, now LGTM to me.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] codecov-commenter commented on pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#issuecomment-842160355
# [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?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 [#453](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f820ab5) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/5d479ae148d2acdb51082bb0f129548fdfa146b4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5d479ae) will **increase** coverage by `62.95%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #453 +/- ##
============================================
+ Coverage 37.04% 100.00% +62.95%
============================================
Files 47 1 -46
Lines 3841 1 -3840
============================================
- Hits 1423 1 -1422
+ Misses 2233 0 -2233
+ Partials 185 0 -185
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/types/errors.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL3R5cGVzL2Vycm9ycy5nbw==) | | |
| [pkg/kube/translation/context.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2t1YmUvdHJhbnNsYXRpb24vY29udGV4dC5nbw==) | | |
| [pkg/apisix/ssl.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2FwaXNpeC9zc2wuZ28=) | | |
| [pkg/kube/translation/apisix\_ssl.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2t1YmUvdHJhbnNsYXRpb24vYXBpc2l4X3NzbC5nbw==) | | |
| [pkg/kube/translation/annotations.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2t1YmUvdHJhbnNsYXRpb24vYW5ub3RhdGlvbnMuZ28=) | | |
| [pkg/apisix/cache/memdb.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2FwaXNpeC9jYWNoZS9tZW1kYi5nbw==) | | |
| [pkg/api/router/router.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2FwaS9yb3V0ZXIvcm91dGVyLmdv) | | |
| [pkg/kube/translation/apisix\_upstream.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2t1YmUvdHJhbnNsYXRpb24vYXBpc2l4X3Vwc3RyZWFtLmdv) | | |
| [pkg/log/default\_logger.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2xvZy9kZWZhdWx0X2xvZ2dlci5nbw==) | | |
| [pkg/apisix/route.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2FwaXNpeC9yb3V0ZS5nbw==) | | |
| ... and [25 more](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5d479ae...f820ab5](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r636616387
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +325,47 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller tries to leading ...",
zap.String("namespace", c.namespace),
zap.String("pod", c.name),
)
+
+ var cancelFunc context.CancelFunc
Review comment:
The cancelFunc manage context of c.run, leaderContextCancelFun just manage the election context.
If call leaderContextCancelFun in health check the election will run into next loop and not wait last c.run exited.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#issuecomment-845633421
@tokers no
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#issuecomment-849251017
> @tokers Bad news. You performed an incorrect git operation after force push. So my collaboration record is lost. 😞😔😣
It's may bad. But don't worry your collaboration is still counted, and now you are the contributor.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] codecov-commenter edited a comment on pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#issuecomment-842160355
# [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?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 [#453](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (96b2315) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/5d479ae148d2acdb51082bb0f129548fdfa146b4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5d479ae) will **decrease** coverage by `0.53%`.
> The diff coverage is `14.06%`.
[](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #453 +/- ##
==========================================
- Coverage 37.04% 36.51% -0.54%
==========================================
Files 47 47
Lines 3841 3897 +56
==========================================
Hits 1423 1423
- Misses 2233 2290 +57
+ Partials 185 184 -1
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/apisix/nonexistentclient.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2FwaXNpeC9ub25leGlzdGVudGNsaWVudC5nbw==) | `54.09% <0.00%> (-0.91%)` | :arrow_down: |
| [pkg/ingress/apisix\_cluster\_config.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvYXBpc2l4X2NsdXN0ZXJfY29uZmlnLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/apisix\_route.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvYXBpc2l4X3JvdXRlLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/apisix\_tls.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvYXBpc2l4X3Rscy5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/apisix\_upstream.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvYXBpc2l4X3Vwc3RyZWFtLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/controller.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvY29udHJvbGxlci5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/endpoint.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvZW5kcG9pbnQuZ28=) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/ingress.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvaW5ncmVzcy5nbw==) | `8.58% <0.00%> (ø)` | |
| [pkg/ingress/secret.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3Mvc2VjcmV0Lmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/apisix/cluster.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2FwaXNpeC9jbHVzdGVyLmdv) | `24.33% <33.33%> (-2.71%)` | :arrow_down: |
| ... and [1 more](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5d479ae...96b2315](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#issuecomment-848905195
@tokers Bad news. You performed an incorrect git operation after force push. So my collaboration record is lost. 😞😔😣
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] slene commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
slene commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633088880
##########
File path: pkg/ingress/apisix_cluster_config.go
##########
@@ -62,7 +64,6 @@ func (c *apisixClusterConfigController) run(ctx context.Context) {
go c.runWorker(ctx)
}
<-ctx.Done()
- c.workqueue.ShutDown()
Review comment:
If not change them. Wiil cause goroutine leak in this pr. When re-create *controller.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r634853784
##########
File path: pkg/apisix/cluster.go
##########
@@ -63,6 +77,7 @@ type ClusterOptions struct {
type cluster struct {
name string
baseURL string
+ baseURLHost string
Review comment:
Or you can just save the `url.URL` object.
##########
File path: pkg/apisix/cluster.go
##########
@@ -290,6 +310,51 @@ func (c *cluster) GlobalRule() GlobalRule {
return c.globalRules
}
+// HealthCheck implements Cluster.HealthCheck method.
+func (c *cluster) HealthCheck(ctx context.Context) (err error) {
+ if c.cacheSyncErr != nil {
Review comment:
Yep.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] codecov-commenter edited a comment on pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#issuecomment-842160355
# [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?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 [#453](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9d95a9d) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/5d479ae148d2acdb51082bb0f129548fdfa146b4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5d479ae) will **decrease** coverage by `0.49%`.
> The diff coverage is `14.51%`.
[](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #453 +/- ##
==========================================
- Coverage 37.04% 36.55% -0.50%
==========================================
Files 47 47
Lines 3841 3893 +52
==========================================
Hits 1423 1423
- Misses 2233 2286 +53
+ Partials 185 184 -1
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/apisix/nonexistentclient.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2FwaXNpeC9ub25leGlzdGVudGNsaWVudC5nbw==) | `54.09% <0.00%> (-0.91%)` | :arrow_down: |
| [pkg/ingress/apisix\_cluster\_config.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvYXBpc2l4X2NsdXN0ZXJfY29uZmlnLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/apisix\_route.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvYXBpc2l4X3JvdXRlLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/apisix\_tls.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvYXBpc2l4X3Rscy5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/apisix\_upstream.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvYXBpc2l4X3Vwc3RyZWFtLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/controller.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvY29udHJvbGxlci5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/endpoint.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvZW5kcG9pbnQuZ28=) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/ingress.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvaW5ncmVzcy5nbw==) | `8.58% <0.00%> (ø)` | |
| [pkg/ingress/secret.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3Mvc2VjcmV0Lmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/apisix/cluster.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2FwaXNpeC9jbHVzdGVyLmdv) | `24.61% <35.41%> (-2.43%)` | :arrow_down: |
| ... and [1 more](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5d479ae...9d95a9d](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] codecov-commenter commented on pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#issuecomment-842160355
# [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?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 [#453](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f820ab5) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/5d479ae148d2acdb51082bb0f129548fdfa146b4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5d479ae) will **increase** coverage by `62.95%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #453 +/- ##
============================================
+ Coverage 37.04% 100.00% +62.95%
============================================
Files 47 1 -46
Lines 3841 1 -3840
============================================
- Hits 1423 1 -1422
+ Misses 2233 0 -2233
+ Partials 185 0 -185
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/types/errors.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL3R5cGVzL2Vycm9ycy5nbw==) | | |
| [pkg/kube/translation/context.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2t1YmUvdHJhbnNsYXRpb24vY29udGV4dC5nbw==) | | |
| [pkg/apisix/ssl.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2FwaXNpeC9zc2wuZ28=) | | |
| [pkg/kube/translation/apisix\_ssl.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2t1YmUvdHJhbnNsYXRpb24vYXBpc2l4X3NzbC5nbw==) | | |
| [pkg/kube/translation/annotations.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2t1YmUvdHJhbnNsYXRpb24vYW5ub3RhdGlvbnMuZ28=) | | |
| [pkg/apisix/cache/memdb.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2FwaXNpeC9jYWNoZS9tZW1kYi5nbw==) | | |
| [pkg/api/router/router.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2FwaS9yb3V0ZXIvcm91dGVyLmdv) | | |
| [pkg/kube/translation/apisix\_upstream.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2t1YmUvdHJhbnNsYXRpb24vYXBpc2l4X3Vwc3RyZWFtLmdv) | | |
| [pkg/log/default\_logger.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2xvZy9kZWZhdWx0X2xvZ2dlci5nbw==) | | |
| [pkg/apisix/route.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2FwaXNpeC9yb3V0ZS5nbw==) | | |
| ... and [25 more](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5d479ae...f820ab5](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633025408
##########
File path: pkg/apisix/cluster.go
##########
@@ -290,6 +303,39 @@ func (c *cluster) GlobalRule() GlobalRule {
return c.globalRules
}
+// HealthCheck implements Cluster.HealthCheck method.
+func (c *cluster) HealthCheck(ctx context.Context, backoff wait.Backoff) (err error) {
+ if c.cacheSyncErr != nil {
+ err = c.cacheSyncErr
+ return
+ }
+ if atomic.LoadInt32(&c.cacheState) == _cacheSyncing {
+ return
+ }
+ var lastCheckErr error
+ err = wait.ExponentialBackoffWithContext(ctx, backoff, func() (done bool, _ error) {
+ if lastCheckErr = c.healthCheck(ctx); lastCheckErr != nil {
+ log.Warnf("failed to HealthCheck for cluster %s: %s, will retry", c.name, lastCheckErr)
+ return
+ }
+ done = true
+ return
+ })
+ if err != nil {
+ // if ErrWaitTimeout then set lastSyncErr
+ c.cacheSyncErr = lastCheckErr
+ }
+ return err
+}
+
+func (c *cluster) healthCheck(ctx context.Context) (err error) {
+ // TODO
Review comment:
Just use TCP socket probe is OK.
##########
File path: pkg/ingress/controller.go
##########
@@ -423,3 +449,26 @@ func (c *Controller) syncSSL(ctx context.Context, ssl *apisixv1.Ssl, event types
}
return err
}
+
+func (c *Controller) checkClusterHealthy(ctx context.Context) {
+ for {
+ select {
+ case <-ctx.Done():
+ case <-time.After(5 * time.Second):
+ }
+
+ // Retry three times in a row, and exit if all of them fail.
+ backoff := wait.Backoff{
+ Duration: 5 * time.Second,
+ Factor: 1,
+ Steps: 3,
+ }
+ err := c.apisix.Cluster(c.cfg.APISIX.DefaultClusterName).HealthCheck(ctx, backoff)
+ if err != nil {
+ // Finally failed health check, then give up leader.
+ c.leaderContextCancelFunc()
+ log.Warnf("failed to HealthCheck for default cluster: %s, give up leader", err)
Review comment:
```suggestion
log.Warnf("failed to check health for default cluster: %s, give up leader", err)
```
##########
File path: pkg/kube/init.go
##########
@@ -25,16 +25,12 @@ import (
// KubeClient contains some objects used to communicate with Kubernetes API Server.
type KubeClient struct {
+ cfg *config.Config
+
// Client is the object used to operate Kubernetes builtin resources.
Client kubernetes.Interface
// APISIXClient is the object used to operate resources under apisix.apache.org group.
APISIXClient clientset.Interface
- // SharedIndexInformerFactory is the index informer factory object used to watch and
Review comment:
Why change this?
##########
File path: pkg/config/config.go
##########
@@ -85,6 +85,8 @@ type APISIXConfig struct {
// DefaultClusterAdminKey is the admin key for the default cluster.
// TODO: Obsolete the plain way to specify admin_key, which is insecure.
DefaultClusterAdminKey string `json:"default_cluster_admin_key" yaml:"default_cluster_admin_key"`
+ // DefaultClusterClientTimeout is the request timeout for default cluster client.
+ DefaultClusterClientTimeout types.TimeDuration `json:"default_cluster_client_timeout" yaml:"default_cluster_client_timeout"`
Review comment:
We will introduce `ApisixClusterConfig` to set these options, so don't add it here.
##########
File path: pkg/apisix/cluster.go
##########
@@ -50,6 +50,19 @@ var (
ErrDuplicatedCluster = errors.New("duplicated cluster")
_errReadOnClosedResBody = errors.New("http: read on closed response body")
+
+ // Default shared transport if apisix client
+ defaultTransport = &http.Transport{
Review comment:
```suggestion
_defaultTransport = &http.Transport{
```
##########
File path: pkg/apisix/apisix.go
##########
@@ -19,6 +19,7 @@ import (
"sync"
v1 "github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
+ "k8s.io/apimachinery/pkg/util/wait"
Review comment:
By convention, we put the 3-party package at the middle of `import`.
```go
import (
std
......
"k8s.io/apimachinery/pkg/util/wait"
v1 "github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
)
```
##########
File path: pkg/ingress/apisix_cluster_config.go
##########
@@ -62,7 +64,6 @@ func (c *apisixClusterConfigController) run(ctx context.Context) {
go c.runWorker(ctx)
}
<-ctx.Done()
- c.workqueue.ShutDown()
Review comment:
It's not related to PR change. If you wanna change them to use `defer`, just in another PR.
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +326,42 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller is start leading ...",
Review comment:
```suggestion
log.Infow("controller tries to leading ...",
```
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +326,42 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller is start leading ...",
zap.String("namespace", c.namespace),
zap.String("pod", c.name),
)
+
defer c.leaderContextCancelFunc()
c.metricsCollector.ResetLeader(true)
- err := c.apisix.AddCluster(&apisix.ClusterOptions{
+ clusterOpts := &apisix.ClusterOptions{
Name: c.cfg.APISIX.DefaultClusterName,
AdminKey: c.cfg.APISIX.DefaultClusterAdminKey,
BaseURL: c.cfg.APISIX.DefaultClusterBaseURL,
- })
+ Timeout: c.cfg.APISIX.DefaultClusterClientTimeout.Duration,
+ }
+ err := c.apisix.AddCluster(clusterOpts)
if err != nil && err != apisix.ErrDuplicatedCluster {
- // TODO give up the leader role.
Review comment:
Don't remove the TODO, unless you really achieve this.
##########
File path: pkg/apisix/nonexistentclient.go
##########
@@ -20,6 +20,7 @@ import (
"github.com/apache/apisix-ingress-controller/pkg/apisix/cache"
v1 "github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
+ "k8s.io/apimachinery/pkg/util/wait"
Review comment:
Ditto with the `import` convention.
##########
File path: pkg/ingress/controller.go
##########
@@ -317,30 +326,42 @@ election:
}
func (c *Controller) run(ctx context.Context) {
- log.Infow("controller now is running as leader",
+ log.Infow("controller is start leading ...",
zap.String("namespace", c.namespace),
zap.String("pod", c.name),
)
+
defer c.leaderContextCancelFunc()
c.metricsCollector.ResetLeader(true)
- err := c.apisix.AddCluster(&apisix.ClusterOptions{
+ clusterOpts := &apisix.ClusterOptions{
Name: c.cfg.APISIX.DefaultClusterName,
AdminKey: c.cfg.APISIX.DefaultClusterAdminKey,
BaseURL: c.cfg.APISIX.DefaultClusterBaseURL,
- })
+ Timeout: c.cfg.APISIX.DefaultClusterClientTimeout.Duration,
+ }
+ err := c.apisix.AddCluster(clusterOpts)
if err != nil && err != apisix.ErrDuplicatedCluster {
- // TODO give up the leader role.
log.Errorf("failed to add default cluster: %s", err)
return
}
if err := c.apisix.Cluster(c.cfg.APISIX.DefaultClusterName).HasSynced(ctx); err != nil {
- // TODO give up the leader role.
Review comment:
Ditto.
##########
File path: pkg/ingress/controller.go
##########
@@ -180,40 +158,71 @@ func NewController(cfg *config.Config) (*Controller, error) {
watchingNamespace: watchingNamespace,
secretSSLMap: new(sync.Map),
recorder: eventBroadcaster.NewRecorder(scheme.Scheme, v1.EventSource{Component: _component}),
-
- epInformer: kubeClient.SharedIndexInformerFactory.Core().V1().Endpoints().Informer(),
- epLister: kubeClient.SharedIndexInformerFactory.Core().V1().Endpoints().Lister(),
- svcInformer: kubeClient.SharedIndexInformerFactory.Core().V1().Services().Informer(),
- svcLister: kubeClient.SharedIndexInformerFactory.Core().V1().Services().Lister(),
- ingressLister: ingressLister,
- ingressInformer: ingressInformer,
- secretInformer: kubeClient.SharedIndexInformerFactory.Core().V1().Secrets().Informer(),
- secretLister: kubeClient.SharedIndexInformerFactory.Core().V1().Secrets().Lister(),
- apisixRouteInformer: apisixRouteInformer,
- apisixRouteLister: apisixRouteLister,
- apisixUpstreamInformer: kubeClient.APISIXSharedIndexInformerFactory.Apisix().V1().ApisixUpstreams().Informer(),
- apisixUpstreamLister: kubeClient.APISIXSharedIndexInformerFactory.Apisix().V1().ApisixUpstreams().Lister(),
- apisixTlsInformer: kubeClient.APISIXSharedIndexInformerFactory.Apisix().V1().ApisixTlses().Informer(),
- apisixTlsLister: kubeClient.APISIXSharedIndexInformerFactory.Apisix().V1().ApisixTlses().Lister(),
- apisixClusterConfigInformer: kubeClient.APISIXSharedIndexInformerFactory.Apisix().V2alpha1().ApisixClusterConfigs().Informer(),
- apisixClusterConfigLister: kubeClient.APISIXSharedIndexInformerFactory.Apisix().V2alpha1().ApisixClusterConfigs().Lister(),
}
+ return c, nil
+}
+
+func (c *Controller) initWhenStartLeader() {
Review comment:
```suggestion
func (c *Controller) initWhenStartLeading() {
```
##########
File path: pkg/kube/apisix/apis/config/v2alpha1/types.go
##########
@@ -17,6 +17,7 @@ package v2alpha1
import (
"encoding/json"
+ "github.com/apache/apisix-ingress-controller/pkg/types"
Review comment:
Put our own packages at the bottom of `import`.
##########
File path: pkg/ingress/controller.go
##########
@@ -423,3 +449,26 @@ func (c *Controller) syncSSL(ctx context.Context, ssl *apisixv1.Ssl, event types
}
return err
}
+
+func (c *Controller) checkClusterHealthy(ctx context.Context) {
Review comment:
```suggestion
func (c *Controller) checkClusterHealth(ctx context.Context) {
```
##########
File path: pkg/apisix/apisix.go
##########
@@ -50,6 +51,8 @@ type Cluster interface {
String() string
// HasSynced checks whether all resources in APISIX cluster is synced to cache.
HasSynced(context.Context) error
+ // HealthCheck checks apisix cluster health in realtime.
+ HealthCheck(context.Context, wait.Backoff) error
Review comment:
IMHO We shall not accept a `wait.Backoff` object from the caller. What we try to do is hide some details and just tell the caller whether the cluster is healthy or not.
##########
File path: pkg/ingress/apisix_cluster_config.go
##########
@@ -62,7 +64,6 @@ func (c *apisixClusterConfigController) run(ctx context.Context) {
go c.runWorker(ctx)
}
<-ctx.Done()
- c.workqueue.ShutDown()
Review comment:
Did you mean the case that cache sync failure?
##########
File path: pkg/kube/init.go
##########
@@ -25,16 +25,12 @@ import (
// KubeClient contains some objects used to communicate with Kubernetes API Server.
type KubeClient struct {
+ cfg *config.Config
+
// Client is the object used to operate Kubernetes builtin resources.
Client kubernetes.Interface
// APISIXClient is the object used to operate resources under apisix.apache.org group.
APISIXClient clientset.Interface
- // SharedIndexInformerFactory is the index informer factory object used to watch and
Review comment:
OK, got it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] codecov-commenter edited a comment on pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#issuecomment-842160355
# [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?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 [#453](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (96b2315) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/5d479ae148d2acdb51082bb0f129548fdfa146b4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5d479ae) will **decrease** coverage by `0.54%`.
> The diff coverage is `14.06%`.
[](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #453 +/- ##
==========================================
- Coverage 37.04% 36.49% -0.55%
==========================================
Files 47 46 -1
Lines 3841 3896 +55
==========================================
- Hits 1423 1422 -1
- Misses 2233 2290 +57
+ Partials 185 184 -1
```
| [Impacted Files](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/apisix/nonexistentclient.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2FwaXNpeC9ub25leGlzdGVudGNsaWVudC5nbw==) | `54.09% <0.00%> (-0.91%)` | :arrow_down: |
| [pkg/ingress/apisix\_cluster\_config.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvYXBpc2l4X2NsdXN0ZXJfY29uZmlnLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/apisix\_route.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvYXBpc2l4X3JvdXRlLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/apisix\_tls.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvYXBpc2l4X3Rscy5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/apisix\_upstream.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvYXBpc2l4X3Vwc3RyZWFtLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/controller.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvY29udHJvbGxlci5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/endpoint.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvZW5kcG9pbnQuZ28=) | `0.00% <0.00%> (ø)` | |
| [pkg/ingress/ingress.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3MvaW5ncmVzcy5nbw==) | `8.58% <0.00%> (ø)` | |
| [pkg/ingress/secret.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2luZ3Jlc3Mvc2VjcmV0Lmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/apisix/cluster.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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-cGtnL2FwaXNpeC9jbHVzdGVyLmdv) | `24.33% <33.33%> (-2.71%)` | :arrow_down: |
| ... and [2 more](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5d479ae...96b2315](https://codecov.io/gh/apache/apisix-ingress-controller/pull/453?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] gxthrj commented on a change in pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#discussion_r633098741
##########
File path: pkg/apisix/cluster.go
##########
@@ -132,13 +144,20 @@ func (c *cluster) syncCache() {
}()
backoff := wait.Backoff{
- Duration: time.Second,
- Factor: 2,
- Steps: 6,
- }
- err := wait.ExponentialBackoff(backoff, c.syncCacheOnce)
+ Duration: 2 * time.Second,
+ Factor: 1,
+ Steps: 5,
+ }
+ var lastSyncErr error
+ err := wait.ExponentialBackoff(backoff, func() (done bool, _ error) {
+ // not possible return: false, nil
Review comment:
```suggestion
// impossibly return: false, nil
```
##########
File path: pkg/ingress/controller.go
##########
@@ -423,3 +449,26 @@ func (c *Controller) syncSSL(ctx context.Context, ssl *apisixv1.Ssl, event types
}
return err
}
+
+func (c *Controller) checkClusterHealthy(ctx context.Context) {
+ for {
+ select {
+ case <-ctx.Done():
+ case <-time.After(5 * time.Second):
+ }
+
+ // Retry three times in a row, and exit if all of them fail.
+ backoff := wait.Backoff{
Review comment:
Put logic into the `case <-time.After(5 * time.Second)`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tokers commented on pull request #453: fix: panic of start leading. sync ingress failed when apisix not start.
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #453:
URL: https://github.com/apache/apisix-ingress-controller/pull/453#issuecomment-845627618
@slene Any new updates?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org