Merge ~andersson123/autopkgtest-cloud:cache-amqp-fix into autopkgtest-cloud:master

[Tim Andersson]

[Paride Legovini (paride)]

AFAICT currently the autopkgtest-cloud service would keep working fine if the web workers are down, as those are only used for user interaction. Here we add an dependency on the web units being up, in order for the process to work, and I think we don't want this.

Also: loading the queue contents in two very different ways depending on whether the unit is leader or not will make debugging difficult.

The AutopkgtestQueueContents class already has checks for leader, adding an "external" check like proposed here calls for other changes in that logic.

What I think is that there should be another way to query rabbitmq for queue length, without popping / pushing all the queue items.

review: Disapprove

[Tim Andersson (andersson123)]

I don't understand how this adds a dependency on the web units being up. The only dependency is that the non-leader web unit depends on the leader web-unit to be functional, somewhat. If there's something I'm missing let me know, please elaborate.

I don't see why loading it in two different ways makes debugging difficult - the tracebacks would always be explicitly different (one would involve urllib, one would involve amqplib), and we can easily find out which web unit is the leader, thus helping debugging. We can also make the debug logging more verbose.

Using the leader check from AutopkgtestQueueContents does make sense, I'll amend for that.

As far as I understand, that's not how rabbitmq works. See the link below:
https://stackoverflow.com/questions/10709533/is-it-possible-to-view-rabbitmq-message-contents-directly-from-the-command-line

I think there's maybe ways with rabbitmqctl and rabbitmq_admin but we're not running this script from the rabbitmq machine.

[Brian Murray (brian-murray)]

Paride raises a valid about hard coding the cookies for the servers in the script in part because this would need updating whenever we add or replace web servers. (Something we've had to do in the past!)

An alternate approach would be to write the IP addresses of all the autopkgtest-web servers to ~/autopkgtest-cloud.conf. This file is already read by cache-amqp and could be used to connect to the other apache2 servers directly and grab the queues.json file and by passing the haproxy server.

Does that sound like a workable solution to everyone?

review: Needs Fixing

[Tim Andersson (andersson123)]

I thought about what you suggest but you can't ssh between web units, so that'd be quite involved to make that possible.

I think we could *not* hard code the cookies with something like this (pseudocode):

for i in range(x):
    # get queues.json until it's different to the one on disk
    srv_name = "S" + str(i)
    etc

What do you think? I think incorporating the ip addresses is much more involved IMO

[Tim Andersson (andersson123)]

Or, actually, we could incorporate the cookies in ~/autopkgtest-cloud.conf ?

I think that's probably the best approach

[Tim Andersson (andersson123)]

https://git.launchpad.net/~andersson123/autopkgtest-cloud/commit/?id=ef034b7fc60f92e69bd1fda874694182086e60e6

I think this MP relies on some of the changes from the above commit

[Brian Murray (brian-murray)]

This looks good I've just one in-line comment regarding clarifying the types of cookies in the configuration. Feel free to merge this after making that change. Thanks!

review: Approve

[Tim Andersson (andersson123)]

Great, thanks! I'll merge once CI has passed.

diff --git a/charms/focal/autopkgtest-web/config.yaml b/charms/focal/autopkgtest-web/config.yaml
index cc97824..20def8c 100644
--- a/charms/focal/autopkgtest-web/config.yaml
+++ b/charms/focal/autopkgtest-web/config.yaml
@@ -34,3 +34,9 @@ options:
     default:
     description: "$no_proxy environment variable (for accessing sites \
                   other than github, like login.ubuntu.com and launchpad.net)"
+  cookies:
+    type: string
+    default:
+    description: "SRVNAME cookies for each autopkgtest-web unit. \
+                  Each web unit has a cookie assigned which tells the \
+                  haproxy server which web unit to redirect a request to."
diff --git a/charms/focal/autopkgtest-web/reactive/autopkgtest_web.py b/charms/focal/autopkgtest-web/reactive/autopkgtest_web.py
index db60fcb..93eaa21 100644
--- a/charms/focal/autopkgtest-web/reactive/autopkgtest_web.py
+++ b/charms/focal/autopkgtest-web/reactive/autopkgtest_web.py
@@ -54,10 +54,12 @@ def setup_rabbitmq(rabbitmq):
     "amqp.available",
     "config.set.storage-url-internal",
     "config.set.hostname",
+    "config.set.cookies",
 )
 def write_autopkgtest_cloud_conf(rabbitmq):
     swiftinternal = config().get("storage-url-internal")
     hostname = config().get("hostname")
+    cookies = config().get("cookies")
     rabbituser = rabbitmq.username()
     rabbitpassword = rabbitmq.password()
     rabbithost = rabbitmq.private_address()
@@ -71,6 +73,7 @@ def write_autopkgtest_cloud_conf(rabbitmq):
             database_ro=/home/ubuntu/public/autopkgtest.db
             SwiftURL={swiftinternal}
             ExternalURL=https://{hostname}/results
+            cookies={cookies}
 
             [amqp]
             uri=amqp://{rabbituser}:{rabbitpassword}@{rabbithost}""".format(
diff --git a/charms/focal/autopkgtest-web/webcontrol/cache-amqp b/charms/focal/autopkgtest-web/webcontrol/cache-amqp
index 512b495..33eb496 100755
--- a/charms/focal/autopkgtest-web/webcontrol/cache-amqp
+++ b/charms/focal/autopkgtest-web/webcontrol/cache-amqp
@@ -10,6 +10,7 @@ import sys
 import tempfile
 import time
 import urllib.parse
+import urllib.request
 
 import amqplib.client_0_8 as amqp
 from amqplib.client_0_8.exceptions import AMQPChannelException
@@ -259,9 +260,42 @@ if __name__ == "__main__":
     except KeyError:
         print("No database found", file=sys.stderr)
         sys.exit(1)
+    try:
+        webpage = cp["web"]["ExternalURL"].replace("/results", "")
+    except KeyError:
+        print("No external url found!")
+        sys.exit(1)
+    try:
+        cookies = cp["web"]["cookies"]
+    except KeyError:
+        print("No cookies in config!")
+        sys.exit(1)
 
-    aq = AutopkgtestQueueContents(amqp_uri, database)
-    queue_contents = aq.get_queue_contents()
+    queue_contents = {}
+    # We only actually want to interact with the rabbitmq queue if
+    # we're the leader - otherwise these two cache-amqp processes
+    # make the queue size go crazy in the KPI
+    if os.path.isfile("/run/autopkgtest-web-is-leader"):
+        # Get queue details from rabbitmq directly
+        aq = AutopkgtestQueueContents(amqp_uri, database)
+        queue_contents = aq.get_queue_contents()
+    else:
+        # We get queues.json from autopkgtest.ubuntu.com, see if it's
+        # the same as the version on disk. If it's not, then this webworker
+        # (the non leader one) has acquired queues.json from the leader.
+        # If they're the same we've queried ourselves for queues.json
+        # and we increment the SRVNAME cookie to query the other
+        # webworker (the leader)
+        queued_local = {}
+        if os.path.isfile(args.output):
+            with open(args.output, "r") as f:
+                queued_local = json.load(f)
+        for srvname_cookie in cookies.split(" "):
+            req = urllib.request.Request(webpage + "/queues.json")
+            req.add_header("Cookie", "SRVNAME=" + srvname_cookie)
+            queue_contents = json.loads(urllib.request.urlopen(req).read())
+            if queue_contents != queued_local:
+                break
 
     with tempfile.NamedTemporaryFile(
         mode="w", dir=os.path.dirname(args.output), delete=False
diff --git a/mojo/service-bundle b/mojo/service-bundle
index 17d8bbd..42d0566 100644
--- a/mojo/service-bundle
+++ b/mojo/service-bundle
@@ -191,6 +191,7 @@ applications:
             swift-web-credentials: include-file://{{local_dir}}/swift-web-credentials.conf
             https-proxy: {{ https_proxy }}
             no-proxy: {{ no_proxy }}
+            cookies: S0 S1
 {%- endif %}
     haproxy:
         charm: cs:haproxy