[JUJU-2030]Kill unconverted api workers 2 9 #14834

hmlanigan · 2022-10-26T21:56:07Z

Split the unconverted-api-workers into two new workers and remove.

machine-setup runs setup on machines which require an API Connection. Once setup is done it can be uninstalled.

kvm-container-provisioner and lxd-container-provisioner both wait for a machine of their container type to be provisioned if the machine supports their container type. If it doesn't, the worker uninstalls. The first container provisioned of their type will cause setup for the container systems to run, then a container provisioner is started.

QA steps

$ juju bootstrap openstack dot --constraints allocate-public-ip=true
$ juju bootstrap openstack src --constraints allocate-public-ip=true
$ juju add-model move-me 
$ juju set-model-constraints allocate-public-ip=true
$ juju add-machine lxd
$ juju add-machine kvm
$ juju add-machine 
$ juju remove-machine 0/lxd/0
$ juju add-machine lxd:0

# once settled
$ juju migrate move-me dst
$ juju switch dst:move-me

$ juju add-machine lxd:1

# checkout the juju_engine_report on each.  The kvm-container-provisioner and lxd-container-provisioner have reports on whether the container watchers are still running and if the provisioners are running.

Bug reference

https://bugs.launchpad.net/juju/+bug/1994488
https://bugs.launchpad.net/juju/+bug/1983140

Due to unconverted-api-worker refactor into workers using manifolds, WatchAllContainers is no longer used by the provisioner.

A step in refactoring the unconverted-api-workers into pieces using individual manifolds. MachineStartup will handle machine setup necessary but requiring an API Connection. The actual work is part of the machine agent and will be done via a config method.

A step in refactoring the unconverted-api-worker manifold into individual pieces. These will hold a container provisioner based on container type.

The unconverted-api-worker is being split into three pieces. machine-setup is short lived and does setup on the machine which required an API Connection. The other two are container provisioner workers for lxd and kvm. The functionality of the container-watcher for setup is placed into the container provisioner worker. It waits for a container to be provisioned, configures the container system on the machine and starts an container provisioner worker.

Always verify LXD and KVM in setupContainerSupport.

Renamed ContainerWorker to containerSetupAndProvisioner for understandability. Updated unit tests for container setup.

Change Report() results to make tests easier. Remove unused code from unit tests. Change capitalization of ContainerSetupAndProvisioner.

Changes to code to faciliate mocks, including adding a couple of interfaces and a shim. Moved code out of start to a new method for same. This can be resolved once we have good mocking in this package.

tlm · 2022-11-01T12:58:50Z

cmd/jujud/agent/engine_test.go

 		"machine-action-runner",
+		//"machine-setup", exits when done


Can we remove ?

This one I left on purpose to explain that the machine-setup manifold isn't running when at first glance it should be.

tlm · 2022-11-01T13:14:32Z

cmd/jujud/agent/machine.go

