M2GOLD-12 Support external cluster registry in voyager aggregator - #149
M2GOLD-12 Support external cluster registry in voyager aggregator#149nickhiggs wants to merge 2 commits into
Conversation
239e1c2 to
c401960
Compare
| --config "$(CURDIR)/build/local/aggregator/config.yaml" \ | ||
| --client-config-from=file \ | ||
| --client-config-file-name="$$HOME/.kube/config" \ | ||
| --kubeconfig="$$HOME/.kube/config" \ |
There was a problem hiding this comment.
- Two config parameters?
- Use
KUBECONFIGlike above
There was a problem hiding this comment.
From memory this was because different apiserver config components require different config flags when not running in-cluster
| --kubeconfig="$$HOME/.kube/config" \ | ||
| --client-config-context=$(VALIDATED_KUBE_CONTEXT) \ | ||
| --secure-port=6443 \ | ||
| --local=true \ |
There was a problem hiding this comment.
Can we move as many flags as possible into the config file?
| } | ||
| if initializers, err := opts.ExtraAdmissionInitializers(config); err != nil { | ||
| return err | ||
| } else if err := opts.Admission.ApplyTo(&config.Config, config.SharedInformerFactory, config.ClientConfig, scheme, initializers...); err != nil { |
There was a problem hiding this comment.
Allows reuse of the initializers which is only valid in this context
|
|
||
| type ExternalClusterRegistry struct { | ||
| namespace string | ||
| rest *clusters.ClusterregistryV1alpha1Client |
There was a problem hiding this comment.
There should be an interface for it
| clusters := map[string]interface{}{} | ||
|
|
||
| for _, c := range clusterList.Items { | ||
| if label, ok := c.Labels["customer"]; ok && label == "paas" { |
There was a problem hiding this comment.
Shouldn't be hardcoded. Use k8s.io/apimachinery/pkg/labels package to abstract the details away. Put k8s.io/apimachinery/pkg/labels.Selector as a field and use it here via .Matches(). Configure the labels for it from config file.
| } | ||
|
|
||
| func (e *ExternalClusterRegistry) GetClusters() ([]*cr_v1a1.Cluster, error) { | ||
| if time.Since(e.lastRun) > e.resyncPeriod { |
There was a problem hiding this comment.
ExternalClusterRegistryis supposed to be thread-safe, right? Currently it is not becausee.lastRunis read and written without any synchronization means.- Should likely avoid making multiple concurrent calls to the registry because why if you can do a single call and then serve data to multiple callers.
- Since you are maintaining a cache here, why not make it asynchronous and decouple reads from cache refreshes? Have a goroutine that is periodically refreshing the cache and always serve data from it? Can even use watch if it is supported to refresh data in the cache. How sensitive are we to stale information in that cache?
|
|
||
| clusters := e.cache.List() | ||
|
|
||
| typedClusters := make([]*cr_v1a1.Cluster, 0) |
| } | ||
|
|
||
| func (c *ClusterInformer) GetClusters() ([]*cr_v1a1.Cluster, error) { | ||
| clusters, err := c.informer.GetIndexer().ByIndex(ByClusterLabelIndexName, "paas") |
There was a problem hiding this comment.
paas should not be hardcoded, move to config file
| informer cache.SharedIndexInformer | ||
| } | ||
|
|
||
| func NewClusterInformer(config *ctrl.Config, cctx *ctrl.Context) (*ClusterInformer, error) { |
There was a problem hiding this comment.
Ideally this package should not depend on ctrl. All the plumbing logic should be in the controller constructor, like in all other controllers.
|
|
||
| rest, err := clusters.NewForConfig(cfg) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Errors coming from external things need to be wrapped with errors.WithStack in many places (so when things go wrong we get a stack trace thanks golang)
3 participants
Threw together support for an external cluster registry stored in S3 or similar. Also added in some local config for the aggregator so it can be run outside a cluster for local dev.