Merge pull request #13538 from hpidcock/fix-storage-id

#13538

Storage hooks don't include the storage hook context which makes it difficult for operator framework
to give useful abstractions.

This PR adds JUJU_STORAGE_ID JUJU_STORAGE_KIND and JUJU_STORAGE_LOCATION to the hook
env when storage hooks are run.

Example values:
```
JUJU_STORAGE_ID=data/0
JUJU_STORAGE_KIND=filesystem
JUJU_STORAGE_LOCATION=/data
```

## QA steps

Using https://github.com/hpidcock/testing-charms/tree/master/simpletest

```
$ juju bootstrap localhost
$ juju deploy ~/projects/testing-charms/simpletest --storage data=1G
$ juju show-status-log simpletest/0
Time Type Status Message
01 Dec 2021 14:35:29+10:00 juju-unit allocating
01 Dec 2021 14:35:29+10:00 workload waiting waiting for machine
01 Dec 2021 14:36:49+10:00 workload waiting installing agent
01 Dec 2021 14:36:50+10:00 workload waiting agent initializing
01 Dec 2021 14:36:50+10:00 juju-unit executing running data-storage-attached hook
01 Dec 2021 14:36:50+10:00 workload maintenance Storage attached for data data/0 filesystem /data
01 Dec 2021 14:36:50+10:00 juju-unit executing running install hook
01 Dec 2021 14:36:50+10:00 workload maintenance Installing thing
01 Dec 2021 14:36:51+10:00 juju-unit executing running leader-elected hook
01 Dec 2021 14:36:51+10:00 juju-unit executing running config-changed hook
01 Dec 2021 14:36:51+10:00 juju-unit executing running start hook
01 Dec 2021 14:36:51+10:00 workload maintenance Starting thing
01 Dec 2021 14:36:51+10:00 juju-unit idle
```

## Documentation changes

Need to add JUJU_STORAGE_ID JUJU_STORAGE_KIND and JUJU_STORAGE_LOCATION to hook env docs.

## Bug reference

https://bugs.launchpad.net/juju/+bug/1948228

worker/uniter/runner/context/context.go

@@ -710,6 +710,9 @@ func (ctx *HookContext) HookStorage() (jujuc.ContextStorageAttachment, error) {
 // available to the context.
 // Implements jujuc.HookContext.ContextStorage, part of runner.Context.
 func (ctx *HookContext) Storage(tag names.StorageTag) (jujuc.ContextStorageAttachment, error) {
+	if ctx.storage == nil {
+		return nil, errors.NotFoundf("storage %s", tag)
+	}
 	return ctx.storage.Storage(tag)
 }
 
@@ -1129,6 +1132,16 @@ func (ctx *HookContext) HookVars(
 		)
 	}
 
+	if storage, err := ctx.HookStorage(); err == nil {
+		vars = append(vars,
+			"JUJU_STORAGE_ID="+storage.Tag().Id(),
+			"JUJU_STORAGE_LOCATION="+storage.Location(),
+			"JUJU_STORAGE_KIND="+storage.Kind().String(),
+		)
+	} else if !errors.IsNotFound(err) {
+		return nil, errors.Trace(err)
+	}
+
 	return append(vars, OSDependentEnvVars(paths, env)...), nil
 }
 

worker/uniter/runner/context/context_test.go

