From fefa65d128b99e6d07c735c6376e003d13f25105 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Wed, 15 Jan 2020 16:49:00 -0500 Subject: [PATCH 1/4] Enable install hook for k8s charms. Part of uniter++ work. --- worker/uniter/storage/resolver.go | 18 ++++++++++++------ worker/uniter/uniter.go | 25 +++++++++++++------------ 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/worker/uniter/storage/resolver.go b/worker/uniter/storage/resolver.go index efd6e526a1a..864158375cc 100644 --- a/worker/uniter/storage/resolver.go +++ b/worker/uniter/storage/resolver.go @@ -47,11 +47,6 @@ func (s *storageResolver) NextOp( opFactory operation.Factory, ) (operation.Operation, error) { - // Only IAAS models need to first run the install hook. - // For CAAS models, storage is specified in the pod config - // and mounted as the pod in started. - blockedWaitingForInstall := !localState.Installed && s.modelType == model.IAAS - if remoteState.Life == params.Dying { // The unit is dying, so destroy all of its storage. if !s.dying { @@ -70,14 +65,25 @@ func (s *storageResolver) NextOp( return nil, errors.Trace(err) } + // Only IAAS models need to first run the install hook. + // For CAAS models, storage is specified in the pod config + // and mounted as the pod in started. + blockedWaitingForInstall := !localState.Installed && s.modelType == model.IAAS + var runStorageHooks bool switch { case localState.Kind == operation.Continue: // There's nothing in progress. - runStorageHooks = true + fallthrough case blockedWaitingForInstall && localState.Kind == operation.RunHook && localState.Step == operation.Queued: // The install operation completed, and there's an install // hook queued. Run storage-attached hooks first. + fallthrough + case s.modelType == model.CAAS: + // CAAS models skip the uniter Install operation, but do run the + // install hook. No need to wait for the install hook to be run + // as storage is mounted when a pod is started. This will keep + // behavior consistent with before CAAS units run an install hook, runStorageHooks = true } if !runStorageHooks { diff --git a/worker/uniter/uniter.go b/worker/uniter/uniter.go index 6da2ec7cf4d..96d7852fea3 100644 --- a/worker/uniter/uniter.go +++ b/worker/uniter/uniter.go @@ -618,24 +618,25 @@ func (u *Uniter) init(unitTag names.UnitTag) (err error) { return errors.Trace(err) } - var initialState operation.State - if u.modelType == model.IAAS { - initialState = operation.State{ - Kind: operation.Install, - Step: operation.Queued, - CharmURL: charmURL, - } - } else { + initialState := operation.State{ + Kind: operation.Install, + Step: operation.Queued, + CharmURL: charmURL, + } + + if u.modelType == model.CAAS { + // For CAAS, run the install hook, but not the + // full install operation. initialState = operation.State{ - Hook: &hook.Info{Kind: hooks.Start}, - Kind: operation.RunHook, - Step: operation.Queued, - Installed: true, + Hook: &hook.Info{Kind: hooks.Install}, + Kind: operation.RunHook, + Step: operation.Queued, } if err := u.unit.SetCharmURL(charmURL); err != nil { return errors.Trace(err) } } + operationExecutor, err := u.newOperationExecutor(u.paths.State.OperationsFile, initialState, u.acquireExecutionLock) if err != nil { return errors.Trace(err) From f08258523da0ae13533e6e39e8fce8429c7c83ca Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Wed, 22 Jan 2020 12:52:42 -0500 Subject: [PATCH 2/4] Add k8s ubuntu charm for install hook work. --- .../charm-repo/kubernetes/ubuntu/hooks/install | 4 ++++ testcharms/charm-repo/kubernetes/ubuntu/hooks/start | 3 +++ .../charm-repo/kubernetes/ubuntu/hooks/update-status | 4 ++++ .../charm-repo/kubernetes/ubuntu/metadata.yaml | 12 ++++++++++++ 4 files changed, 23 insertions(+) create mode 100755 testcharms/charm-repo/kubernetes/ubuntu/hooks/install create mode 100755 testcharms/charm-repo/kubernetes/ubuntu/hooks/start create mode 100755 testcharms/charm-repo/kubernetes/ubuntu/hooks/update-status create mode 100644 testcharms/charm-repo/kubernetes/ubuntu/metadata.yaml diff --git a/testcharms/charm-repo/kubernetes/ubuntu/hooks/install b/testcharms/charm-repo/kubernetes/ubuntu/hooks/install new file mode 100755 index 00000000000..7047ea7e16d --- /dev/null +++ b/testcharms/charm-repo/kubernetes/ubuntu/hooks/install @@ -0,0 +1,4 @@ +#!/bin/sh +date=`date` +status-set waiting "Hello from install, it is $date." +juju-log -l INFO "Hello from install." diff --git a/testcharms/charm-repo/kubernetes/ubuntu/hooks/start b/testcharms/charm-repo/kubernetes/ubuntu/hooks/start new file mode 100755 index 00000000000..e37c89bc565 --- /dev/null +++ b/testcharms/charm-repo/kubernetes/ubuntu/hooks/start @@ -0,0 +1,3 @@ +#!/bin/bash +application-version-set $(grep DISTRIB_RELEASE /etc/lsb-release | cut -d= -sf2) +juju-log -l INFO "Hello from start." diff --git a/testcharms/charm-repo/kubernetes/ubuntu/hooks/update-status b/testcharms/charm-repo/kubernetes/ubuntu/hooks/update-status new file mode 100755 index 00000000000..47d2d616b0e --- /dev/null +++ b/testcharms/charm-repo/kubernetes/ubuntu/hooks/update-status @@ -0,0 +1,4 @@ +#!/bin/sh +date=`date` +status-set active "Hello from update-status, it is $date." +juju-log -l INFO "Hello from update-status." diff --git a/testcharms/charm-repo/kubernetes/ubuntu/metadata.yaml b/testcharms/charm-repo/kubernetes/ubuntu/metadata.yaml new file mode 100644 index 00000000000..7bb7625d860 --- /dev/null +++ b/testcharms/charm-repo/kubernetes/ubuntu/metadata.yaml @@ -0,0 +1,12 @@ +name: ubuntu-k8s +summary: "ubuntu operating system" +description: "A popular operating system" +provides: + ubuntu: + interface: ubuntu +resources: + ubuntu_image: + type: "oci-image" + description: "Image used for ubuntu pod." +series: + - kubernetes From 0b0537a4e829c9101e2e035c1fa271f595ae87a1 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Wed, 29 Jan 2020 17:03:10 -0500 Subject: [PATCH 3/4] Rework storage resolver for CAAS based on now calling install hook. Ensure that the start hook is run for CAAS units with storage. Add tests for CAAS units, wait for started to attach storage. --- worker/uniter/resolver.go | 8 --- worker/uniter/storage/attachments_test.go | 59 +++++++++++++++++++-- worker/uniter/storage/resolver.go | 63 +++++++++++++++-------- 3 files changed, 98 insertions(+), 32 deletions(-) diff --git a/worker/uniter/resolver.go b/worker/uniter/resolver.go index 2f0ffd0a904..81e037aa288 100644 --- a/worker/uniter/resolver.go +++ b/worker/uniter/resolver.go @@ -316,11 +316,3 @@ func (s *uniterResolver) nextOp( return nil, resolver.ErrNoOperation } - -// NopResolver is a resolver that does nothing. -type NopResolver struct{} - -// The NopResolver's NextOp operation should always return the no operation error. -func (NopResolver) NextOp(resolver.LocalState, remotestate.Snapshot, operation.Factory) (operation.Operation, error) { - return nil, resolver.ErrNoOperation -} diff --git a/worker/uniter/storage/attachments_test.go b/worker/uniter/storage/attachments_test.go index 1cbb8bbf843..4d7a488de77 100644 --- a/worker/uniter/storage/attachments_test.go +++ b/worker/uniter/storage/attachments_test.go @@ -224,6 +224,20 @@ func (s *attachmentsSuite) TestAttachmentsUpdateShortCircuitDeath(c *gc.C) { } func (s *attachmentsSuite) TestAttachmentsStorage(c *gc.C) { + s.testAttachmentsStorage(c, operation.State{Kind: operation.Continue}) +} + +func (s *caasAttachmentsSuite) TestAttachmentsStorageStarted(c *gc.C) { + opState := operation.State{ + Kind: operation.RunHook, + Step: operation.Queued, + Installed: true, + Started: true, + } + s.testAttachmentsStorage(c, opState) +} + +func (s *attachmentsSuite) testAttachmentsStorage(c *gc.C, opState operation.State) { stateDir := c.MkDir() unitTag := names.NewUnitTag("mysql/0") abort := make(chan struct{}) @@ -244,9 +258,7 @@ func (s *attachmentsSuite) TestAttachmentsStorage(c *gc.C) { assertStorageTags(c, att) // Inform the resolver of an attachment. - localState := resolver.LocalState{State: operation.State{ - Kind: operation.Continue, - }} + localState := resolver.LocalState{State: opState} op, err := r.NextOp(localState, remotestate.Snapshot{ Life: params.Alive, Storage: map[names.StorageTag]remotestate.StorageSnapshot{ @@ -270,6 +282,47 @@ func (s *attachmentsSuite) TestAttachmentsStorage(c *gc.C) { c.Assert(ctx.Location(), gc.Equals, "/dev/sdb") } +func (s *caasAttachmentsSuite) TestAttachmentsStorageNotStarted(c *gc.C) { + stateDir := c.MkDir() + unitTag := names.NewUnitTag("mysql/0") + abort := make(chan struct{}) + + st := &mockStorageAccessor{ + unitStorageAttachments: func(u names.UnitTag) ([]params.StorageAttachmentId, error) { + return nil, nil + }, + } + + att, err := storage.NewAttachments(st, unitTag, stateDir, abort) + c.Assert(err, jc.ErrorIsNil) + r := storage.NewResolver(att, s.modelType) + + storageTag := names.NewStorageTag("data/0") + _, err = att.Storage(storageTag) + c.Assert(err, jc.Satisfies, errors.IsNotFound) + assertStorageTags(c, att) + + // Inform the resolver of an attachment. + localState := resolver.LocalState{State: operation.State{ + Kind: operation.RunHook, + Step: operation.Queued, + Installed: true, + Started: false, + }} + _, err = r.NextOp(localState, remotestate.Snapshot{ + Life: life.Alive, + Storage: map[names.StorageTag]remotestate.StorageSnapshot{ + storageTag: { + Kind: params.StorageKindBlock, + Life: life.Alive, + Location: "/dev/sdb", + Attached: true, + }, + }, + }, &mockOperations{}) + c.Assert(err, gc.Equals, resolver.ErrNoOperation) +} + func (s *attachmentsSuite) TestAttachmentsCommitHook(c *gc.C) { stateDir := c.MkDir() unitTag := names.NewUnitTag("mysql/0") diff --git a/worker/uniter/storage/resolver.go b/worker/uniter/storage/resolver.go index 864158375cc..b8a1b8ac314 100644 --- a/worker/uniter/storage/resolver.go +++ b/worker/uniter/storage/resolver.go @@ -65,32 +65,51 @@ func (s *storageResolver) NextOp( return nil, errors.Trace(err) } - // Only IAAS models need to first run the install hook. - // For CAAS models, storage is specified in the pod config - // and mounted as the pod in started. - blockedWaitingForInstall := !localState.Installed && s.modelType == model.IAAS - + // The decision making below with regard to when to run the storage hooks for + // the first time after a charm is deployed is applicable only to IAAS models. + // For IAAS models, we do not run install hook before any storage provisioned + // with the deployment of the unit is available. The presumption is that IAAS + // charms may need storage on which to install their workloads. + // + // For CAAS models, it's different because we need to create the pod before + // any storage is provisioned. See in-line explanation below. + // + // TODO(juju3) - allow storage hooks to run after install for IAAS models + // This will make IAAS and CAAS behaviour the same, and charms should be + // resilient to resources such as storage not being ready at any given time. var runStorageHooks bool switch { case localState.Kind == operation.Continue: // There's nothing in progress. - fallthrough - case blockedWaitingForInstall && localState.Kind == operation.RunHook && localState.Step == operation.Queued: - // The install operation completed, and there's an install - // hook queued. Run storage-attached hooks first. - fallthrough - case s.modelType == model.CAAS: - // CAAS models skip the uniter Install operation, but do run the - // install hook. No need to wait for the install hook to be run - // as storage is mounted when a pod is started. This will keep - // behavior consistent with before CAAS units run an install hook, runStorageHooks = true + case localState.Kind == operation.RunHook && localState.Step == operation.Queued: + // There's a hook queued. + switch s.modelType { + case model.CAAS: + // For CAAS models, we wait until after the start hook has run before + // running storage-attached hooks since storage is provisioned only + // after the pod has been created. + // + // NB we must be careful here. The initial hook sequence is + // install->leader-elected->config-changed->started + // This chain is activated by each preceding hook queuing the next. + // If we allow a storage-attached hook to run before the start hook, we + // will potentially overwrite the state to initiate the next hook. + // So we need to get to at least started. This means that any charm logic + // that needs storage cannot be in install or start and if in config-changed, + // needs to be deferred until storage is available. + runStorageHooks = localState.Started + case model.IAAS: + // For IAAS models, we run storage-attached hooks before install. + runStorageHooks = !localState.Installed + } } if !runStorageHooks { return nil, resolver.ErrNoOperation } - if !localState.Installed && s.storage.Pending() == 0 { + // This message is only interesting for IAAS models. + if s.modelType == model.IAAS && !localState.Installed && s.storage.Pending() == 0 { logger.Infof("initial storage attachments ready") } @@ -103,11 +122,13 @@ func (s *storageResolver) NextOp( } if s.storage.Pending() > 0 { logger.Debugf("still pending %v", s.storage.pending.SortedValues()) - if blockedWaitingForInstall { - // We only wait for pending storage before - // the install hook runs; we should not block - // other hooks from running while storage is - // being provisioned. + // For IAAS models, storage hooks are run before install. + // If the install hook has not yet run and there's still + // pending storage, we wait. We don't wait after the + // install hook has run as we should not block other + // hooks from running while new storage added post install + // is being provisioned. + if !localState.Installed && s.modelType == model.IAAS { return nil, resolver.ErrWaiting } } From 2c7f60780afc8c8dbcedee50f57d9bacf2bc6db0 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Tue, 7 Apr 2020 11:06:11 -0400 Subject: [PATCH 4/4] Change life to params, to resolve backported test fail. --- worker/uniter/storage/attachments_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/worker/uniter/storage/attachments_test.go b/worker/uniter/storage/attachments_test.go index 4d7a488de77..00b4eff26cf 100644 --- a/worker/uniter/storage/attachments_test.go +++ b/worker/uniter/storage/attachments_test.go @@ -310,11 +310,11 @@ func (s *caasAttachmentsSuite) TestAttachmentsStorageNotStarted(c *gc.C) { Started: false, }} _, err = r.NextOp(localState, remotestate.Snapshot{ - Life: life.Alive, + Life: params.Alive, Storage: map[names.StorageTag]remotestate.StorageSnapshot{ storageTag: { Kind: params.StorageKindBlock, - Life: life.Alive, + Life: params.Alive, Location: "/dev/sdb", Attached: true, },