Remove supported containers check at agent for machine startup

[cderici]

The running-in-container is removed years ago, so this uses systemd-detect-virt to detect the environment we're running in.

Also the setupContainerSupport seems to have no effect on provisioning machines, so this removes the pre-checks and allows any invalid provisions to be failed dynamically.

Checklist

• Code style: imports ordered, good names, simple structure, etc
• Comments saying why design decisions were made
• Go unit tests, with comments saying what you're testing

QA steps

This is used in the agent machine startup, so any newly added machine should be provisioned just fine. Also the placement should be checked as well.

juju bootstrap lxd
juju add-model
juju deploy ubuntu
juju add-machine
juju deploy ubuntu --to 1

juju bootstrap aws
juju add-model
juju deploy ubuntu
juju add-machine
juju deploy ubuntu --to 1

Links

Launchpad bug: https://bugs.launchpad.net/juju/+bug/2056200

Jira card: JUJU-5633

[SimonRichardson]

You're not guaranteed to return 1 and only 1 result set in the list. ParseContainerType only expects one argument.

[SimonRichardson]

Can we just delete this and tell me what the consequences are?

I suspect this has been broken, so it was never correctly filled in and we haven't noticed till I removed KVM. Considering that, just remove it and try the following:

• juju bootstrap lxd
• juju bootstrap aws and juju add-machine lxd
• juju deploy ubuntu --to lxd

This should cover the common cases to ensure this works (there are more)

[cderici]

You're right in this suspicion, removed all of these checks (including the kvm check) and everything seems to be working just fine (on both lxd and aws).

[cderici]

/build

[cderici]

/build

[SimonRichardson]

@cderici let's discuss this at the sprint.

[cderici]

@SimonRichardson bootstrapping with this on lxd and aws, adding machines and deployments works just fine (also tried placement etc), but I'm gonna need extra pair of eyes on the failing test.

I'm not sure why this test depends on this check (fails without instance.LXD in there). It literally passes when I add instance.LXD unconditionally like supportedContainers = append(supportedContainers, instance.LXD) (i.e. nothing is mocking it), but it doesn't makes sense to me how it's getting true from container.ContainersSupported() in the first place since the running-in-container doesn't even exist.

[manadart]

Your QA steps don't check whether you can run a container on a machine.

With this change, you can't because all machines exhibit this behaviour:

machine-1: 15:22:59 INFO juju.worker.lxdprovisioner uninstalling no supported containers on "machine-1"
machine-1: 15:22:59 INFO juju.worker.kvmprovisioner uninstalling no supported containers on "machine-1"

[cderici]

/build

[manadart]

Still got failures in trackerSuite based on container support changes.

[cderici]

Still got failures in trackerSuite based on container support changes.

Yes, I'll write down what's happening for future reference, before this change the NewTracker in the containerbroker fails with ErrUninstall -- "resource permanently unavailable" --, whenever i) the supported containers can't be determined, or ii) the instance.LXD is not found. Since we add the instance.LXD unconditionally to the supported containers, these fails never happen (which renders the check against these redundant - hence the change in the broker).

The failing tests:

• trackerSuite.TestNewTrackerWithKVMContainers
• trackerSuite.TestNewTrackerWithNoContainers
• trackerSuite.TestNewTrackerWithNoDeterminedContainers

all check against those failures. The new expectation for these tests is to not fail (because the machines always support instance.LXD), which renders their scenarios identical after making changes:

_, err := s.withScenario(c,
		nil,
		s.expectMachineTag,
		s.expectMachines,
		s.expectContainerConfig,
	)

so changing the expectation and removing the redundant ones.

A side note, there's no specific check in the broker for the instance.KVM, the test trackerSuite.TestNewTrackerWithKVMContainers is written originally to fail when instance.LXD is not found, which is not happening anymore, so it's removed.