Merge pull request #3556 from overleaf/jpa-clsi-persistance-query-param

[CompileController] enable clsi node persistence via query parameter

GitOrigin-RevId: 515814d6ad5832e69538ef6d63f81c61c66fd73f

services/web/app/src/Features/Compile/ClsiManager.js

@@ -136,7 +136,7 @@ const ClsiManager = {
     ClsiManager._makeRequest(projectId, opts, callback)
   },
 
-  deleteAuxFiles(projectId, userId, options, callback) {
+  deleteAuxFiles(projectId, userId, options, clsiserverid, callback) {
     if (options == null) {
       options = {}
     }
@@ -149,27 +149,32 @@ const ClsiManager = {
       url: compilerUrl,
       method: 'DELETE'
     }
-    ClsiManager._makeRequest(projectId, opts, clsiErr => {
-      // always clear the project state from the docupdater, even if there
-      // was a problem with the request to the clsi
-      DocumentUpdaterHandler.clearProjectState(projectId, docUpdaterErr => {
-        if (clsiErr != null) {
-          return callback(
-            OError.tag(clsiErr, 'Failed to delete aux files', { projectId })
-          )
-        }
-        if (docUpdaterErr != null) {
-          return callback(
-            OError.tag(
-              docUpdaterErr,
-              'Failed to clear project state in doc updater',
-              { projectId }
+    ClsiManager._makeRequestWithClsiServerId(
+      projectId,
+      opts,
+      clsiserverid,
+      clsiErr => {
+        // always clear the project state from the docupdater, even if there
+        // was a problem with the request to the clsi
+        DocumentUpdaterHandler.clearProjectState(projectId, docUpdaterErr => {
+          if (clsiErr != null) {
+            return callback(
+              OError.tag(clsiErr, 'Failed to delete aux files', { projectId })
             )
-          )
-        }
-        callback()
-      })
-    })
+          }
+          if (docUpdaterErr != null) {
+            return callback(
+              OError.tag(
+                docUpdaterErr,
+                'Failed to clear project state in doc updater',
+                { projectId }
+              )
+            )
+          }
+          callback()
+        })
+      }
+    )
   },
 
   _sendBuiltRequest(projectId, userId, req, options, callback) {
@@ -253,6 +258,23 @@ const ClsiManager = {
     )
   },
 
+  _makeRequestWithClsiServerId(projectId, opts, clsiserverid, callback) {
+    if (clsiserverid) {
+      // ignore cookies and newBackend, go straight to the clsi node
+      opts.qs = Object.assign({ clsiserverid }, opts.qs)
+      request(opts, (err, response, body) => {
+        if (err) {
+          return callback(
+            OError.tag(err, 'error making request to CLSI', { projectId })
+          )
+        }
+        callback(null, response, body)
+      })
+    } else {
+      ClsiManager._makeRequest(projectId, opts, callback)
+    }
+  },
+
   _makeRequest(projectId, opts, callback) {
     async.series(
       {
@@ -787,7 +809,7 @@ const ClsiManager = {
     })
   },
 
-  wordCount(projectId, userId, file, options, callback) {
+  wordCount(projectId, userId, file, options, clsiserverid, callback) {
     ClsiManager._buildRequest(projectId, options, (err, req) => {
       if (err != null) {
         return callback(
@@ -812,25 +834,32 @@ const ClsiManager = {
         },
         method: 'GET'
       }
-      ClsiManager._makeRequest(projectId, opts, (err, response, body) => {
-        if (err != null) {
-          return callback(OError.tag(err, 'CLSI request failed', { projectId }))
-        }
-        if (response.statusCode >= 200 && response.statusCode < 300) {
-          callback(null, body)
-        } else {
-          callback(
-            new OError(
-              `CLSI returned non-success code: ${response.statusCode}`,
-              {
-                projectId,
-                clsiResponse: body,
-                statusCode: response.statusCode
-              }
+      ClsiManager._makeRequestWithClsiServerId(
+        projectId,
+        opts,
+        clsiserverid,
+        (err, response, body) => {
+          if (err != null) {
+            return callback(
+              OError.tag(err, 'CLSI request failed', { projectId })
             )
-          )
+          }
+          if (response.statusCode >= 200 && response.statusCode < 300) {
+            callback(null, body)
+          } else {
+            callback(
+              new OError(
+                `CLSI returned non-success code: ${response.statusCode}`,
+                {
+                  projectId,
+                  clsiResponse: body,
+                  statusCode: response.statusCode
+                }
+              )
+            )
+          }
         }
-      })
+      )
     })
   }
 }

services/web/app/src/Features/Compile/CompileController.js

@@ -255,11 +255,12 @@ module.exports = CompileController = {
 
   deleteAuxFiles(req, res, next) {
     const project_id = req.params.Project_id
+    const { clsiserverid } = req.query
     return CompileController._compileAsUser(req, function(error, user_id) {
       if (error != null) {
         return next(error)
       }
-      return CompileManager.deleteAuxFiles(project_id, user_id, function(
+      CompileManager.deleteAuxFiles(project_id, user_id, clsiserverid, function(
         error
       ) {
         if (error != null) {
@@ -465,7 +466,7 @@ module.exports = CompileController = {
     if (next == null) {
       next = function(error) {}
     }
-    return ClsiCookieManager.getCookieJar(project_id, function(err, jar) {
+    _getPersistenceOptions(req, project_id, (err, persistenceOptions) => {
       let qs
       if (err != null) {
         OError.tag(err, 'error getting cookie jar for clsi request')
@@ -479,10 +480,15 @@ module.exports = CompileController = {
       url = `${compilerUrl}${url}`
       const oneMinute = 60 * 1000
       // the base request
-      const options = { url, method: req.method, timeout: oneMinute, jar }
+      const options = {
+        url,
+        method: req.method,
+        timeout: oneMinute,
+        ...persistenceOptions
+      }
       // add any provided query string
       if (qs != null) {
-        options.qs = qs
+        options.qs = Object.assign(options.qs, qs)
       }
       // if we have a build parameter, pass it through to the clsi
       if (
@@ -518,20 +524,35 @@ module.exports = CompileController = {
   wordCount(req, res, next) {
     const project_id = req.params.Project_id
     const file = req.query.file || false
+    const { clsiserverid } = req.query
     return CompileController._compileAsUser(req, function(error, user_id) {
       if (error != null) {
         return next(error)
       }
-      return CompileManager.wordCount(project_id, user_id, file, function(
-        error,
-        body
-      ) {
-        if (error != null) {
-          return next(error)
+      CompileManager.wordCount(
+        project_id,
+        user_id,
+        file,
+        clsiserverid,
+        function(error, body) {
+          if (error != null) {
+            return next(error)
+          }
+          res.contentType('application/json')
+          return res.send(body)
         }
-        res.contentType('application/json')
-        return res.send(body)
-      })
+      )
+    })
+  }
+}
+
+function _getPersistenceOptions(req, projectId, callback) {
+  const { clsiserverid } = req.query
+  if (clsiserverid && typeof clsiserverid === 'string') {
+    callback(null, { qs: { clsiserverid } })
+  } else {
+    ClsiCookieManager.getCookieJar(projectId, (err, jar) => {
+      callback(err, { jar })
     })
   }
 }

services/web/app/src/Features/Compile/CompileManager.js

@@ -136,7 +136,7 @@ module.exports = CompileManager = {
     })
   },
 
-  deleteAuxFiles(project_id, user_id, callback) {
+  deleteAuxFiles(project_id, user_id, clsiserverid, callback) {
     if (callback == null) {
       callback = function(error) {}
     }
@@ -147,7 +147,13 @@ module.exports = CompileManager = {
       if (error != null) {
         return callback(error)
       }
-      return ClsiManager.deleteAuxFiles(project_id, user_id, limits, callback)
+      ClsiManager.deleteAuxFiles(
+        project_id,
+        user_id,
+        limits,
+        clsiserverid,
+        callback
+      )
     })
   },
 
@@ -253,7 +259,7 @@ module.exports = CompileManager = {
     })
   },
 
-  wordCount(project_id, user_id, file, callback) {
+  wordCount(project_id, user_id, file, clsiserverid, callback) {
     if (callback == null) {
       callback = function(error) {}
     }
@@ -264,7 +270,14 @@ module.exports = CompileManager = {
       if (error != null) {
         return callback(error)
       }
-      return ClsiManager.wordCount(project_id, user_id, file, limits, callback)
+      ClsiManager.wordCount(
+        project_id,
+        user_id,
+        file,
+        limits,
+        clsiserverid,
+        callback
+      )
     })
   }
 }

services/web/app/src/router.js

@@ -49,6 +49,7 @@ const TemplatesRouter = require('./Features/Templates/TemplatesRouter')
 const InstitutionsController = require('./Features/Institutions/InstitutionsController')
 const UserMembershipRouter = require('./Features/UserMembership/UserMembershipRouter')
 const SystemMessageController = require('./Features/SystemMessages/SystemMessageController')
+const { Joi, validate } = require('./infrastructure/Validation')
 
 const logger = require('logger-sharelatex')
 const _ = require('underscore')
@@ -445,6 +446,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) {
 
   webRouter.delete(
     '/project/:Project_id/output',
+    validate({ query: { clsiserverid: Joi.string() } }),
     AuthorizationMiddleware.ensureUserCanReadProject,
     CompileController.deleteAuxFiles
   )
@@ -460,6 +462,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) {
   )
   webRouter.get(
     '/project/:Project_id/wordcount',
+    validate({ query: { clsiserverid: Joi.string() } }),
     AuthorizationMiddleware.ensureUserCanReadProject,
     CompileController.wordCount
   )

services/web/test/unit/src/Compile/ClsiManagerTests.js

@@ -391,7 +391,7 @@ describe('ClsiManager', function() {
 
   describe('deleteAuxFiles', function() {
     beforeEach(function() {
-      this.ClsiManager._makeRequest = sinon.stub().callsArg(2)
+      this.ClsiManager._makeRequestWithClsiServerId = sinon.stub().yields(null)
       this.DocumentUpdaterHandler.clearProjectState = sinon.stub().callsArg(1)
     })
 
@@ -401,16 +401,21 @@ describe('ClsiManager', function() {
           this.project_id,
           this.user_id,
           { compileGroup: 'standard' },
+          'node-1',
           this.callback
         )
       })
 
       it('should call the delete method in the standard CLSI', function() {
-        this.ClsiManager._makeRequest
-          .calledWith(this.project_id, {
-            method: 'DELETE',
-            url: `${this.settings.apis.clsi.url}/project/${this.project_id}/user/${this.user_id}`
-          })
+        this.ClsiManager._makeRequestWithClsiServerId
+          .calledWith(
+            this.project_id,
+            {
+              method: 'DELETE',
+              url: `${this.settings.apis.clsi.url}/project/${this.project_id}/user/${this.user_id}`
+            },
+            'node-1'
+          )
           .should.equal(true)
       })
 
@@ -888,14 +893,9 @@ describe('ClsiManager', function() {
 
   describe('wordCount', function() {
     beforeEach(function() {
-      this.ClsiManager._makeRequest = sinon
+      this.ClsiManager._makeRequestWithClsiServerId = sinon
         .stub()
-        .callsArgWith(
-          2,
-          null,
-          { statusCode: 200 },
-          (this.body = { mock: 'foo' })
-        )
+        .yields(null, { statusCode: 200 }, (this.body = { mock: 'foo' }))
       this.ClsiManager._buildRequest = sinon.stub().callsArgWith(
         2,
         null,
@@ -912,17 +912,25 @@ describe('ClsiManager', function() {
           this.user_id,
           false,
           {},
+          'node-1',
           this.callback
         )
       })
 
       it('should call wordCount with root file', function() {
-        this.ClsiManager._makeRequest
-          .calledWith(this.project_id, {
-            method: 'GET',
-            url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount`,
-            qs: { file: 'rootfile.text', image: undefined }
-          })
+        this.ClsiManager._makeRequestWithClsiServerId
+          .calledWith(
+            this.project_id,
+            {
+              method: 'GET',
+              url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount`,
+              qs: {
+                file: 'rootfile.text',
+                image: undefined
+              }
+            },
+            'node-1'
+          )
           .should.equal(true)
       })
 
@@ -938,17 +946,22 @@ describe('ClsiManager', function() {
           this.user_id,
           'main.tex',
           {},
+          'node-2',
           this.callback
         )
       })
 
       it('should call wordCount with param file', function() {
-        this.ClsiManager._makeRequest
-          .calledWith(this.project_id, {
-            method: 'GET',
-            url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount`,
-            qs: { file: 'main.tex', image: undefined }
-          })
+        this.ClsiManager._makeRequestWithClsiServerId
+          .calledWith(
+            this.project_id,
+            {
+              method: 'GET',
+              url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount`,
+              qs: { file: 'main.tex', image: undefined }
+            },
+            'node-2'
+          )
           .should.equal(true)
       })
     })
@@ -962,17 +975,22 @@ describe('ClsiManager', function() {
           this.user_id,
           'main.tex',
           {},
+          'node-3',
           this.callback
         )
       })
 
       it('should call wordCount with file and image', function() {
-        this.ClsiManager._makeRequest
-          .calledWith(this.project_id, {
-            method: 'GET',
-            url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount`,
-            qs: { file: 'main.tex', image: this.image }
-          })
+        this.ClsiManager._makeRequestWithClsiServerId
+          .calledWith(
+            this.project_id,
+            {
+              method: 'GET',
+              url: `http://clsi.example.com/project/${this.project_id}/user/${this.user_id}/wordcount`,
+              qs: { file: 'main.tex', image: this.image }
+            },
+            'node-3'
+          )
           .should.equal(true)
       })
     })
@@ -1008,6 +1026,83 @@ describe('ClsiManager', function() {
     })
   })
 
+  describe('_makeRequestWithClsiServerId', function() {
+    beforeEach(function() {
+      this.response = { statusCode: 200 }
+      this.request.yields(null, this.response)
+      this.opts = {
+        method: 'GET',
+        url: 'http://clsi'
+      }
+    })
+
+    describe('with a regular request', function() {
+      it('should process a request with a cookie jar', function(done) {
+        this.ClsiManager._makeRequestWithClsiServerId(
+          this.project_id,
+          this.opts,
+          undefined,
+          err => {
+            if (err) return done(err)
+            const args = this.request.args[0]
+            args[0].method.should.equal(this.opts.method)
+            args[0].url.should.equal(this.opts.url)
+            args[0].jar.should.equal(this.jar)
+            expect(args[0].qs).to.not.exist
+            done()
+          }
+        )
+      })
+
+      it('should persist the cookie from the response', function(done) {
+        this.ClsiManager._makeRequestWithClsiServerId(
+          this.project_id,
+          this.opts,
+          undefined,
+          err => {
+            if (err) return done(err)
+            this.ClsiCookieManager.setServerId
+              .calledWith(this.project_id, this.response)
+              .should.equal(true)
+            done()
+          }
+        )
+      })
+    })
+
+    describe('with a persistent request', function() {
+      it('should not add a cookie jar', function(done) {
+        this.ClsiManager._makeRequestWithClsiServerId(
+          this.project_id,
+          this.opts,
+          'node-1',
+          err => {
+            if (err) return done(err)
+            const requestOpts = this.request.args[0][0]
+            expect(requestOpts.method).to.equal(this.opts.method)
+            expect(requestOpts.url).to.equal(this.opts.url)
+            expect(requestOpts.jar).to.not.exist
+            expect(requestOpts.qs).to.deep.equal({ clsiserverid: 'node-1' })
+            done()
+          }
+        )
+      })
+
+      it('should not persist a cookie on response', function(done) {
+        this.ClsiManager._makeRequestWithClsiServerId(
+          this.project_id,
+          this.opts,
+          'node-1',
+          err => {
+            if (err) return done(err)
+            expect(this.ClsiCookieManager.setServerId.called).to.equal(false)
+            done()
+          }
+        )
+      })
+    })
+  })
+
   describe('_makeGoogleCloudRequest', function() {
     beforeEach(function() {
       this.settings.apis.clsi_new = { url: 'https://compiles.somewhere.test' }

services/web/test/unit/src/Compile/CompileControllerTests.js

@@ -649,8 +649,9 @@ describe('CompileController', function() {
 
   describe('deleteAuxFiles', function() {
     beforeEach(function() {
-      this.CompileManager.deleteAuxFiles = sinon.stub().callsArg(2)
+      this.CompileManager.deleteAuxFiles = sinon.stub().yields()
       this.req.params = { Project_id: this.project_id }
+      this.req.query = { clsiserverid: 'node-1' }
       this.res.sendStatus = sinon.stub()
       return this.CompileController.deleteAuxFiles(
         this.req,
@@ -661,7 +662,7 @@ describe('CompileController', function() {
 
     it('should proxy to the CLSI', function() {
       return this.CompileManager.deleteAuxFiles
-        .calledWith(this.project_id)
+        .calledWith(this.project_id, this.user_id, 'node-1')
         .should.equal(true)
     })
 
@@ -714,8 +715,9 @@ describe('CompileController', function() {
     beforeEach(function() {
       this.CompileManager.wordCount = sinon
         .stub()
-        .callsArgWith(3, null, { content: 'body' })
+        .yields(null, { content: 'body' })
       this.req.params = { Project_id: this.project_id }
+      this.req.query = { clsiserverid: 'node-42' }
       this.res.send = sinon.stub()
       this.res.contentType = sinon.stub()
       return this.CompileController.wordCount(this.req, this.res, this.next)
@@ -723,7 +725,7 @@ describe('CompileController', function() {
 
     it('should proxy to the CLSI', function() {
       return this.CompileManager.wordCount
-        .calledWith(this.project_id, this.user_id, false)
+        .calledWith(this.project_id, this.user_id, false, 'node-42')
         .should.equal(true)
     })