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/01/14 14:06:43 UTC

[GitHub] [apisix-ingress-controller] tokers opened a new pull request #176: chore: caching

tokers opened a new pull request #176:
URL: https://github.com/apache/apisix-ingress-controller/pull/176


   Please answer these questions before submitting a pull request
   
   - Why submit this pull request?
   - [ ] Bugfix
   - [x] New feature provided
   - [ ] Improve performance
   - [ ] Backport patches
   
   - Related issues
   
   ___
   ### 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 pull request #176: chore: caching

Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #176:
URL: https://github.com/apache/apisix-ingress-controller/pull/176#issuecomment-762640268


   @gxthrj Please take a look when you have time.


----------------------------------------------------------------
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 #176: chore: caching

Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #176:
URL: https://github.com/apache/apisix-ingress-controller/pull/176#discussion_r560713624



##########
File path: pkg/seven/state/builder.go
##########
@@ -227,64 +221,18 @@ func SolverSingleUpstream(u *v1.Upstream, swg ServiceWorkerGroup, wg *sync.WaitG
 						needToUpdate = false
 					}
 				}
-				if needToUpdate {
-					// 1.sync memDB
-					upstreamDB := &db.UpstreamDB{Upstreams: []*v1.Upstream{u}}
-					if err := upstreamDB.UpdateUpstreams(); err != nil {
-						log.Errorf("solver upstream failed, update upstream to local db failed, err: %s", err.Error())
-						errNotify = err
-						return
-					}
-
-					// 2.sync apisix
-					var cluster string
-					if u.Group != nil {
-						cluster = *u.Group
-					}
+				if needToUpdate || (u.FromKind != nil && *u.FromKind == WatchFromKind) {

Review comment:
       Fixed.

##########
File path: pkg/apisix/route.go
##########
@@ -48,9 +48,17 @@ func newRouteClient(c *cluster) Route {
 	}
 }
 
+// Get only looks up the cache, it's not necessary to access APISIX, since all resources
+// are created by Create, which reflects the change to cache in turn, so if resource
+// is not in cache, it's not in APISIX either.
+func (r *routeClient) Get(_ context.Context, fullname string) (*v1.Route, error) {
+	return r.cluster.cache.GetRoute(fullname)

Review comment:
       Added.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-ingress-controller] tokers commented on pull request #176: chore: caching

Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #176:
URL: https://github.com/apache/apisix-ingress-controller/pull/176#issuecomment-763348403


   > Unit-test CI failed @tokers Please have a look.
   
   Fixed.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-ingress-controller] codecov-io edited a comment on pull request #176: chore: caching

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #176:
URL: https://github.com/apache/apisix-ingress-controller/pull/176#issuecomment-762555613


   # [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=h1) Report
   > Merging [#176](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=desc) (16a9a9d) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/e11f7e092afa1f5fd673003baacf21f7adc4025a?el=desc) (e11f7e0) will **decrease** coverage by `1.10%`.
   > The diff coverage is `35.73%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/graphs/tree.svg?width=650&height=150&src=pr&token=WPLQXPY3V0)](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #176      +/-   ##
   ==========================================
   - Coverage   48.65%   47.54%   -1.11%     
   ==========================================
     Files          30       31       +1     
     Lines        1482     1874     +392     
   ==========================================
   + Hits          721      891     +170     
   - Misses        675      844     +169     
   - Partials       86      139      +53     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cmd/ingress/ingress.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-Y21kL2luZ3Jlc3MvaW5ncmVzcy5nbw==) | `70.76% <0.00%> (ø)` | |
   | [pkg/seven/state/builder.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL3NldmVuL3N0YXRlL2J1aWxkZXIuZ28=) | `0.00% <0.00%> (ø)` | |
   | [pkg/seven/state/service\_worker.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL3NldmVuL3N0YXRlL3NlcnZpY2Vfd29ya2VyLmdv) | `0.00% <0.00%> (ø)` | |
   | [pkg/seven/state/solver.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL3NldmVuL3N0YXRlL3NvbHZlci5nbw==) | `3.96% <0.00%> (+2.26%)` | :arrow_up: |
   | [pkg/apisix/cluster.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9jbHVzdGVyLmdv) | `31.77% <22.61%> (-7.12%)` | :arrow_down: |
   | [pkg/apisix/nonexistentclient.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9ub25leGlzdGVudGNsaWVudC5nbw==) | `71.42% <36.36%> (-28.58%)` | :arrow_down: |
   | [pkg/apisix/service.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9zZXJ2aWNlLmdv) | `47.54% <36.61%> (-19.71%)` | :arrow_down: |
   | [pkg/apisix/upstream.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC91cHN0cmVhbS5nbw==) | `51.72% <36.61%> (-18.16%)` | :arrow_down: |
   | [pkg/apisix/route.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9yb3V0ZS5nbw==) | `46.45% <37.50%> (-16.45%)` | :arrow_down: |
   | [pkg/apisix/ssl.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9zc2wuZ28=) | `53.84% <46.00%> (-11.73%)` | :arrow_down: |
   | ... and [4 more](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=footer). Last update [e11f7e0...16a9a9d](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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 merged pull request #176: chore: caching

Posted by GitBox <gi...@apache.org>.
tokers merged pull request #176:
URL: https://github.com/apache/apisix-ingress-controller/pull/176


   


----------------------------------------------------------------
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 #176: chore: caching

Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #176:
URL: https://github.com/apache/apisix-ingress-controller/pull/176#discussion_r559965159



##########
File path: pkg/seven/state/builder.go
##########
@@ -227,64 +221,18 @@ func SolverSingleUpstream(u *v1.Upstream, swg ServiceWorkerGroup, wg *sync.WaitG
 						needToUpdate = false
 					}
 				}
-				if needToUpdate {
-					// 1.sync memDB
-					upstreamDB := &db.UpstreamDB{Upstreams: []*v1.Upstream{u}}
-					if err := upstreamDB.UpdateUpstreams(); err != nil {
-						log.Errorf("solver upstream failed, update upstream to local db failed, err: %s", err.Error())
-						errNotify = err
-						return
-					}
-
-					// 2.sync apisix
-					var cluster string
-					if u.Group != nil {
-						cluster = *u.Group
-					}
+				if needToUpdate || (u.FromKind != nil && *u.FromKind == WatchFromKind) {

Review comment:
       Need to differentiate `service registration` and `CRD synchronization`, 
   When `FromKind == WatchFromKind`, the upstream info only contains nodes, and will lose other information in upstream, such as loadBalance infomation.

##########
File path: pkg/apisix/cluster.go
##########
@@ -85,20 +89,129 @@ func newCluster(o *ClusterOptions) (Cluster, error) {
 				ExpectContinueTimeout: o.Timeout,
 			},
 		},
+		cache:      nil,
+		cacheReady: make(chan struct{}),
 	}
 	c.route = newRouteClient(c)
 	c.upstream = newUpstreamClient(c)
 	c.service = newServiceClient(c)
 	c.ssl = newSSLClient(c)
 
+	go c.warmingUp()
+
 	return c, nil
 }
 
-// String exposes the client information in human readable format.
+func (c *cluster) warmingUp() {
+	log.Infow("warming up caching", zap.String("cluster", c.name))
+	now := time.Now()
+	defer log.Infow("caching warmed",
+		zap.String("cost_time", time.Now().Sub(now).String()),
+		zap.String("cluster", c.name),
+	)
+
+	backoff := wait.Backoff{
+		Duration: time.Second,
+		Factor:   2,
+		Steps:    6,
+	}
+	err := wait.ExponentialBackoff(backoff, c.warmingUpOnce)
+	if err != nil {
+		c.cacheWarmingUpErr = err
+	}
+	close(c.cacheReady)
+}
+
+func (c *cluster) warmingUpOnce() (bool, error) {
+	dbcache, err := cache.NewMemDBCache()
+	if err != nil {
+		return false, err
+	}
+	c.cache = dbcache
+
+	routes, err := c.route.List(context.TODO())
+	if err != nil {
+		log.Errorf("failed to list route in APISIX: %s", err)
+		return false, err
+	}
+	services, err := c.service.List(context.TODO())
+	if err != nil {
+		log.Errorf("failed to list services in APISIX: %s", err)
+		return false, err
+	}
+	upstreams, err := c.upstream.List(context.TODO())
+	if err != nil {
+		log.Errorf("failed to list upstreams in APISIX: %s", err)
+		return false, err
+	}
+	ssl, err := c.ssl.List(context.TODO())
+	if err != nil {
+		log.Errorf("failed to list ssl in APISIX: %s", err)
+		return false, err
+	}
+
+	for _, r := range routes {
+		if err := c.cache.InsertRoute(r); err != nil {
+			log.Errorw("failed to insert route to cache",
+				zap.String("route", *r.ID),
+				zap.String("cluster", c.name),
+				zap.String("error", err.Error()),
+			)
+			return false, err
+		}
+	}
+	for _, s := range services {
+		if err := c.cache.InsertService(s); err != nil {
+			log.Errorw("failed to insert service to cache",
+				zap.String("service", *s.ID),
+				zap.String("cluster", c.name),
+				zap.String("error", err.Error()),
+			)
+			return false, err
+		}
+	}
+	for _, u := range upstreams {
+		if err := c.cache.InsertUpstream(u); err != nil {
+			log.Errorw("failed to insert upstream to cache",
+				zap.String("upstream", *u.ID),
+				zap.String("cluster", c.name),
+				zap.String("error", err.Error()),
+			)
+			return false, err
+		}
+	}
+	for _, s := range ssl {
+		if err := c.cache.InsertSSL(s); err != nil {
+			log.Errorw("failed to insert ssl to cache",
+				zap.String("ssl", *s.ID),
+				zap.String("cluster", c.name),
+				zap.String("error", err.Error()),
+			)
+			return false, err
+		}
+	}
+	return true, nil
+}
+
+// String implements Cluster.String method.
 func (c *cluster) String() string {
 	return fmt.Sprintf("name=%s; base_url=%s", c.name, c.baseURL)
 }
 
+// Ready implements Cluster.Ready method.
+func (c *cluster) Ready(ctx context.Context) error {
+	if c.cacheWarmingUpErr != nil {
+		return c.cacheWarmingUpErr
+	}
+	select {
+	case <-ctx.Done():
+		log.Errorf("failed to wait cluster to ready: %s", ctx.Err())
+		return ctx.Err()
+	case <-c.cacheReady:
+		return nil

Review comment:
       Had better to record log when ready .

##########
File path: pkg/apisix/route.go
##########
@@ -48,9 +48,17 @@ func newRouteClient(c *cluster) Route {
 	}
 }
 
+// Get only looks up the cache, it's not necessary to access APISIX, since all resources
+// are created by Create, which reflects the change to cache in turn, so if resource
+// is not in cache, it's not in APISIX either.
+func (r *routeClient) Get(_ context.Context, fullname string) (*v1.Route, error) {
+	return r.cluster.cache.GetRoute(fullname)

Review comment:
       If cache return nil, need to read from admin api.
   Or we need a way to resync from APISIX to cache.

##########
File path: pkg/seven/state/solver.go
##########
@@ -57,92 +57,58 @@ func (s *ApisixCombination) Solver() (string, error) {
 func (s *ApisixCombination) Remove() error {
 	// services
 	for _, svc := range s.Services {
-		if err := RemoveService(svc); err != nil {
-			return err
+		var cluster string
+		if svc.Group != nil {
+			cluster = *svc.Group
 		}
-	}
-
-	// upstreams
-	for _, up := range s.Upstreams {
-		if err := RemoveUpstream(up); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-func RemoveService(svc *v1.Service) error {
-	// find svc ID
-	serviceRequest := db.ServiceRequest{FullName: *svc.FullName}
-	if service, err := serviceRequest.FindByName(); err != nil {
-		// if not found, keep going on.
-		if errors.Is(err, utils.ErrNotFound) {
-			return nil
-		}
-		return err
-	} else {
-		svc.ID = service.ID
-	}
-	// find ref route
-	routeRequest := db.RouteRequest{ServiceId: *svc.ID}
-	if route, err := routeRequest.ExistByServiceId(); err != nil {
-		if !errors.Is(err, utils.ErrNotFound) {
-			// except ErrNotFound, need to retry
-			return err
-		} else {
-			// do delete svc
-			var cluster string
-			if svc.Group != nil {
-				cluster = *svc.Group
-			}
-			if err := conf.Client.Cluster(cluster).Service().Delete(context.TODO(), svc); err != nil {
-				log.Errorf("failed to delete svc %s from APISIX: %s", *svc.FullName, err)
+		svcInCache, err := conf.Client.Cluster(cluster).Service().Get(context.TODO(), *svc.FullName)
+		if err != nil {
+			if err == cache.ErrNotFound {
+				log.Errorf("failed to remove service %s: %s", *svc.FullName, err)
+				continue
+			} else {
 				return err
+			}
+		}
+		paddingService(svc, svcInCache)
+		err = conf.Client.Cluster(cluster).Service().Delete(context.TODO(), svc)
+		if err != nil {
+			if err == cache.ErrNotFound {
+				log.Errorf("failed to remove service %s: %s", *svc.FullName, err)
+			} else if err == cache.ErrStillInUse {
+				log.Warnf("failed to remove service %s: %s", *svc.FullName, err)
 			} else {
-				db := db.ServiceDB{Services: []*v1.Service{svc}}
-				db.DeleteService()
-				return nil
+				return err
 			}
 		}
-	} else {
-		return fmt.Errorf("svc %s is still referenced by route %s", *svc.FullName, *route.FullName)
 	}
-}
 
-func RemoveUpstream(up *v1.Upstream) error {
-	upstreamRequest := db.UpstreamRequest{FullName: *up.FullName}
-	if upstream, err := upstreamRequest.FindByName(); err != nil {
-		// if not found, keep going on.
-		if errors.Is(err, utils.ErrNotFound) {
-			return nil
+	// upstreams
+	for _, ups := range s.Upstreams {
+		var cluster string
+		if ups.Group != nil {
+			cluster = *ups.Group
 		}
-		return err
-	} else {
-		up.ID = upstream.ID
-	}
-	serviceRequest := db.ServiceRequest{UpstreamId: *up.ID}
-	if svc, err := serviceRequest.ExistByUpstreamId(); err != nil {

Review comment:
       Where is this logic in this refactor, need to check if upstream is refer by any service.




----------------------------------------------------------------
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 #176: chore: caching

Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #176:
URL: https://github.com/apache/apisix-ingress-controller/pull/176#discussion_r560724048



##########
File path: pkg/apisix/apisix.go
##########
@@ -73,39 +81,28 @@ type Upstream interface {
 // Service is the specific client interface to take over the create, update,
 // list and delete for APISIX's Service resource.
 type Service interface {
+	Get(context.Context, string) (*v1.Service, error)
 	List(context.Context) ([]*v1.Service, error)
 	Create(context.Context, *v1.Service) (*v1.Service, error)
 	Delete(context.Context, *v1.Service) error
 	Update(context.Context, *v1.Service) (*v1.Service, error)
 }
 
 type apisix struct {
-	defaultCluster     Cluster
 	nonExistentCluster Cluster
-	defaultClusterName string
 	clusters           map[string]Cluster
 }
 
-// NewForOptions creates an APISIX client to perform resources change pushing.
-// Users should carry a ClusterOptions to configure the default APISIX cluster.
-func NewForOptions(co *ClusterOptions) (APISIX, error) {
-	defaultCluster, err := newCluster(co)
-	if err != nil {
-		return nil, err
-	}
+// New creates an APISIX client to perform resources change pushing.
+func New() (APISIX, error) {

Review comment:
       Suggest to use `Client()` instead of `New()`




----------------------------------------------------------------
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 #176: chore: caching

Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #176:
URL: https://github.com/apache/apisix-ingress-controller/pull/176#discussion_r559971926



##########
File path: pkg/apisix/cluster.go
##########
@@ -85,20 +89,129 @@ func newCluster(o *ClusterOptions) (Cluster, error) {
 				ExpectContinueTimeout: o.Timeout,
 			},
 		},
+		cache:      nil,
+		cacheReady: make(chan struct{}),
 	}
 	c.route = newRouteClient(c)
 	c.upstream = newUpstreamClient(c)
 	c.service = newServiceClient(c)
 	c.ssl = newSSLClient(c)
 
+	go c.warmingUp()
+
 	return c, nil
 }
 
-// String exposes the client information in human readable format.
+func (c *cluster) warmingUp() {
+	log.Infow("warming up caching", zap.String("cluster", c.name))
+	now := time.Now()
+	defer log.Infow("caching warmed",
+		zap.String("cost_time", time.Now().Sub(now).String()),
+		zap.String("cluster", c.name),
+	)
+
+	backoff := wait.Backoff{
+		Duration: time.Second,
+		Factor:   2,
+		Steps:    6,
+	}
+	err := wait.ExponentialBackoff(backoff, c.warmingUpOnce)
+	if err != nil {
+		c.cacheWarmingUpErr = err
+	}
+	close(c.cacheReady)
+}
+
+func (c *cluster) warmingUpOnce() (bool, error) {
+	dbcache, err := cache.NewMemDBCache()
+	if err != nil {
+		return false, err
+	}
+	c.cache = dbcache
+
+	routes, err := c.route.List(context.TODO())
+	if err != nil {
+		log.Errorf("failed to list route in APISIX: %s", err)
+		return false, err
+	}
+	services, err := c.service.List(context.TODO())
+	if err != nil {
+		log.Errorf("failed to list services in APISIX: %s", err)
+		return false, err
+	}
+	upstreams, err := c.upstream.List(context.TODO())
+	if err != nil {
+		log.Errorf("failed to list upstreams in APISIX: %s", err)
+		return false, err
+	}
+	ssl, err := c.ssl.List(context.TODO())
+	if err != nil {
+		log.Errorf("failed to list ssl in APISIX: %s", err)
+		return false, err
+	}
+
+	for _, r := range routes {
+		if err := c.cache.InsertRoute(r); err != nil {
+			log.Errorw("failed to insert route to cache",
+				zap.String("route", *r.ID),
+				zap.String("cluster", c.name),
+				zap.String("error", err.Error()),
+			)
+			return false, err
+		}
+	}
+	for _, s := range services {
+		if err := c.cache.InsertService(s); err != nil {
+			log.Errorw("failed to insert service to cache",
+				zap.String("service", *s.ID),
+				zap.String("cluster", c.name),
+				zap.String("error", err.Error()),
+			)
+			return false, err
+		}
+	}
+	for _, u := range upstreams {
+		if err := c.cache.InsertUpstream(u); err != nil {
+			log.Errorw("failed to insert upstream to cache",
+				zap.String("upstream", *u.ID),
+				zap.String("cluster", c.name),
+				zap.String("error", err.Error()),
+			)
+			return false, err
+		}
+	}
+	for _, s := range ssl {
+		if err := c.cache.InsertSSL(s); err != nil {
+			log.Errorw("failed to insert ssl to cache",
+				zap.String("ssl", *s.ID),
+				zap.String("cluster", c.name),
+				zap.String("error", err.Error()),
+			)
+			return false, err
+		}
+	}
+	return true, nil
+}
+
+// String implements Cluster.String method.
 func (c *cluster) String() string {
 	return fmt.Sprintf("name=%s; base_url=%s", c.name, c.baseURL)
 }
 
+// Ready implements Cluster.Ready method.
+func (c *cluster) Ready(ctx context.Context) error {
+	if c.cacheWarmingUpErr != nil {
+		return c.cacheWarmingUpErr
+	}
+	select {
+	case <-ctx.Done():
+		log.Errorf("failed to wait cluster to ready: %s", ctx.Err())
+		return ctx.Err()
+	case <-c.cacheReady:
+		return nil

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] codecov-io edited a comment on pull request #176: chore: caching

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #176:
URL: https://github.com/apache/apisix-ingress-controller/pull/176#issuecomment-762555613


   # [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=h1) Report
   > Merging [#176](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=desc) (f44eb45) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/e11f7e092afa1f5fd673003baacf21f7adc4025a?el=desc) (e11f7e0) will **decrease** coverage by `1.21%`.
   > The diff coverage is `35.73%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/graphs/tree.svg?width=650&height=150&src=pr&token=WPLQXPY3V0)](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #176      +/-   ##
   ==========================================
   - Coverage   48.65%   47.43%   -1.22%     
   ==========================================
     Files          30       31       +1     
     Lines        1482     1874     +392     
   ==========================================
   + Hits          721      889     +168     
   - Misses        675      846     +171     
   - Partials       86      139      +53     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cmd/ingress/ingress.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-Y21kL2luZ3Jlc3MvaW5ncmVzcy5nbw==) | `70.76% <0.00%> (ø)` | |
   | [pkg/seven/state/builder.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL3NldmVuL3N0YXRlL2J1aWxkZXIuZ28=) | `0.00% <0.00%> (ø)` | |
   | [pkg/seven/state/service\_worker.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL3NldmVuL3N0YXRlL3NlcnZpY2Vfd29ya2VyLmdv) | `0.00% <0.00%> (ø)` | |
   | [pkg/seven/state/solver.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL3NldmVuL3N0YXRlL3NvbHZlci5nbw==) | `1.98% <0.00%> (+0.28%)` | :arrow_up: |
   | [pkg/apisix/cluster.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9jbHVzdGVyLmdv) | `31.77% <22.61%> (-7.12%)` | :arrow_down: |
   | [pkg/apisix/nonexistentclient.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9ub25leGlzdGVudGNsaWVudC5nbw==) | `71.42% <36.36%> (-28.58%)` | :arrow_down: |
   | [pkg/apisix/service.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9zZXJ2aWNlLmdv) | `47.54% <36.61%> (-19.71%)` | :arrow_down: |
   | [pkg/apisix/upstream.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC91cHN0cmVhbS5nbw==) | `51.72% <36.61%> (-18.16%)` | :arrow_down: |
   | [pkg/apisix/route.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9yb3V0ZS5nbw==) | `46.45% <37.50%> (-16.45%)` | :arrow_down: |
   | [pkg/apisix/ssl.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9zc2wuZ28=) | `53.84% <46.00%> (-11.73%)` | :arrow_down: |
   | ... and [3 more](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=footer). Last update [e11f7e0...f44eb45](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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-io commented on pull request #176: chore: caching

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #176:
URL: https://github.com/apache/apisix-ingress-controller/pull/176#issuecomment-762555613


   # [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=h1) Report
   > Merging [#176](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=desc) (0ec4927) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/958a02b76cae8e624372f30940fa7bdc2d4b5425?el=desc) (958a02b) will **decrease** coverage by `0.62%`.
   > The diff coverage is `31.70%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/graphs/tree.svg?width=650&height=150&src=pr&token=WPLQXPY3V0)](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #176      +/-   ##
   ==========================================
   - Coverage   48.65%   48.02%   -0.63%     
   ==========================================
     Files          30       31       +1     
     Lines        1482     1720     +238     
   ==========================================
   + Hits          721      826     +105     
   - Misses        675      756      +81     
   - Partials       86      138      +52     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cmd/ingress/ingress.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-Y21kL2luZ3Jlc3MvaW5ncmVzcy5nbw==) | `70.76% <0.00%> (ø)` | |
   | [pkg/seven/state/builder.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL3NldmVuL3N0YXRlL2J1aWxkZXIuZ28=) | `0.00% <0.00%> (ø)` | |
   | [pkg/seven/state/service\_worker.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL3NldmVuL3N0YXRlL3NlcnZpY2Vfd29ya2VyLmdv) | `0.00% <0.00%> (ø)` | |
   | [pkg/seven/state/solver.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL3NldmVuL3N0YXRlL3NvbHZlci5nbw==) | `1.98% <0.00%> (+0.28%)` | :arrow_up: |
   | [pkg/apisix/route.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9yb3V0ZS5nbw==) | `47.67% <18.51%> (-15.23%)` | :arrow_down: |
   | [pkg/apisix/service.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9zZXJ2aWNlLmdv) | `50.00% <18.51%> (-17.25%)` | :arrow_down: |
   | [pkg/apisix/ssl.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9zc2wuZ28=) | `49.41% <18.51%> (-16.17%)` | :arrow_down: |
   | [pkg/apisix/upstream.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC91cHN0cmVhbS5nbw==) | `56.07% <21.42%> (-13.81%)` | :arrow_down: |
   | [pkg/apisix/cluster.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9jbHVzdGVyLmdv) | `34.06% <27.02%> (-4.83%)` | :arrow_down: |
   | [pkg/apisix/nonexistentclient.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9ub25leGlzdGVudGNsaWVudC5nbw==) | `71.42% <36.36%> (-28.58%)` | :arrow_down: |
   | ... and [3 more](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=footer). Last update [958a02b...0ec4927](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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-io edited a comment on pull request #176: chore: caching

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #176:
URL: https://github.com/apache/apisix-ingress-controller/pull/176#issuecomment-762555613


   # [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=h1) Report
   > Merging [#176](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=desc) (541daa5) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/958a02b76cae8e624372f30940fa7bdc2d4b5425?el=desc) (958a02b) will **decrease** coverage by `1.21%`.
   > The diff coverage is `35.73%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/graphs/tree.svg?width=650&height=150&src=pr&token=WPLQXPY3V0)](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #176      +/-   ##
   ==========================================
   - Coverage   48.65%   47.43%   -1.22%     
   ==========================================
     Files          30       31       +1     
     Lines        1482     1874     +392     
   ==========================================
   + Hits          721      889     +168     
   - Misses        675      846     +171     
   - Partials       86      139      +53     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cmd/ingress/ingress.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-Y21kL2luZ3Jlc3MvaW5ncmVzcy5nbw==) | `70.76% <0.00%> (ø)` | |
   | [pkg/seven/state/builder.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL3NldmVuL3N0YXRlL2J1aWxkZXIuZ28=) | `0.00% <0.00%> (ø)` | |
   | [pkg/seven/state/service\_worker.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL3NldmVuL3N0YXRlL3NlcnZpY2Vfd29ya2VyLmdv) | `0.00% <0.00%> (ø)` | |
   | [pkg/seven/state/solver.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL3NldmVuL3N0YXRlL3NvbHZlci5nbw==) | `1.98% <0.00%> (+0.28%)` | :arrow_up: |
   | [pkg/apisix/cluster.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9jbHVzdGVyLmdv) | `31.77% <22.61%> (-7.12%)` | :arrow_down: |
   | [pkg/apisix/nonexistentclient.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9ub25leGlzdGVudGNsaWVudC5nbw==) | `71.42% <36.36%> (-28.58%)` | :arrow_down: |
   | [pkg/apisix/service.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9zZXJ2aWNlLmdv) | `47.54% <36.61%> (-19.71%)` | :arrow_down: |
   | [pkg/apisix/upstream.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC91cHN0cmVhbS5nbw==) | `51.72% <36.61%> (-18.16%)` | :arrow_down: |
   | [pkg/apisix/route.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9yb3V0ZS5nbw==) | `46.45% <37.50%> (-16.45%)` | :arrow_down: |
   | [pkg/apisix/ssl.go](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree#diff-cGtnL2FwaXNpeC9zc2wuZ28=) | `53.84% <46.00%> (-11.73%)` | :arrow_down: |
   | ... and [3 more](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=footer). Last update [958a02b...541daa5](https://codecov.io/gh/apache/apisix-ingress-controller/pull/176?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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 #176: chore: caching

Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #176:
URL: https://github.com/apache/apisix-ingress-controller/pull/176#discussion_r559972425



##########
File path: pkg/seven/state/solver.go
##########
@@ -57,92 +57,58 @@ func (s *ApisixCombination) Solver() (string, error) {
 func (s *ApisixCombination) Remove() error {
 	// services
 	for _, svc := range s.Services {
-		if err := RemoveService(svc); err != nil {
-			return err
+		var cluster string
+		if svc.Group != nil {
+			cluster = *svc.Group
 		}
-	}
-
-	// upstreams
-	for _, up := range s.Upstreams {
-		if err := RemoveUpstream(up); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-func RemoveService(svc *v1.Service) error {
-	// find svc ID
-	serviceRequest := db.ServiceRequest{FullName: *svc.FullName}
-	if service, err := serviceRequest.FindByName(); err != nil {
-		// if not found, keep going on.
-		if errors.Is(err, utils.ErrNotFound) {
-			return nil
-		}
-		return err
-	} else {
-		svc.ID = service.ID
-	}
-	// find ref route
-	routeRequest := db.RouteRequest{ServiceId: *svc.ID}
-	if route, err := routeRequest.ExistByServiceId(); err != nil {
-		if !errors.Is(err, utils.ErrNotFound) {
-			// except ErrNotFound, need to retry
-			return err
-		} else {
-			// do delete svc
-			var cluster string
-			if svc.Group != nil {
-				cluster = *svc.Group
-			}
-			if err := conf.Client.Cluster(cluster).Service().Delete(context.TODO(), svc); err != nil {
-				log.Errorf("failed to delete svc %s from APISIX: %s", *svc.FullName, err)
+		svcInCache, err := conf.Client.Cluster(cluster).Service().Get(context.TODO(), *svc.FullName)
+		if err != nil {
+			if err == cache.ErrNotFound {
+				log.Errorf("failed to remove service %s: %s", *svc.FullName, err)
+				continue
+			} else {
 				return err
+			}
+		}
+		paddingService(svc, svcInCache)
+		err = conf.Client.Cluster(cluster).Service().Delete(context.TODO(), svc)
+		if err != nil {
+			if err == cache.ErrNotFound {
+				log.Errorf("failed to remove service %s: %s", *svc.FullName, err)
+			} else if err == cache.ErrStillInUse {
+				log.Warnf("failed to remove service %s: %s", *svc.FullName, err)
 			} else {
-				db := db.ServiceDB{Services: []*v1.Service{svc}}
-				db.DeleteService()
-				return nil
+				return err
 			}
 		}
-	} else {
-		return fmt.Errorf("svc %s is still referenced by route %s", *svc.FullName, *route.FullName)
 	}
-}
 
-func RemoveUpstream(up *v1.Upstream) error {
-	upstreamRequest := db.UpstreamRequest{FullName: *up.FullName}
-	if upstream, err := upstreamRequest.FindByName(); err != nil {
-		// if not found, keep going on.
-		if errors.Is(err, utils.ErrNotFound) {
-			return nil
+	// upstreams
+	for _, ups := range s.Upstreams {
+		var cluster string
+		if ups.Group != nil {
+			cluster = *ups.Group
 		}
-		return err
-	} else {
-		up.ID = upstream.ID
-	}
-	serviceRequest := db.ServiceRequest{UpstreamId: *up.ID}
-	if svc, err := serviceRequest.ExistByUpstreamId(); err != nil {

Review comment:
       In Delete implementations.




----------------------------------------------------------------
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 pull request #176: chore: caching

Posted by GitBox <gi...@apache.org>.
gxthrj commented on pull request #176:
URL: https://github.com/apache/apisix-ingress-controller/pull/176#issuecomment-763342296


   Unit-test CI  failed @tokers Please have a look.


----------------------------------------------------------------
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 #176: chore: caching

Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #176:
URL: https://github.com/apache/apisix-ingress-controller/pull/176#discussion_r560774798



##########
File path: pkg/apisix/apisix.go
##########
@@ -73,39 +81,28 @@ type Upstream interface {
 // Service is the specific client interface to take over the create, update,
 // list and delete for APISIX's Service resource.
 type Service interface {
+	Get(context.Context, string) (*v1.Service, error)
 	List(context.Context) ([]*v1.Service, error)
 	Create(context.Context, *v1.Service) (*v1.Service, error)
 	Delete(context.Context, *v1.Service) error
 	Update(context.Context, *v1.Service) (*v1.Service, error)
 }
 
 type apisix struct {
-	defaultCluster     Cluster
 	nonExistentCluster Cluster
-	defaultClusterName string
 	clusters           map[string]Cluster
 }
 
-// NewForOptions creates an APISIX client to perform resources change pushing.
-// Users should carry a ClusterOptions to configure the default APISIX cluster.
-func NewForOptions(co *ClusterOptions) (APISIX, error) {
-	defaultCluster, err := newCluster(co)
-	if err != nil {
-		return nil, err
-	}
+// New creates an APISIX client to perform resources change pushing.
+func New() (APISIX, error) {

Review comment:
       Changed to NewClient.




----------------------------------------------------------------
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