LXD connections are attempted via SSH iff a non-default `key_pair` is specified

[OddBloke]

Due to this code:

pycloudlib/pycloudlib/lxd/instance.py

Lines 31 to 39 in 1ac9d4c

	def _run_command(self, command, stdin):
	"""Run command in the instance."""
	if self.key_pair:
	return super()._run_command(command, stdin)
	base_cmd = [
	'lxc', 'exec', self.name, '--', 'sudo', '-u', self.username, '--'
	]
	return subp(base_cmd + list(command), rcs=None)

This also interacts badly with LXD's SSH key defaults. For most clouds, the key_pair used by default is

pycloudlib/pycloudlib/cloud.py

Lines 30 to 32 in 1ac9d4c

	self.key_pair = KeyPair(
	'/home/%s/.ssh/id_rsa.pub' % _username, name=_username
	)

LXD, however, has a different default:

pycloudlib/pycloudlib/lxd/cloud.py

Lines 49 to 50 in 1ac9d4c

	# User must manually specify the key pair to be used
	self.key_pair = None

This means that in the default case, LXD will exec. In itself, this is not a problem.

However, it also means that if a consumer chooses to specify a non-default key (via .use_key), LXD's behaviour changes. self.key_pair is no longer None, and so LXD will start using SSH to access instances.

This is biting us in the cloud-init integration testing: we have a configuration option to specify an SSH key (and we call use_key if provided). If it isn't set, then we want to use pycloudlib's default (i.e. we don't call use_key at all). This means that users who do specify the configuration option see test failures on LXD: we rely on the fact that LXD does not require functioning networking to access the instance for some tests, and so using SSH (as pycloudlib does for such users) is guaranteed to fail.

[OddBloke]

It's clear to me that we should decouple LXD's choice of execution method from the presence of an SSH key: what we have currently is action at a distance, and took me a while to figure out.

It's less clear to me what the correct way of selecting execution method is. I don't think it belongs on execute: that's part of the super-class's API, so introducing a LXD-specific flag for it isn't ideal (and, indeed, ISTR we've tried that before).

Given that a parameter isn't an option, the two choices that I can think of are (a) a flag on the LXDInstance instance (if self.use_ssh:) which is documented as something consumers can toggle; or (b) have _run_command only handle one of the paths, and have a separate callable on the LXDInstance class (something like instance.run_ssh_command(...) or instance.exec_command(...), depending on what we choose to be the default behaviour) which handles the other path.

(If we choose (b) we likely should default to SSH: that would let us drop LXD's _run_command implementation altogether, and make executes behaviour consistent across clouds.)

[OddBloke]

With the below diff applied, I performed a full run of cloud-init's integration tests for (exec, SSH) x (containers, VMs). I gathered all the logged values, and used python3 -c "import statistics, sys; f = open(sys.argv[1]); lines = [float(l) for l in f.readlines()]; print(statistics.mean(lines), statistics.stdev(lines));" $file to determine these stats:

• Command execution
  • Containers
    • SSH: 1.8441089216868083 4.3788216087710685
    • exec: 0.6892001838014837 2.9038017724578737
  • VMs
    • SSH: 5.415026400159683 12.181209723419395
    • exec: 3.8129768721262614 14.493298537196539
• Total launch
  • Containers
    • SSH: 13.811445441739313 9.204732248776105
    • exec: 14.199659371376038 8.794188580850891
  • VMs
    • SSH: 42.23560203711192 15.237234929157054
    • exec: 46.9354841027941 29.479169790474028

These numbers are far from perfect: most notably, the command execution stats include cloud-init wait times, and whoami times, from which we would expect some underlying variance (above and beyond the variance of any command's execution) because they execute at different points for different amounts of time depending on the connection mechanism (specifically, lxc exec fails immediately if we can't access the system yet, whereas we wait for SSH timeouts via SSH: both of these counts as 1 command execution).

That said, I think we can see that launching instances is pretty much the same via either mechanism: this isn't too surprising, we can infer that both mechanisms (generally) gain access to instances before cloud-init is complete and therefore are both waiting for the same event.

Executing via SSH does appear to be slower than exec, but not by a substantial margin, and I think that the commands run while waiting for boot are skewing it.

If we strip out the long-running-because-we're-still-booting commands from both the container exec and SSH logs, then I see the following:

• Container
  • SSH command execution
    • 0.10482012534487074 0.14268130866973075 0.33558363914489747
  • exec command execution
    • 0.2956880251566569 0.15257323303979864 0.5218142986297607
• VM
  • SSH command execution
    • 0.1113380072068195 0.15245459811784765 0.3554847240447998
  • exec command execution
    • 2.5195744402268354 11.341280197598701 2.18221435546875

Here, it's clear that SSHing in is faster. Intuitively, this makes sense to me: executing a command via exec goes shell -> lxd binary -> lxd daemon (-> lxd agent) -> instance whereas SSH goes shell -> SSH binary -> sshd -> instance which is both fewer boundaries in the VM case and using code that has had decades to be optimised (and also doesn't have to worry about GC).

