[sled-agent] Avoid races on service import, properties on instances, explicit refresh

[smklein]

Fixes #1115 , hopefully.

Previously

I used the following script:

set -x
oxide org     create myorg -D "description"
oxide project create --organization myorg myproject -D "description"
set +x

set -x
for i in {0..10000}; do
  INSTANCE_NAME="myinstance${i}"
  oxide instance create --organization myorg --project myproject ${INSTANCE_NAME} -D "description" -c 2 -m 1073741824 --hostname myinstance
  oxide instance stop   --confirm --organization myorg --project myproject ${INSTANCE_NAME}
  oxide instance delete --confirm --organization myorg --project myproject ${INSTANCE_NAME}
done
set +x

After < 10 iterations, I'd typically see an SMF failure, either due to a missing property, or due to some issue on import.

After discussion with @bnaecker and @rmustacc , we confirmed that the Propolis Server zone automatically imports the manifest, and that it may be racing with an explicit import.

This PR

This PR performs the following actions:

1. Rather than importing the manifest, we simply try to add a service instance in a retry loop. As seen in the log files, this typically fails for the first couple invocations (with the output svccfg: Pattern 'svc:/system/illumos/propolis- server' doesn't match any instances or services), but succeeds once the built-in configd service has finished importing the propolis manifest.
2. It sets configuration parameters on the service instance, rather than the service itself.
3. It explicitly refreshes the instance before enabling it, to make sure the properties are up-to-date.

The aforementioned "create an instance, stop it, destroy it" script has been running on my machine for nearly 100 iterations without a single failure using this PR, which is a major improvement.

[bnaecker]

We had talked about putting an additional check here, that the config/server_addr property has actually taken for the instance. Do you think that's overly paranoid?

[smklein]

I think I'm okay omitting it, based on the data collected (500+ iterations without the server_addr missing), but I'd be happy to add it later if we see any issues?

[bnaecker]

Maybe add the service name to these log messages? The actual address might also be nice, in the svccfg setprop call below.

[smklein]

I'll add more context. These all have the instance ID as part of the context already, so the log messages look like:

[2022-05-27T00:08:00.58389195Z]  INFO: SledAgent/InstanceManager/6692 on thelio: Adding service property group (instance_id=cd7cbd04-5507-4c53-a1e0-1f1e6016e7bb)
[2022-05-27T00:08:00.617921961Z]  INFO: SledAgent/InstanceManager/6692 on thelio: Setting server address property (instance_id=cd7cbd04-5507-4c53-a1e0-1f1e6016e7bb)
[2022-05-27T00:08:00.649773537Z]  INFO: SledAgent/InstanceManager/6692 on thelio: Refreshing instance (instance_id=cd7cbd04-5507-4c53-a1e0-1f1e6016e7bb)
[2022-05-27T00:08:00.675333382Z]  INFO: SledAgent/InstanceManager/6692 on thelio: Enabling instance (instance_id=cd7cbd04-5507-4c53-a1e0-1f1e6016e7bb)   

This should make it easy to disambiguate if multiple instances are being allocated simultaneously

[bnaecker]

Thanks for taking this on! Looks good, and sounds like you've not hit any similar races after a while through the instance creation loop. I have a couple of comments, but otherwise 👍 .

[bnaecker]

For reference, this should close #1115.