Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add memory storage for supporting ListAndWatch #345

Merged
merged 7 commits into from
Sep 19, 2022

Conversation

xyz2277
Copy link
Contributor

@xyz2277 xyz2277 commented Sep 6, 2022

Co-authored-by: duanmeng duanmeng_yewu@cmss.chinamobile.com
Co-authored-by: wuyingjun wuyingjun_yewu@cmss.chinamobile.com
Co-authored-by: hanweisen hanweisen_yewu@cmss.chinamobile.com
Signed-off-by: zhangyongxi zhangyongxi_yewu@cmss.chinamobile.com

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #265

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

1. Add memory storage.
2. Support ListAndWatch for Multi-cluster.

@clusterpedia-bot
Copy link

Hi @xyz2277,
Thanks for your pull request!
If the PR is ready, use the /auto-cc command to assign Reviewer to Review.
We will review it shortly.

Details

Instructions for interacting with me using comments are available here.
If you have questions or suggestions related to my behavior, please file an issue against the gh-ci-bot repository.

@wuyingjun-lucky wuyingjun-lucky force-pushed the memory-storage-1 branch 2 times, most recently from cc363ab to 6fc7bc6 Compare September 6, 2022 10:05
@wuyingjun-lucky
Copy link
Member

/cc @Iceber
May be you guys can start first found review

@Iceber
Copy link
Member

Iceber commented Sep 6, 2022

Thanks to everyone who contributed 🌹, I will start reviewing

@wuyingjun-lucky wuyingjun-lucky force-pushed the memory-storage-1 branch 2 times, most recently from 378fcd6 to 0e6e290 Compare September 7, 2022 02:38
@@ -26,7 +26,7 @@ ifeq ($(LATEST_TAG),$(shell git describe --abbrev=0 --tags))
VERSION=$(LATEST_TAG)
endif

all: apiserver clustersynchro-manager controller-manager
all: apiserver binding-apiserver clustersynchro-manager controller-manager
Copy link
Member

@Iceber Iceber Sep 8, 2022

Choose a reason for hiding this comment

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

I feel that this name is not very good, but I do not have a good suggestion:laughing:, so if there is no better name, we can start with this.

@@ -28,3 +28,6 @@ subjects:
- kind: ServiceAccount
name: clusterpedia-controller-manager
namespace: clusterpedia-system
- kind: ServiceAccount
name: clusterpedia-binding-apiserver
namespace: clusterpedia-system
Copy link
Member

Choose a reason for hiding this comment

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

I think there should be a clusterrole&clusterrolebinding file under the directory

synchromanager := synchromanager.NewManager(crdclient, config.StorageFactory)
synchromanager.Run(1, context.StopCh)
}

Copy link
Member

Choose a reason for hiding this comment

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

Regarding the clustersynchro manager, is it possible to run it in cmd? so that apiserver and clustersynchro manager do not have any coupling and just use the same storage factory.

Copy link
Member

Choose a reason for hiding this comment

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

The manager also does not rely on the clusterpediaInformerFactory, it creates a new infromer using the crdclient, so there is no need to wait for the clusterpediaInformerFactory.WaitForCacheSync

Copy link
Member

Choose a reason for hiding this comment

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

The clustersynchro use kubeconfig to get crdclient,and apiserver use GenericConfig *genericapiserver.RecommendedConfig,if we put it in cmd,should we add a kubeconfig flag,or use GenericConfig *genericapiserver.RecommendedConfig instead?

@@ -23,6 +24,10 @@ func (o *StorageOptions) Validate() []error {
return nil
}

