From 34d0e20acc8fa6689e89605957a42ad05c7c5858 Mon Sep 17 00:00:00 2001 From: Ales Stimec Date: Wed, 12 Jun 2024 11:52:31 +0200 Subject: [PATCH] Addressing review comments. --- internal/jujuapi/websocket.go | 1 - internal/jujuclient/dial.go | 4 ++-- internal/rpc/proxy.go | 21 +++++++-------------- internal/rpc/proxy_test.go | 1 - internal/servermon/monitoring.go | 4 ++-- 5 files changed, 11 insertions(+), 20 deletions(-) diff --git a/internal/jujuapi/websocket.go b/internal/jujuapi/websocket.go index 6e28a241e..bd529648f 100644 --- a/internal/jujuapi/websocket.go +++ b/internal/jujuapi/websocket.go @@ -191,7 +191,6 @@ func controllerConnectionFunc(s modelProxyServer, jwtGenerator *jimm.JWTGenerato return jimmRPC.WebsocketConnectionWithMetadata{ Conn: controllerConn, ControllerUUID: m.Controller.UUID, - ModelUUID: m.UUID.String, ModelName: fullModelName, }, nil } diff --git a/internal/jujuclient/dial.go b/internal/jujuclient/dial.go index 683c7a067..923596ee1 100644 --- a/internal/jujuclient/dial.go +++ b/internal/jujuclient/dial.go @@ -236,9 +236,9 @@ func (c *Connection) redial(ctx context.Context, requiredPermissions map[string] // the server and waits for the response to be returned or the context to // be canceled. func (c *Connection) Call(ctx context.Context, facade string, version int, id, method string, args, resp interface{}) (err error) { - labels := []string{facade, method, "", c.mt.Id()} + labels := []string{facade, method, ""} if c.ctl != nil { - labels = []string{facade, method, c.ctl.UUID, c.mt.Id()} + labels = []string{facade, method, c.ctl.UUID} } durationObserver := servermon.DurationObserver(servermon.JujuCallDurationHistogram, labels...) defer durationObserver() diff --git a/internal/rpc/proxy.go b/internal/rpc/proxy.go index 3cfb6bae4..7e280725f 100644 --- a/internal/rpc/proxy.go +++ b/internal/rpc/proxy.go @@ -56,7 +56,6 @@ type WebsocketConnection interface { type WebsocketConnectionWithMetadata struct { Conn WebsocketConnection ControllerUUID string - ModelUUID string ModelName string } @@ -170,8 +169,7 @@ func (c *writeLockConn) sendMessage(responseObject any, request *message) { } type inflightMsgs struct { - controlerUUID string - modelUUID string + controllerUUID string mu sync.Mutex loginMessage *message @@ -205,8 +203,7 @@ func (msgs *inflightMsgs) removeMessage(msg *message) { servermon.JujuCallDurationHistogram.WithLabelValues( msg.Type, msg.Request, - msgs.controlerUUID, - msgs.modelUUID, + msgs.controllerUUID, ).Observe(time.Since(msg.start).Seconds()) msgs.mu.Lock() @@ -248,7 +245,7 @@ func (p *modelProxy) sendError(socket *writeLockConn, req *message, err error) { socket.writeJson(msg) } // An error message is a response back to the client. - servermon.JujuCallErrorCount.WithLabelValues(req.Type, req.Request, p.msgs.controlerUUID, p.msgs.modelUUID) + servermon.JujuCallErrorCount.WithLabelValues(req.Type, req.Request, p.msgs.controllerUUID) p.auditLogMessage(msg, true) } @@ -316,7 +313,6 @@ func (p *clientProxy) start(ctx context.Context) error { p.dst.conn.Close() } }() - for { zapctx.Debug(ctx, "Reading on client connection") msg := new(message) @@ -324,7 +320,6 @@ func (p *clientProxy) start(ctx context.Context) error { // Error reading on the socket implies it is closed, simply return. return err } - zapctx.Debug(ctx, "Read message from client", zap.Any("message", msg)) err := p.makeControllerConnection(ctx) if err != nil { @@ -340,7 +335,7 @@ func (p *clientProxy) start(ctx context.Context) error { toClient, toController, err := p.handleAdminFacade(ctx, msg) if err != nil { p.sendError(p.src, msg, err) - return nil + continue } if toClient != nil { p.src.sendMessage(nil, toClient) @@ -353,16 +348,15 @@ func (p *clientProxy) start(ctx context.Context) error { zapctx.Error(ctx, "Invalid request ID 0") err := errors.E(op, "Invalid request ID 0") p.sendError(p.src, msg, err) - return nil + continue } p.msgs.addMessage(msg) zapctx.Debug(ctx, "Writing to controller") - if err := p.dst.writeJson(msg); err != nil { zapctx.Error(ctx, "clientProxy error writing to dst", zap.Error(err)) p.sendError(p.src, msg, err) p.msgs.removeMessage(msg) - return nil + continue } } } @@ -386,8 +380,7 @@ func (p *clientProxy) makeControllerConnection(ctx context.Context) error { return err } - p.msgs.controlerUUID = connWithMetadata.ControllerUUID - p.msgs.modelUUID = connWithMetadata.ModelUUID + p.msgs.controllerUUID = connWithMetadata.ControllerUUID p.modelName = connWithMetadata.ModelName p.dst = &writeLockConn{conn: connWithMetadata.Conn} diff --git a/internal/rpc/proxy_test.go b/internal/rpc/proxy_test.go index 8c464cb65..4410fbf3f 100644 --- a/internal/rpc/proxy_test.go +++ b/internal/rpc/proxy_test.go @@ -254,7 +254,6 @@ func TestProxySocketsAdminFacade(t *testing.T) { return rpc.WebsocketConnectionWithMetadata{ Conn: controllerWebsocket, ModelName: "test model", - ModelUUID: uuid.NewString(), ControllerUUID: uuid.NewString(), }, nil }, diff --git a/internal/servermon/monitoring.go b/internal/servermon/monitoring.go index f8aec1e91..4f8c868d1 100644 --- a/internal/servermon/monitoring.go +++ b/internal/servermon/monitoring.go @@ -69,13 +69,13 @@ var ( Name: "call_duration_seconds", Help: "Histogram of juju call time in seconds", Buckets: []float64{.005, .01, .025, .05, .1, .25, .5, 1, 2.5, 5, 10}, - }, []string{"facade", "method", "controller", "model"}) + }, []string{"facade", "method", "controller"}) JujuCallErrorCount = promauto.NewCounterVec(prometheus.CounterOpts{ Namespace: "jimm", Subsystem: "juju", Name: "error_total", Help: "The number of juju call errors.", - }, []string{"facade", "method", "controller", "model"}) + }, []string{"facade", "method", "controller"}) ConcurrentWebsocketConnections = promauto.NewGauge(prometheus.GaugeOpts{ Namespace: "jimm", Subsystem: "websocket",