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

Ported partner sites feature from ALMA NGAS source code #28

Merged
merged 6 commits into from
Sep 25, 2020

Conversation

smclay
Copy link
Contributor

@smclay smclay commented Sep 16, 2020

This is the partner sites feature that was developed and used by the ALMA observatory. It is very useful and important feature for every day ALMA use in development and testing. It is also useful in production.

I cleaned up the code and refactored it so it is much cleaner than it was on the older NGAS code base. Please let me know if there are any further improvements I can make.

Update:
I forgot to say that the partner site feature is enabled in the NGAS XML configuration by adding the following element...

    <!--
        Partner Sites

        List of alternative NGAS servers belonging to separate NGAS archive
        installations. When a request to retrieve a file cannot be found on
        the local NGAS archive the request is redirected to the NGAS servers
        included in the partner sites list. The ProxyMode attribute can be
        used to enable/disable partner sites.
    -->
    <PartnerSites ProxyMode="1">
        <PartnerSite Address="http://ngas01.example.com:8080"/>
    </PartnerSites>

Copy link
Contributor

@rtobar rtobar left a comment

Choose a reason for hiding this comment

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

@smclay thanks for the patches, it looks great :). The build is failing though, apparently because of a small change in one of the messages returned by the server. Could you have a look at that?

I also didn't see any new tests that exercise the new code, do you think you could add one? After we have tests passing in Travis we should get some measurements on code coverage, and could see what parts of the new code are getting hit and which aren't.

I left a few comments across the patch. In general it's really good, and I wonder only if we could reuse some code, specially around the issuing of proxy requests.

src/ngamsServer/ngamsServer/ngamsServer.py Show resolved Hide resolved
src/ngamsServer/ngamsServer/ngamsServer.py Show resolved Hide resolved
src/ngamsServer/ngamsServer/ngamsServer.py Outdated Show resolved Hide resolved
src/ngamsServer/ngamsServer/ngamsServer.py Outdated Show resolved Hide resolved
src/ngamsServer/ngamsServer/commands/status.py Outdated Show resolved Hide resolved
src/ngamsServer/ngamsServer/ngamsFileUtils.py Show resolved Hide resolved
@rtobar
Copy link
Contributor

rtobar commented Sep 18, 2020

I do not build NGAS. I assume this is done using the build.sh. I will test this and fix the problem.

Yes, build.sh basically goes through each of the packages and installs it. I usually go with a virtualenv first, then run build.sh -d to the packages installed in development mode. You can finally run the tests directly from the top-level directory with any unit test runner; I usually use pytest as it provides loads of flexibility.

Can you point me to a good example that I can use to get started? Is there a test that starts multiple NGAS servers where I can recreate a partner site setup?

A good example could be

def test_create_remote_subscriptions(self):
"""
Starts two servers A and B, and configures B to automatically create a
subscription to A when it starts. Then, archiving a file into A should
make it into B.
"""
subscription_pars = (('NgamsCfg.SubscriptionDef[1].Enable', '1'),
('NgamsCfg.SubscriptionDef[1].Subscription[1].HostId', 'localhost'),
('NgamsCfg.SubscriptionDef[1].Subscription[1].PortNo', '8888'),
('NgamsCfg.SubscriptionDef[1].Subscription[1].Command', 'QARCHIVE'))
self._prep_subscription_cluster(8888, (8889, subscription_pars, True))
# Listen for archives on server B (B is configured to send us notifications)
listener = self.notification_listener()
# File archived onto server A
self.archive(8888, 'src/SmallFile.fits', mimeType='application/octet-stream')
with contextlib.closing(listener):
# The built-in retry period is 10 seconds, so let's wait at maximum
# for twice that, in case the first attempt fails because the servers
# are not fully initialized
self.assertIsNotNone(listener.wait_for_file(20))
# Double-check that the file is in B
self.retrieve(8889, 'SmallFile.fits', targetFile=tmp_path())

But instead of starting the two servers via a single self.prepCluster call you want to issue two separate self.prepExtSrv calls with different ports and configuration parameters, as needed. That will give you two completely independent NGAS server instances (self.prepCluster starts multiple servers connected to the same database).

The self.archive and self.retrieve methods accept a port when more than one server is started, so you can target specific servers. They also assert that the result is successful (so there's no need for you to do that), and return the ngamsStatus object in case you want to inspect it further. If you are expecting a failure then you can use the corresponding *_fail alternative (e.g., self.archive_fail). Finally, these are not hardcoded, but actually are mirrored from the ngamsPClient methods, so any of the methods in that class can be used in this fashion (e.g., self.status).

@coveralls
Copy link

coveralls commented Sep 18, 2020

Coverage Status

Coverage increased (+0.05%) to 68.656% when pulling 41a4e03 on smclay:partner_sites into c6556bc on ICRAR:master.

smclay and others added 4 commits September 21, 2020 12:05
Until now every time we start an NGAS server during our tests, its
sqlite database is stored not under the NGAS root but under a common
directory. This path is fixed, and the only flexibility that further
server instances have is to point to the same database without
re-creating it, but not to point to a different one.

This commit changes where the sqlite database is created in different
use cases. When starting single NGAS servers, each server gets its own
sqlite file created under its root directory, and when a cluster is
created (via prepCluster()) a single database file is created.

While this adds the ability to start independent servers that are not
connected through a common database, this is true only for sqlite-based
tests. Tests using other databases will still have the limitation of
being connected to the same database instance. If full independence is
needed, those tests should run only when the database backend is sqlite.
@rtobar
Copy link
Contributor

rtobar commented Sep 25, 2020

As discussed via email, the new unit test is now working fine in the sqlite-based tests. The failing tests are expected, as the MySQL and PostgreSQL tests don't support separate, independent servers to be started easily. I will take care of these now, and then I'll merge to the master branch.

@rtobar rtobar merged commit 41a4e03 into ICRAR:master Sep 25, 2020
@rtobar rtobar mentioned this pull request Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants