kuma-cp: add a Monitoring Assignment Discovery Service (MADS) server #531
Conversation
yskopets
force-pushed
the
feature/mads-server
branch
from
3ad7015 to
5781ef6
Compare
pkg/config/mads/config.go
Outdated
| // Monitoring Assignment Discovery Service (MADS) server configuration. | ||
| type MonitoringAssignmentServerConfig struct { | ||
| // Port of a gRPC server that serves Monitoring Assignment Discovery Service (MADS). | ||
| GrpcPort int `yaml:"grpcPort" envconfig:"kuma_observability_monitoring_assignment_server_grpc_port"` |
There was a problem hiding this comment.
env name should mirror the yaml path. YAML path right now is monitoringAssignmentServer.grpcPort.
Please also update loader_test.go file with overrides, defaults and test for /config
pkg/mads/server/components.go
Outdated
| return time.NewTicker(1 * time.Second) | ||
| }, | ||
| OnTick: func() error { | ||
| madsServerLog.V(1).Info("on tick") |
There was a problem hiding this comment.
Can we log for what node and stream id the timer is ticking?
pkg/mads/server/components.go
Outdated
| return util_xds.NewWatchdogCallbacks(func(node *envoy_core.Node, _ int64) (util_watchdog.Watchdog, error) { | ||
| return &util_watchdog.SimpleWatchdog{ | ||
| NewTicker: func() *time.Ticker { | ||
| return time.NewTicker(1 * time.Second) |
There was a problem hiding this comment.
Can we make this configurable? It's an easy way to improve performance of the Control Plane. Example - change 1s to 10s, 10x less operation to execute with more latency for applying policies which is not crucial here.
| # Monitoring Assignment Discovery Service (MADS) server configuration | ||
| monitoringAssignmentServer: | ||
| # Port of a gRPC server that serves Monitoring Assignment Discovery Service (MADS). | ||
| grpcPort: 5676 |
There was a problem hiding this comment.
can you add ENV: comment?
pkg/mads/reconcile/interfaces.go
Outdated
| } | ||
|
|
||
| // Generates a snapshot of xDS resources for a given node. | ||
| type Snapshotter interface { |
There was a problem hiding this comment.
How about SnapshotGenerator?
| envoy_server "github.com/envoyproxy/go-control-plane/pkg/server" | ||
| ) | ||
|
|
||
| type Server interface { |
There was a problem hiding this comment.
I see this code is very similar to SDS. Can we somehow avoid duplication? Of course, if it doesn't make sense then don't do it. Then please at least provide link on which code this implementation is based on.
There was a problem hiding this comment.
I don't want to invest time into this generalisation right now
pkg/mads/reconcile/reconciler.go
Outdated
| } | ||
|
|
||
| func (r *reconciler) Reconcile(node *envoy_core.Node) error { | ||
| new, err := r.snapshotter.Snapshot(node) |
There was a problem hiding this comment.
shouldn't we call Snapshot#Consistent somewhere?
pkg/mads/reconcile/reconciler.go
Outdated
| } | ||
| id := r.hasher.ID(node) | ||
| old, _ := r.cache.GetSnapshot(id) | ||
| r.versioner.Version(new, old) |
There was a problem hiding this comment.
What about concurrency of this method? Hasher will always return the same version. Every node has one goroutine for processing so there will be multiple goroutines operating on the same snapshot setting the version. Is it ok?
There was a problem hiding this comment.
The new is always a fresh object.
Anyway, Versioner has been changed to facilitate immutable Snapshots.
There was a problem hiding this comment.
I was mostly concerned about versioning of the snapshot, but since we compare it for previous snapshot, it should be fine.
yskopets
force-pushed
the
feature/generic-xds-stream-watchdog
branch
from
b58f1b1 to
863b8d7
Compare
yskopets
force-pushed
the
feature/mads-server
branch
from
5781ef6 to
c11f6a3
Compare
|
@jakubdyszkiewicz Please take another look |
yskopets
force-pushed
the
feature/generic-xds-stream-watchdog
branch
from
863b8d7 to
8bf29a7
Compare
yskopets
changed the base branch from
feature/generic-xds-stream-watchdog
to
master
yskopets
force-pushed
the
feature/mads-server
branch
from
c11f6a3 to
e6fa93b
Compare
Summary