@@ -40,40 +40,48 @@ type InterfaceSuite struct {
 var _ = gc.Suite(&InterfaceSuite{})
 
 func (s *InterfaceSuite) TestUnitName(c *gc.C) {
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	c.Assert(ctx.UnitName(), gc.Equals, "u/0")
 }
 
 func (s *InterfaceSuite) TestHookRelation(c *gc.C) {
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	r, err := ctx.HookRelation()
 	c.Assert(err, jc.Satisfies, errors.IsNotFound)
 	c.Assert(r, gc.IsNil)
 }
 
+func (s *InterfaceSuite) TestHookStorage(c *gc.C) {
+	ctx := s.GetContext(c, -1, "", names.NewStorageTag("data/0"))
+	storage, err := ctx.HookStorage()
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(storage, gc.NotNil)
+	c.Assert(storage.Tag().Id(), gc.Equals, "data/0")
+}
+
 func (s *InterfaceSuite) TestRemoteUnitName(c *gc.C) {
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	name, err := ctx.RemoteUnitName()
 	c.Assert(err, jc.Satisfies, errors.IsNotFound)
 	c.Assert(name, gc.Equals, "")
 }
 
 func (s *InterfaceSuite) TestRemoteApplicationName(c *gc.C) {
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	name, err := ctx.RemoteApplicationName()
 	c.Assert(err, jc.Satisfies, errors.IsNotFound)
 	c.Assert(name, gc.Equals, "")
 }
 
 func (s *InterfaceSuite) TestWorkloadName(c *gc.C) {
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	name, err := ctx.WorkloadName()
 	c.Assert(err, jc.Satisfies, errors.IsNotFound)
 	c.Assert(name, gc.Equals, "")
 }
 
 func (s *InterfaceSuite) TestRelationIds(c *gc.C) {
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	relIds, err := ctx.RelationIds()
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(relIds, gc.HasLen, 2)
@@ -87,30 +95,30 @@ func (s *InterfaceSuite) TestRelationIds(c *gc.C) {
 }
 
 func (s *InterfaceSuite) TestRelationContext(c *gc.C) {
-	ctx := s.GetContext(c, 1, "")
+	ctx := s.GetContext(c, 1, "", names.StorageTag{})
 	r, err := ctx.HookRelation()
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(r.Name(), gc.Equals, "db")
 	c.Assert(r.FakeId(), gc.Equals, "db:1")
 }
 
 func (s *InterfaceSuite) TestRelationContextWithRemoteUnitName(c *gc.C) {
-	ctx := s.GetContext(c, 1, "u/123")
+	ctx := s.GetContext(c, 1, "u/123", names.StorageTag{})
 	name, err := ctx.RemoteUnitName()
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(name, gc.Equals, "u/123")
 }
 
 func (s *InterfaceSuite) TestAddingMetricsInWrongContext(c *gc.C) {
-	ctx := s.GetContext(c, 1, "u/123")
+	ctx := s.GetContext(c, 1, "u/123", names.StorageTag{})
 	err := ctx.AddMetric("key", "123", time.Now())
 	c.Assert(err, gc.ErrorMatches, "metrics not allowed in this context")
 	err = ctx.AddMetricLabels("key", "123", time.Now(), map[string]string{"foo": "bar"})
 	c.Assert(err, gc.ErrorMatches, "metrics not allowed in this context")
 }
 
 func (s *InterfaceSuite) TestAvailabilityZone(c *gc.C) {
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	zone, err := ctx.AvailabilityZone()
 	c.Check(err, jc.ErrorIsNil)
 	c.Check(zone, gc.Equals, "a-zone")
@@ -120,7 +128,7 @@ func (s *InterfaceSuite) TestUnitNetworkInfo(c *gc.C) {
 	// Only the error case is tested to ensure end-to-end integration, the rest
 	// of the cases are tested separately for network-get, api/uniter, and
 	// apiserver/uniter, respectively.
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	netInfo, err := ctx.NetworkInfo([]string{"unknown"}, -1)
 	c.Check(err, jc.ErrorIsNil)
 	c.Check(netInfo, gc.DeepEquals, map[string]params.NetworkInfoResult{
@@ -134,7 +142,7 @@ func (s *InterfaceSuite) TestUnitNetworkInfo(c *gc.C) {
 }
 
 func (s *InterfaceSuite) TestUnitStatus(c *gc.C) {
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	defer context.PatchCachedStatus(ctx.(context.Context), "maintenance", "working", map[string]interface{}{"hello": "world"})()
 	status, err := ctx.UnitStatus()
 	c.Check(err, jc.ErrorIsNil)
@@ -144,7 +152,7 @@ func (s *InterfaceSuite) TestUnitStatus(c *gc.C) {
 }
 
 func (s *InterfaceSuite) TestSetUnitStatus(c *gc.C) {
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	status := jujuc.StatusInfo{
 		Status: "maintenance",
 		Info:   "doing work",
@@ -159,7 +167,7 @@ func (s *InterfaceSuite) TestSetUnitStatus(c *gc.C) {
 }
 
 func (s *InterfaceSuite) TestSetUnitStatusUpdatesFlag(c *gc.C) {
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	c.Assert(ctx.(context.Context).HasExecutionSetUnitStatus(), jc.IsFalse)
 	status := jujuc.StatusInfo{
 		Status: "maintenance",
@@ -171,7 +179,7 @@ func (s *InterfaceSuite) TestSetUnitStatusUpdatesFlag(c *gc.C) {
 }
 
 func (s *InterfaceSuite) TestGetSetWorkloadVersion(c *gc.C) {
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	// No workload version set yet.
 	result, err := ctx.UnitWorkloadVersion()
 	c.Assert(result, gc.Equals, "")
@@ -186,7 +194,7 @@ func (s *InterfaceSuite) TestGetSetWorkloadVersion(c *gc.C) {
 }
 
 func (s *InterfaceSuite) TestUnitStatusCaching(c *gc.C) {
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	unitStatus, err := ctx.UnitStatus()
 	c.Check(err, jc.ErrorIsNil)
 	c.Check(unitStatus.Status, gc.Equals, "waiting")
@@ -212,7 +220,7 @@ func (s *InterfaceSuite) TestUnitStatusCaching(c *gc.C) {
 }
 
 func (s *InterfaceSuite) TestUnitCaching(c *gc.C) {
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	pr, err := ctx.PrivateAddress()
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(pr, gc.Equals, "u-0.testing.invalid")
@@ -238,7 +246,7 @@ func (s *InterfaceSuite) TestUnitCaching(c *gc.C) {
 }
 
 func (s *InterfaceSuite) TestConfigCaching(c *gc.C) {
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	settings, err := ctx.ConfigSettings()
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(settings, gc.DeepEquals, charm.Settings{"blog-title": "My Title"})
@@ -287,7 +295,7 @@ func (s *InterfaceSuite) TestGoalState(c *gc.C) {
 		},
 	}
 
-	ctx := s.GetContext(c, -1, "")
+	ctx := s.GetContext(c, -1, "", names.StorageTag{})
 	goalState, err := ctx.GoalState()
 
 	// Mock status Since string
@@ -358,7 +366,7 @@ func (s *InterfaceSuite) TestUpdateActionResults(c *gc.C) {
 
 	for i, t := range tests {
 		c.Logf("UpdateActionResults test %d: %#v: %#v", i, t.keys, t.value)
-		hctx := s.getHookContext(c, s.State.ModelUUID(), -1, "")
+		hctx := s.getHookContext(c, s.State.ModelUUID(), -1, "", names.StorageTag{})
 		context.WithActionContext(hctx, t.initial, nil)
 		err := hctx.UpdateActionResults(t.keys, t.value)
 		c.Assert(err, jc.ErrorIsNil)
@@ -370,7 +378,7 @@ func (s *InterfaceSuite) TestUpdateActionResults(c *gc.C) {
 
 // TestSetActionFailed ensures SetActionFailed works properly.
 func (s *InterfaceSuite) TestSetActionFailed(c *gc.C) {
-	hctx := s.getHookContext(c, s.State.ModelUUID(), -1, "")
+	hctx := s.getHookContext(c, s.State.ModelUUID(), -1, "", names.StorageTag{})
 	context.WithActionContext(hctx, nil, nil)
 	err := hctx.SetActionFailed()
 	c.Assert(err, jc.ErrorIsNil)
@@ -381,7 +389,7 @@ func (s *InterfaceSuite) TestSetActionFailed(c *gc.C) {
 
 // TestSetActionMessage ensures SetActionMessage works properly.
 func (s *InterfaceSuite) TestSetActionMessage(c *gc.C) {
-	hctx := s.getHookContext(c, s.State.ModelUUID(), -1, "")
+	hctx := s.getHookContext(c, s.State.ModelUUID(), -1, "", names.StorageTag{})
 	context.WithActionContext(hctx, nil, nil)
 	err := hctx.SetActionMessage("because reasons")
 	c.Assert(err, jc.ErrorIsNil)
@@ -410,7 +418,7 @@ func (s *InterfaceSuite) TestLogActionMessage(c *gc.C) {
 	_, err = action.Begin()
 	c.Assert(err, jc.ErrorIsNil)
 
-	hctx := s.getHookContext(c, s.State.ModelUUID(), -1, "")
+	hctx := s.getHookContext(c, s.State.ModelUUID(), -1, "", names.StorageTag{})
 	context.WithActionContext(hctx, nil, nil)
 	err = hctx.LogActionMessage("hello world")
 	c.Assert(err, jc.ErrorIsNil)
@@ -427,7 +435,7 @@ func (s *InterfaceSuite) TestRequestRebootAfterHook(c *gc.C) {
 		killed = true
 		return nil
 	}}
-	ctx := s.GetContext(c, -1, "").(*context.HookContext)
+	ctx := s.GetContext(c, -1, "", names.StorageTag{}).(*context.HookContext)
 	ctx.SetProcess(p)
 	err := ctx.RequestReboot(jujuc.RebootAfterHook)
 	c.Assert(err, jc.ErrorIsNil)
@@ -437,7 +445,7 @@ func (s *InterfaceSuite) TestRequestRebootAfterHook(c *gc.C) {
 }
 
 func (s *InterfaceSuite) TestRequestRebootNow(c *gc.C) {
-	ctx := s.GetContext(c, -1, "").(*context.HookContext)
+	ctx := s.GetContext(c, -1, "", names.StorageTag{}).(*context.HookContext)
 
 	var stub testing.Stub
 	var p *mockProcess
@@ -462,7 +470,7 @@ func (s *InterfaceSuite) TestRequestRebootNow(c *gc.C) {
 }
 
 func (s *InterfaceSuite) TestRequestRebootNowTimeout(c *gc.C) {
-	ctx := s.GetContext(c, -1, "").(*context.HookContext)
+	ctx := s.GetContext(c, -1, "", names.StorageTag{}).(*context.HookContext)
 
 	var advanced bool
 	var p *mockProcess

worker/uniter/runner/context/env_test.go

@@ -18,9 +18,11 @@ import (
 
 	jujuos "github.com/juju/juju/core/os"
 	"github.com/juju/juju/core/secrets"
+	"github.com/juju/juju/storage"
 	"github.com/juju/juju/testing"
 	jujuversion "github.com/juju/juju/version"
 	"github.com/juju/juju/worker/uniter/runner/context"
+	runnertesting "github.com/juju/juju/worker/uniter/runner/testing"
 )
 
 type EnvSuite struct {
@@ -156,6 +158,24 @@ func (s *EnvSuite) setDepartingRelation(ctx *context.HookContext) (expectVars []
 	}
 }
 
+func (s *EnvSuite) setStorage(ctx *context.HookContext) (expectVars []string) {
+	tag := names.NewStorageTag("data/0")
+	context.SetEnvironmentHookContextStorage(ctx, &runnertesting.StorageContextAccessor{
+		map[names.StorageTag]*runnertesting.ContextStorage{
+			tag: {
+				tag,
+				storage.StorageKindBlock,
+				"/dev/sdb",
+			},
+		},
+	}, tag)
+	return []string{
+		"JUJU_STORAGE_ID=data/0",
+		"JUJU_STORAGE_KIND=block",
+		"JUJU_STORAGE_LOCATION=/dev/sdb",
+	}
+}
+
 func (s *EnvSuite) TestEnvSetsPath(c *gc.C) {
 	paths := context.OSDependentEnvVars(MockEnvPaths{}, context.NewHostEnvironmenter())
 	c.Assert(paths, gc.Not(gc.HasLen), 0)
@@ -206,9 +226,10 @@ func (s *EnvSuite) TestEnvWindows(c *gc.C) {
 
 	relationVars := s.setRelation(ctx)
 	secretVars := s.setSecret(ctx)
+	storageVars := s.setStorage(ctx)
 	actualVars, err = ctx.HookVars(paths, false, environmenter)
 	c.Assert(err, jc.ErrorIsNil)
-	s.assertVars(c, actualVars, contextVars, pathsVars, windowsVars, relationVars, secretVars)
+	s.assertVars(c, actualVars, contextVars, pathsVars, windowsVars, relationVars, secretVars, storageVars)
 }
 
 func (s *EnvSuite) TestEnvUbuntu(c *gc.C) {
@@ -256,9 +277,11 @@ func (s *EnvSuite) TestEnvUbuntu(c *gc.C) {
 		s.assertVars(c, actualVars, contextVars, pathsVars, ubuntuVars)
 
 		relationVars := s.setDepartingRelation(ctx)
+		secretVars := s.setSecret(ctx)
+		storageVars := s.setStorage(ctx)
 		actualVars, err = ctx.HookVars(paths, false, environmenter)
 		c.Assert(err, jc.ErrorIsNil)
-		s.assertVars(c, actualVars, contextVars, pathsVars, ubuntuVars, relationVars)
+		s.assertVars(c, actualVars, contextVars, pathsVars, ubuntuVars, relationVars, secretVars, storageVars)
 	}
 }
 

worker/uniter/runner/context/export_test.go

@@ -34,6 +34,8 @@ type HookContextParams struct {
 	CharmMetrics        *charm.Metrics
 	ActionData          *ActionData
 	AssignedMachineTag  names.MachineTag
+	Storage             StorageContextAccessor
+	StorageTag          names.StorageTag
 	Paths               Paths
 	Clock               Clock
 }
@@ -54,6 +56,8 @@ func NewHookContext(hcParams HookContextParams) (*HookContext, error) {
 		jujuProxySettings:   hcParams.JujuProxySettings,
 		actionData:          hcParams.ActionData,
 		assignedMachineTag:  hcParams.AssignedMachineTag,
+		storage:             hcParams.Storage,
+		storageTag:          hcParams.StorageTag,
 		clock:               hcParams.Clock,
 		logger:              loggo.GetLogger("test"),
 	}
@@ -135,6 +139,13 @@ func SetEnvironmentHookContextRelation(context *HookContext, relationId int, end
 	context.departingUnitName = departingUnitName
 }
 
+// SetEnvironmentHookContextStorage exists purely to set the fields used in hookVars.
+// It makes no assumptions about the validity of context.
+func SetEnvironmentHookContextStorage(context *HookContext, storage StorageContextAccessor, storageTag names.StorageTag) {
+	context.storage = storage
+	context.storageTag = storageTag
+}
+
 func PatchCachedStatus(ctx jujuc.Context, status, info string, data map[string]interface{}) func() {
 	hctx := ctx.(*HookContext)
 	oldStatus := hctx.status

worker/uniter/runner/context/flush_test.go

@@ -5,6 +5,7 @@ package context_test
 
 import (
 	"github.com/juju/errors"
+	"github.com/juju/names/v4"
 	"github.com/juju/testing"
 	jc "github.com/juju/testing/checkers"
 	"github.com/juju/utils/v2"
@@ -309,7 +310,7 @@ func (s *FlushContextSuite) TestRunHookAddUnitStorageOnSuccess(c *gc.C) {
 func (s *HookContextSuite) context(c *gc.C) *context.HookContext {
 	uuid, err := utils.NewUUID()
 	c.Assert(err, jc.ErrorIsNil)
-	return s.getHookContext(c, uuid.String(), -1, "")
+	return s.getHookContext(c, uuid.String(), -1, "", names.StorageTag{})
 }
 
 func (s *FlushContextSuite) TestBuiltinMetricNotGeneratedIfNotDefined(c *gc.C) {

worker/uniter/runner/context/unitStorage_test.go

@@ -215,7 +215,7 @@ func makeStorageCons(pool string, size, count uint64) state.StorageConstraints {
 
 func (s *unitStorageSuite) addUnitStorage(c *gc.C, cons ...map[string]params.StorageConstraints) *context.HookContext {
 	// Get the context.
-	ctx := s.getHookContext(c, s.State.ModelUUID(), -1, "")
+	ctx := s.getHookContext(c, s.State.ModelUUID(), -1, "", names.StorageTag{})
 	c.Assert(ctx.UnitName(), gc.Equals, s.unit.Name())
 
 	for _, one := range cons {