+	}
+
+	if err := a.setupContainerSupport(apiConn, logger); err != nil {
+		return err


Should we annotate this?

This method can return errors from the dependency pkg. I've noticed that if wrapped, the dependency engine doesn't handle them as expected. I added logging messages to explain why we were calling ErrUninstall. Check that they make sense so we can find answers in the future.

tlm · 2022-11-01T13:15:06Z

cmd/jujud/agent/machine.go

 		}
 		if result.Code != 0 {
-			return nil, errors.New(fmt.Sprintf("cannot patch /etc/update-manager/release-upgrades: %s", result.Stderr))
+			return errors.New(fmt.Sprintf("cannot patch /etc/update-manager/release-upgrades: %s", result.Stderr))


This would be better as an fmt.Errorf call

very old juju 1 code, will do

tlm · 2022-11-01T13:20:08Z

cmd/jujud/agent/machine.go

+	if len(result) != 1 {
+		return errors.Errorf("expected 1 result, got %d", len(result))
+	}
+	if errors.IsNotFound(result[0].Err) || (result[0].Err == nil && result[0].Machine.Life() == life.Dead) {


errors.Is(err, errors.NotFound)

tlm · 2022-11-01T13:38:16Z

worker/provisioner/container_initialisation.go

+func (cs *ContainerSetup) initialiseContainerProvisioner() (Provisioner, error) {
+	cs.logger.Debugf("setup provisioner for %s containers", cs.containerType)
+	if cs.managerConfig == nil {
+		return nil, errors.New("Programming error, manager config not setup")


Should this be a notvalid ?

tlm · 2022-11-01T13:46:45Z

worker/provisioner/container_initialisation_test.go

 }

 var _ = gc.Suite(&containerSetupSuite{})

-func (s *containerSetupSuite) TestStartContainerStartsContainerProvisioner(c *gc.C) {
-	defer s.patch(c).Finish()
+func (s *containerSetupSuite) TestInitialiseContainersLXD(c *gc.C) {


Think these are around the wrong way.

tlm · 2022-11-01T13:46:59Z

worker/provisioner/container_initialisation_test.go


-	// Adding one new container machine.
-	s.notify([]string{"0/lxd/0"})
+func (s *containerSetupSuite) TestInitialiseContainersKVM(c *gc.C) {


tlm · 2022-11-01T13:47:44Z

worker/provisioner/container_initialisation_test.go


-	s.cleanKill(c, runner)
+func (s *containerSetupSuite) TestInitialiseContainerProvisionerLXD(c *gc.C) {


Around the wrong way.

tlm · 2022-11-01T13:47:50Z

worker/provisioner/container_initialisation_test.go

+	s.testInitialiseContainers(c, instance.KVM)
+}
+
+func (s *containerSetupSuite) TestInitialiseContainerProvisonerKVM(c *gc.C) {


Around the wrong way.

tlm · 2022-11-01T14:21:55Z

worker/provisioner/containermanifold.go

+	}
+	pr := apiprovisioner.NewState(apiCaller)
+
+	m, err := cfg.machineSupportsContainers(&containerShim{api: pr}, mTag)


nit can we name m to something better?

tlm · 2022-11-01T14:27:57Z

worker/provisioner/containermanifold.go

+		typeSet.Add(string(v))
+	}
+	if !typeSet.Contains(string(cfg.ContainerType)) {
+		cfg.Logger.Infof("%s does not support %s containers", mTag, string(cfg.ContainerType))


s/containers/container

tlm · 2022-11-01T14:28:46Z

worker/provisioner/containerworker.go

+	"github.com/juju/juju/core/watcher"
+)
+
+// NewContainerSetupAndProvisioner return a ContainerSetupAndProvisioner.


s/return/returns

Use errors.Is rather than deprecated methods and update very old style messages.

The kvm-container-provisioner is only running on hardware which supports kvm. Thus the test is hardware dependent and gives different results. Allow for this by specifying optional workers. Startup of kvm-container-provisioner is tested in the provisioner worker.

tlm

lgtm

cmd/jujud/agent/agenttest/engine.go

hmlanigan · 2022-11-02T10:55:23Z

/merge

of unconverted-api-workers to manifold workers.

hmlanigan · 2022-11-02T13:37:42Z

/merge

hmlanigan · 2022-11-02T16:09:23Z

/merge

#14862 Merge Conflicts (which I resolved): - .github/workflows/smoke.yml - cmd/jujud/agent/machine/manifolds_test.go - cmd/jujud/agent/machine_test.go - go.mod - go.sum - scripts/win-installer/setup.iss - snap/snapcraft.yaml - tests/suites/manual/deploy_manual.sh - version/version.go Merges: - #14837 - #14846 - #14834 - #14851 - #14857 - #14853

#14902 Merge 3.0 branch #14837 #14846 #14834 #14854 #14851 ``` # Conflicts: # scripts/win-installer/setup.iss # snap/snapcraft.yaml # version/version.go ```

#15042 The kvm-provisioner worker is expected to run in all machine manifolds, but uninstalls itself if KVM support is not detected. #14834 Softened the check for the worker by adding optional workers to the engine test logic. #14862 was merged incorrectly resulting in an extra worker assertion before including the kvm-provisioner was added as an optional worker. This causes tests to fail depending on where they are run. Here we remove the duplicate assertion along with the optional worker logic. Instead we just patch `IsKVMSupported` to return true and use a hard expectation that the working is running. ## QA steps `go test -v ./cmd/jujud/agent/ -check.v -check.f=MachineSuite.TestMachineWorkers` ## Documentation changes None. ## Bug reference N/A

hmlanigan added 9 commits November 1, 2022 14:09

Remove unused WatchAllContainers.

30eb0da

Due to unconverted-api-worker refactor into workers using manifolds, WatchAllContainers is no longer used by the provisioner.

Create ContainerProvisioningManifold and ContainerWorker

a001610

A step in refactoring the unconverted-api-worker manifold into individual pieces. These will hold a container provisioner based on container type.

Do no assume container support cannot change.

eef927c

Always verify LXD and KVM in setupContainerSupport.

Name changes and unit tests.

4ffd7e8

Renamed ContainerWorker to containerSetupAndProvisioner for understandability. Updated unit tests for container setup.

minor updates.

6925fe2

Change Report() results to make tests easier. Remove unused code from unit tests. Change capitalization of ContainerSetupAndProvisioner.

Add unit tests for ContainerSetupAndProvisioner

c05884e

Implement unit tests for container manifold.

9af39de

Changes to code to faciliate mocks, including adding a couple of interfaces and a shim. Moved code out of start to a new method for same. This can be resolved once we have good mocking in this package.

hmlanigan marked this pull request as ready for review November 1, 2022 13:10

hmlanigan force-pushed the kill-unconverted-api-workers-2-9 branch from df1ccae to 9af39de Compare November 1, 2022 13:10

tlm reviewed Nov 1, 2022

View reviewed changes

Resolve linter reported error, check returned error.

8b75649

tlm reviewed Nov 1, 2022

View reviewed changes

tlm approved these changes Nov 1, 2022

View reviewed changes

hmlanigan added 3 commits November 2, 2022 11:28

Improve error messages.

761a2a4

Use errors.Is rather than deprecated methods and update very old style messages.

Improve wording of comments, log notes and variables

3db3585

hmlanigan force-pushed the kill-unconverted-api-workers-2-9 branch from a6d3c7f to 9f7bea7 Compare November 2, 2022 10:29

tlm approved these changes Nov 2, 2022

View reviewed changes

cmd/jujud/agent/agenttest/engine.go Show resolved Hide resolved

WatchAllContainers removed from mock. No longer used with refactor

5747c39

of unconverted-api-workers to manifold workers.

jujubot merged commit 51672c0 into juju:2.9 Nov 2, 2022

hmlanigan deleted the kill-unconverted-api-workers-2-9 branch November 3, 2022 07:21

jack-w-shaw mentioned this pull request Nov 4, 2022

2.9 into 3.0 #14862

Merged

wallyworld mentioned this pull request Nov 21, 2022

Merge 3.0 #14902

Merged

manadart mentioned this pull request Jan 11, 2023

[JUJU-2419] Expect kvm-provisioner worker to run in machine manifolds #15042

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JUJU-2030]Kill unconverted api workers 2 9 #14834

[JUJU-2030]Kill unconverted api workers 2 9 #14834

hmlanigan commented Oct 26, 2022 •

edited

tlm Nov 1, 2022

hmlanigan Nov 1, 2022

tlm Nov 1, 2022

hmlanigan Nov 1, 2022

tlm Nov 1, 2022

hmlanigan Nov 1, 2022

tlm Nov 1, 2022

hmlanigan Nov 1, 2022

tlm Nov 1, 2022

hmlanigan Nov 1, 2022

tlm Nov 1, 2022

hmlanigan Nov 1, 2022

tlm Nov 1, 2022

tlm Nov 1, 2022

tlm Nov 1, 2022

hmlanigan Nov 1, 2022

tlm Nov 1, 2022

tlm Nov 1, 2022

hmlanigan Nov 1, 2022

tlm Nov 1, 2022

hmlanigan Nov 1, 2022

tlm left a comment

hmlanigan commented Nov 2, 2022

hmlanigan commented Nov 2, 2022

hmlanigan commented Nov 2, 2022


		s.cleanKill(c, runner)
		func (s containerSetupSuite) TestInitialiseContainerProvisionerLXD(c gc.C) {

[JUJU-2030]Kill unconverted api workers 2 9 #14834

[JUJU-2030]Kill unconverted api workers 2 9 #14834

Conversation

hmlanigan commented Oct 26, 2022 • edited

QA steps

Bug reference

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlm left a comment

Choose a reason for hiding this comment

hmlanigan commented Nov 2, 2022

hmlanigan commented Nov 2, 2022

hmlanigan commented Nov 2, 2022

hmlanigan commented Oct 26, 2022 •

edited