---

diff --git a/pycloudlib/instance.py b/pycloudlib/instance.py
index ed8f4b2..e1a837a 100644
--- a/pycloudlib/instance.py
+++ b/pycloudlib/instance.py
@@ -94,8 +94,11 @@ class BaseInstance(ABC):
     def wait(self):
         """Wait for instance to be up and cloud-init to be complete."""
         self._wait_for_instance_start()
+        before = time.time()
         self._wait_for_execute()
         self._wait_for_cloudinit()
+        after = time.time()
+        self._log.critical("!!>: %s", after - before)
 
     @abstractmethod
     def wait_for_delete(self):
@@ -334,7 +337,7 @@ class BaseInstance(ABC):
                 )
                 last_exception = e
                 retries -= 1
-                time.sleep(10)
+                time.sleep(1)
 
         self._log.error('Failed ssh connection to %s@%s:%s after 10 minutes',
                         self.username, self.ip, self.port)
@@ -382,6 +385,7 @@ class BaseInstance(ABC):
                 retries -= 1
                 time.sleep(10)
 
+
         if result.failed:
             raise OSError(
                 "{}\n{}".format(
diff --git a/pycloudlib/lxd/instance.py b/pycloudlib/lxd/instance.py
index d2d9a87..ca59b30 100644
--- a/pycloudlib/lxd/instance.py
+++ b/pycloudlib/lxd/instance.py
@@ -30,13 +30,17 @@ class LXDInstance(BaseInstance):
 
     def _run_command(self, command, stdin):
         """Run command in the instance."""
-        if self.key_pair:
-            return super()._run_command(command, stdin)
-
-        base_cmd = [
-            'lxc', 'exec', self.name, '--', 'sudo', '-u', self.username, '--'
-        ]
-        return subp(base_cmd + list(command), rcs=None)
+        start = time.time()
+        if True:  # This was manually modified between runs
+            ret = super()._run_command(command, stdin)
+        else:
+            base_cmd = [
+                'lxc', 'exec', self.name, '--', 'sudo', '-u', self.username, '--'
+            ]
+            ret = subp(base_cmd + list(command), rcs=None)
+        end = time.time()
+        self._log.info("!!!: %s", end - start)
+        return ret
 
     @property
     def is_vm(self):
@@ -72,7 +76,7 @@ class LXDInstance(BaseInstance):
             IP address assigned to instance.
 
         """
-        retries = 5
+        retries = 60
 
         while retries != 0:
             command = 'lxc list {} -c 4 --format csv'.format(self.name)
@@ -82,7 +86,7 @@ class LXDInstance(BaseInstance):
                 break
 
             retries -= 1
-            time.sleep(20)
+            time.sleep(1)
 
         ip_address = result.split()[0]
         return ip_address

[OddBloke]

Given the above, I think it's not only reasonable but preferable to make LXD's default execution behaviour use SSH. It's both quicker, and more consistent with our other platforms (meaning that it's more likely that local/CI LXD testing will catch issues that we otherwise might not find until testing against an actual cloud).

On further reflection, having a flag on the instance object ((a) as described above) doesn't seem right to me: the method of execution is a property of that particular execution, not a property of the instance (which we know because, most of the time, we can use either execution method against an instance).

So: my proposal is to add a .lxc_exec method to LXDInstance which will have the same code as LXDInstance._run_command does today minus the super()._run_command call.

[OddBloke]

@TheRealFalcon @blackboxsw @lucasmoura Your thoughts would be appreciated!

[TheRealFalcon]

Great exploration and analysis! Given these results, I agree with your proposal. Use ssh for "normal" execution, and have a special method on the LXD instance to run things specific to LXD that we run via exec.

[OddBloke]

On further reflection, having a flag on the instance object ((a) as described above) doesn't seem right to me: the method of execution is a property of that particular execution, not a property of the instance (which we know because, most of the time, we can use either execution method against an instance).

On even further reflection (and, more relevantly, actual implementation), this conclusion was mistaken: if we don't make method of execution a property of the instance, then we have to thread a parameter to select method of execution all the way through to _wait_for_execute (else it will timeout waiting for SSH against instances which are expecting to use exec). This complexity is not warranted for the marginal benefit of being able to select per-execute call, so I'm going to switch to having this be a property of the instance.

This means instead of dropping _run_command, I'll instead modify it to key off a new instance variable (i.e. my original (a) proposal).