if o.Name == memorystorage.StorageName {
Copy link
Member

Choose a reason for hiding this comment

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

In the storage definition, try not to couple with the concrete storage implementation, we can determine whether o.ConfigPath == ”“ or ConfigPath type is *string.

Copy link
Member

@duanmengkk duanmengkk Sep 9, 2022

Choose a reason for hiding this comment

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

Do you mean?

if o.ConfigPath == "" {
	return nil
}

I used to think so.But if storage is mysql,and users do not config configpath --storage-config,It will cause error later

Copy link
Member

Choose a reason for hiding this comment

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

I used to think so.But if storage is mysql,and users do not config configpath --storage-config,It will cause error later

We can check if the configPath is empty in internalStorage.NewStorageFactory and let the storage layer decide if the configPath is needed.

Copy link
Member

Choose a reason for hiding this comment

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

ok,got it

@@ -42,4 +43,7 @@ type ResourceStorageConfig struct {
Codec runtime.Codec
StorageVersion schema.GroupVersion
MemoryVersion schema.GroupVersion
WatchCache *watchcache.WatchCache
Copy link
Member

Choose a reason for hiding this comment

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

An interface can be defined to avoid dependencies on the implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean add a new interface in ResourceStorage like this ?

type ResourceStorage interface {
	GetStorageConfig() *ResourceStorageConfig

	GetWatchCache() *watchcache.WatchCache

	Get(ctx context.Context, cluster, namespace, name string, obj runtime.Object) error
	List(ctx context.Context, listObj runtime.Object, opts *internal.ListOptions) error

	Create(ctx context.Context, cluster string, obj runtime.Object) error
	Update(ctx context.Context, cluster string, obj runtime.Object) error
	Delete(ctx context.Context, cluster string, obj runtime.Object) error
}

Copy link
Member

Choose a reason for hiding this comment

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

I'm looking at the memory storage implementation, and config as a static structure really shouldn't be used to feed information.

type ResourceStorage interface {
	GetStorageConfig() *ResourceStorageConfig

	GetWatchCache() *watchcache.WatchCache

	Get(ctx context.Context, cluster, namespace, name string, obj runtime.Object) error
	List(ctx context.Context, listObj runtime.Object, opts *internal.ListOptions) error

	Create(ctx context.Context, cluster string, obj runtime.Object) error
	Update(ctx context.Context, cluster string, obj runtime.Object) error
	Delete(ctx context.Context, cluster string, obj runtime.Object) error
}

This looks better, and it is recommended not to rely on *watchcache.WatchCache, but to abstract an interface such as Watcher

Copy link
Contributor Author

Choose a reason for hiding this comment

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

type ResourceStorage interface {
	GetStorageConfig() *ResourceStorageConfig

	GetWatchCache() *watchcache.WatchCache

	Get(ctx context.Context, cluster, namespace, name string, obj runtime.Object) error
	List(ctx context.Context, listObj runtime.Object, opts *internal.ListOptions) error

	Create(ctx context.Context, cluster string, obj runtime.Object) error
	Update(ctx context.Context, cluster string, obj runtime.Object) error
	Delete(ctx context.Context, cluster string, obj runtime.Object) error
}

GetWatchCache() is only for memory storage, is that ok for other storages?

Or we just do like this, Watcher is an interface.

type ResourceStorageConfig struct {
	GroupResource        schema.GroupResource
	StorageGroupResource schema.GroupResource

	Codec          runtime.Codec
	StorageVersion schema.GroupVersion
	MemoryVersion  schema.GroupVersion
	WatchCache     Watcher
	Cluster        string
	Namespaced     bool
}

Copy link
Member

@Iceber Iceber Sep 13, 2022

Choose a reason for hiding this comment

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

Don't let the caller get additional information from the configuration passed to the function again,config is just an input for passing some configuration information to the function,and is not a stateful structure

GetWatchCache() is only for memory storage, is that ok for other storages?

Other storage layers may also implement watch functions, GetWatchCache/GetWatcher/Watcher should be a general method of the Resource Storage interface, whether or not to implement it is up to the specific storage layer.
eg.

type ResourceStorage interface {
	GetStorageConfig() *ResourceStorageConfig

	GetWatcher() ResourceWatcher

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about just add watch func, like get and list?

type ResourceStorage interface {
	GetStorageConfig() *ResourceStorageConfig

	Get(ctx context.Context, cluster, namespace, name string, obj runtime.Object) error
	List(ctx context.Context, listObj runtime.Object, opts *internal.ListOptions) error
	Watch(ctx context.Context, options *metainternalversion.ListOptions) (watch.Interface, error)

	Create(ctx context.Context, cluster string, obj runtime.Object) error
	Update(ctx context.Context, cluster string, obj runtime.Object) error
	Delete(ctx context.Context, cluster string, obj runtime.Object) error
}

Copy link
Member

Choose a reason for hiding this comment

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

This way looks better

@Iceber Iceber self-requested a review September 8, 2022 06:36
@Iceber Iceber added this to the v0.6.0 milestone Sep 8, 2022
@wuyingjun-lucky wuyingjun-lucky force-pushed the memory-storage-1 branch 2 times, most recently from efc6370 to 33f0745 Compare September 9, 2022 06:32
@kerthcet
Copy link
Contributor

kerthcet commented Sep 9, 2022

/cc

resourceStorage := &ResourceStorage{
incoming: make(chan ClusterWatchEvent, 100),
Codec: config.Codec,
crvSynchro: cache.NewClusterResourceVersionSynchro(config.Cluster),
Copy link
Member

Choose a reason for hiding this comment

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

Does the initialization of resource storage necessarily need to be bound to a cluster name? I think resource storage is cluster free and then when ready to watch, a watcher is initialized based on the set of clusters passed in, what do you think?

If the resource storage is cluster bound, it would be strange to use a resource storage with a bound cluster to query multiple cluster resources.

Copy link
Member

@duanmengkk duanmengkk Sep 9, 2022

Choose a reason for hiding this comment

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

ClusterResourceVersionSynchro is a struct hold the cluster and the latest resourceVersion of event of this cluster.We encode a event with the latest resourceVersion of all clusters,so it is necessary.

storages *resourceStorages is a struct which hold the all resources stores and latest events in all clusters. The methd of NewResourceStorage will excute many times in multiple pediacluster reconcile logic and finally Initialize storages *resourceStorages .The implement logic of NewResourceStorage is also strange for us. We have discussed and have no ideas. Hope for any other good suggestions.

Copy link
Member

@Iceber Iceber Sep 9, 2022

Choose a reason for hiding this comment

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

There is an important concept that resource storage is cluster-independent in APIServer because it needs to support querying an arbitrary set of clusters.

Can we inertly handle cluster information when creating resources, e.g. inertly initializing indexer wc.AddIndexer(newCluster, nil)

Copy link
Member

@Iceber Iceber Sep 9, 2022

Choose a reason for hiding this comment

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

If there must be some cluster-specific initialization, I think it might be possible to add a PrepareCluster (or some other name) method to the StorageFactory to do some special operations on clusters, and also to correspond to CleanCluster.

It is important to keep the Resource Storage Interface independent of the specific cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that it can't be solved by inertly way. I prefer adding a PrepareCluster method to the StorageFactory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

type StorageFactory interface {
	GetResourceVersions(ctx context.Context, cluster string) (map[schema.GroupVersionResource]map[string]interface{}, error)
	PrepareCluster(cluster string, gvr schema.GroupVersionResource) error
	CleanCluster(ctx context.Context, cluster string) error

@Iceber
Copy link
Member

Iceber commented Sep 9, 2022

This memory storage looks like it implements the storage interface for the watch cache, and while one of the purposes of implementing memory is to implement watch, that's not the only purpose.

It might be clearer for the memory storage to implement the storage interface first, and then implement the watch capability, and it might be better to extend the filtering capabilities of memory for retrieval.
This seems to be the opposite idea of the current implementation.

This is just my opinion

@duanmengkk
Copy link
Member

This memory storage looks like it implements the storage interface for the watch cache, and while one of the purposes of implementing memory is to implement watch, that's not the only purpose.

It might be clearer for the memory storage to implement the storage interface first, and then implement the watch capability, and it might be better to extend the filtering capabilities of memory for retrieval. This seems to be the opposite idea of the current implementation.

This is just my opinion

Agree with you. We will start other prs to implement more capabilities of memory for retrieval.Currently we need watch for using of actual production environment

@wuyingjun-lucky
Copy link
Member

This memory storage looks like it implements the storage interface for the watch cache, and while one of the purposes of implementing memory is to implement watch, that's not the only purpose.

It might be clearer for the memory storage to implement the storage interface first, and then implement the watch capability, and it might be better to extend the filtering capabilities of memory for retrieval. This seems to be the opposite idea of the current implementation.

This is just my opinion

WatchCache saves the data into memory and helps us to implements memory-storage-layer. It is not just for client to watch. The pr is including storage-layer implements and some native query provided by k8s like get list and watch.
We will discuss next weekdays on whether to implements filtering capabilities on this pr or next prs.

@xyz2277
Copy link
Contributor Author

xyz2277 commented Sep 15, 2022

@Iceber a new commit has pushed, please review.
By the way, please approving running workflows~

wuyingjun-lucky and others added 6 commits September 16, 2022 10:02
Co-authored-by: duanmeng <duanmeng_yewu@cmss.chinamobile.com>
Co-authored-by: hanweisen <hanweisen_yewu@cmss.chinamobile.com>
Co-authored-by: zhangyongxi <zhangyongxi_yewu@cmss.chinamobile.com>
Signed-off-by: wuyingjun <wuyingjun_yewu@cmss.chinamobile.com>
Co-authored-by: hanweisen <hanweisen_yewu@cmss.chinamobile.com>
Co-authored-by: wuyingjun <wuyingjun_yewu@cmss.chinamobile.com>
Co-authored-by: zhangyongxi <zhangyongxi_yewu@cmss.chinamobile.com>
Signed-off-by: duanmeng <duanmeng_yewu@cmss.chinamobile.com>
Co-authored-by: duanmeng <duanmeng_yewu@cmss.chinamobile.com>
Co-authored-by: hanweisen <hanweisen_yewu@cmss.chinamobile.com>
Co-authored-by: wuyingjun <wuyingjun_yewu@cmss.chinamobile.com>
Signed-off-by: zhangyongxi <zhangyongxi_yewu@cmss.chinamobile.com>
Co-authored-by: hanweisen <hanweisen_yewu@cmss.chinamobile.com>
Co-authored-by: wuyingjun <wuyingjun_yewu@cmss.chinamobile.com>
Co-authored-by: zhangyongxi <zhangyongxi_yewu@cmss.chinamobile.com>
Signed-off-by: duanmeng <duanmeng_yewu@cmss.chinamobile.com>
Co-authored-by: duanmeng <duanmeng_yewu@cmss.chinamobile.com>
Co-authored-by: hanweisen <hanweisen_yewu@cmss.chinamobile.com>
Co-authored-by: zhangyongxi <zhangyongxi_yewu@cmss.chinamobile.com>
Signed-off-by: wuyingjun <wuyingjun_yewu@cmss.chinamobile.com>
Co-authored-by: duanmeng <duanmeng_yewu@cmss.chinamobile.com>
Co-authored-by: wuyingjun <wuyingjun_yewu@cmss.chinamobile.com>
Co-authored-by: hanweisen <hanweisen_yewu@cmss.chinamobile.com>
Signed-off-by: zhangyongxi <zhangyongxi_yewu@cmss.chinamobile.com>
@wuyingjun-lucky
Copy link
Member

@Iceber Ready to be reviewed in second round

@Iceber
Copy link
Member

Iceber commented Sep 19, 2022

@wuyingjun-lucky Okay, I will review as soon as possible

}
if config.StorageFactory == nil {
return nil, fmt.Errorf("CompletedConfig.New() called with config.StorageFactory == nil")
}
Copy link
Member

Choose a reason for hiding this comment

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

This is necessary because if these two fields are not set in config, the caller will panic.
Also, the fields in Config are public fields and it cannot be assumed that the caller has set them


resourceStorage, err := s.storage.NewResourceStorage(config.storageConfig)
resourceStorage, err := s.storage.NewResourceStorage(resourceConfig)
Copy link
Member

Choose a reason for hiding this comment

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

Because it is not used elsewhere, there is no need to assign additional values to variables

if err != nil {
syncCondition.Reason = "SynchroCreateFailed"
syncCondition.Message = fmt.Sprintf("new resource storage config failed: %s", err)
groupResourceStatus.addSyncCondition(syncGVR, syncCondition)
continue
}

storageConfig.Namespaced = apiResource.Namespaced
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to pass Namespaced into negotiator.resourceStorageConfig.NewConfig, since namespaced is also required to generate storageconfig in kubeapiserver.

func (m *RESTManager) genCustomResourceRESTStorage(gvr schema.GroupVersionResource, kind string) (*resourcerest.RESTStorage, error) {
storageConfig, err := m.resourcetSorageConfig.NewCustomResourceConfig(gvr)

func (m *RESTManager) genLegacyResourceRESTStorage(gvr schema.GroupVersionResource, kind string) (*resourcerest.RESTStorage, error) {
storageConfig, err := m.resourcetSorageConfig.NewLegacyResourceConfig(gvr.GroupResource())

And namespaced is a required field in StorageConfig, as a parameter to StorageConfigFactory.New*, to prevent forgetting to set

@Iceber
Copy link
Member

Iceber commented Sep 19, 2022

Everything else looks good 👍🏻, and the we can continue to iterate over the implementation of the memory store.

xyz2277 added a commit to xyz2277/clusterpedia that referenced this pull request Sep 19, 2022
Co-authored-by: duanmeng <duanmeng_yewu@cmss.chinamobile.com>
Co-authored-by: wuyingjun <wuyingjun_yewu@cmss.chinamobile.com>
Co-authored-by: hanweisen <hanweisen_yewu@cmss.chinamobile.com>
Signed-off-by: zhangyongxi <zhangyongxi_yewu@cmss.chinamobile.com>
@xyz2277
Copy link
Contributor Author

xyz2277 commented Sep 19, 2022

@Iceber Ready to be reviewed in third round, and please approve running workflows~

@@ -291,7 +291,7 @@ func (m *RESTManager) genLegacyResourceRESTStorage(gvr schema.GroupVersionResour
}

func (m *RESTManager) genCustomResourceRESTStorage(gvr schema.GroupVersionResource, kind string) (*resourcerest.RESTStorage, error) {
storageConfig, err := m.resourcetSorageConfig.NewCustomResourceConfig(gvr)
storageConfig, err := m.resourcetSorageConfig.NewCustomResourceConfig(gvr, false)
Copy link
Member

@Iceber Iceber Sep 19, 2022

Choose a reason for hiding this comment

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

The caller of the genCustomResourceRESTStorage/genLegacyResourceRESTStorage know if the resource is namespaced.

if resourcescheme.LegacyResourceScheme.IsGroupRegistered(gvr.Group) {
storage, err = m.genLegacyResourceRESTStorage(gvr, info.APIResource.Kind)
} else {
storage, err = m.genCustomResourceRESTStorage(gvr, info.APIResource.Kind)

 	storage, err = m.genLegacyResourceRESTStorage(gvr, info.APIResource.Kind, infor.APIResource.Namespaced) 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The caller of the genCustomResourceRESTStorage/genLegacyResourceRESTStorage know if the resource is namespaced.

if resourcescheme.LegacyResourceScheme.IsGroupRegistered(gvr.Group) {
storage, err = m.genLegacyResourceRESTStorage(gvr, info.APIResource.Kind)
} else {
storage, err = m.genCustomResourceRESTStorage(gvr, info.APIResource.Kind)

 	storage, err = m.genLegacyResourceRESTStorage(gvr, info.APIResource.Kind, infor.APIResource.Namespaced) 

thx, it's been fixed, please approve running workflows again~

xyz2277 added a commit to xyz2277/clusterpedia that referenced this pull request Sep 19, 2022
Co-authored-by: duanmeng <duanmeng_yewu@cmss.chinamobile.com>
Co-authored-by: wuyingjun <wuyingjun_yewu@cmss.chinamobile.com>
Co-authored-by: hanweisen <hanweisen_yewu@cmss.chinamobile.com>
Signed-off-by: zhangyongxi <zhangyongxi_yewu@cmss.chinamobile.com>
Copy link
Member

@Iceber Iceber left a comment

Choose a reason for hiding this comment

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

LGTM!

@xyz2277 @wuyingjun-lucky you can ping me when the commits are ready.

Co-authored-by: duanmeng <duanmeng_yewu@cmss.chinamobile.com>
Co-authored-by: wuyingjun <wuyingjun_yewu@cmss.chinamobile.com>
Co-authored-by: hanweisen <hanweisen_yewu@cmss.chinamobile.com>
Signed-off-by: zhangyongxi <zhangyongxi_yewu@cmss.chinamobile.com>
@wuyingjun-lucky
Copy link
Member

LGTM!

@xyz2277 @wuyingjun-lucky you can ping me when the commits are ready.

ping @Iceber

@Iceber
Copy link
Member

Iceber commented Sep 19, 2022

/lgtm

@clusterpedia-bot clusterpedia-bot added the lgtm Look good to me label Sep 19, 2022
@Iceber Iceber merged commit 6ec48f5 into clusterpedia-io:main Sep 19, 2022
@qmloong
Copy link

qmloong commented Oct 24, 2022

@Iceber @xyz2277 I am very interested in this watch feature. Where can I get more details about this design proposal in addition to #265?

@wuyingjun-lucky
Copy link
Member

/cc @xyz2277

@wuyingjun-lucky
Copy link
Member

wuyingjun-lucky commented Oct 24, 2022

@Iceber @xyz2277 I am very interested in this watch feature. Where can I get more details about this design proposal in addition to #265?

We have introduced this feature to our production. We redefined the multi-cluster resource version to client,and we can watch the event from multi-cluster one by one. The storage-layer is memory. If you are interested on the feature you can use this storage-layer to run clusterpedia. @Iceber is our maintainer. Maybe you can connect him and join our wechat group.

@qmloong
Copy link

qmloong commented Oct 24, 2022

@Iceber OK, thanks. Where can I get the QR code of the WeChat group?

@qmloong
Copy link

qmloong commented Oct 24, 2022

Sorry, I forget that my wechat group chatting function has been banned until October 28 for some irresistible reasons...
Let me take a look at the code base first and use slack to communicate. :-(

@wuyingjun-lucky
Copy link
Member

communicate

If you have any question,you can make a issue or pr to record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Look good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce watch into clusterpedia
7 participants