amadan

carlos / platform Public

region HA 2b: membership reads keep the cloud's request ids, ids checked first, deadlineStore names its store

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Paul Campbell pushed by claude d508e7010e0cf49aa3d5a81776223063ff4a8227 parent a50f87a
6 files changed, +188 −27
  • internal/hostactors/awsmembership/awsmembership.go +18 −3
  • internal/hostactors/awsmembership/awsmembership_test.go +70 −0
  • internal/hostactors/membershipread.go +39 −18
  • internal/hostactors/membershipread_test.go +58 −3
  • internal/provision/plan9_b8_census_test.go +1 −1
  • internal/provision/testdata/plan14-root-reads-region-ha-2b.tsv +2 −2
diff --git a/internal/hostactors/awsmembership/awsmembership.go b/internal/hostactors/awsmembership/awsmembership.go
index d725fe901..24c7f26c7 100644
--- a/internal/hostactors/awsmembership/awsmembership.go
+++ b/internal/hostactors/awsmembership/awsmembership.go
@@ -9,9 +9,11 @@ package awsmembership
import (
"context"
+ "errors"
"github.com/aws/aws-sdk-go-v2/aws"
awsmiddleware "github.com/aws/aws-sdk-go-v2/aws/middleware"
+ awshttp "github.com/aws/aws-sdk-go-v2/aws/transport/http"
"github.com/aws/aws-sdk-go-v2/config"
"github.com/aws/aws-sdk-go-v2/service/autoscaling"
"github.com/aws/aws-sdk-go-v2/service/ec2"
@@ -34,21 +36,34 @@ func New(ctx context.Context) (*Reader, error) {
}
// Member describes instance's Auto Scaling membership, then its EC2 state
-// and owner: two calls, two request ids.
+// and owner: two calls, two request ids. A call that fails ends the read;
+// the ids of the calls made come back with the error, the failed call's
+// from the cloud's error response (failedID).
func (r *Reader) Member(ctx context.Context, awsRegion, instance string) (hostactors.Membership, error) {
asg := autoscaling.NewFromConfig(r.cfg, func(o *autoscaling.Options) { o.Region = awsRegion })
ao, err := asg.DescribeAutoScalingInstances(ctx, &autoscaling.DescribeAutoScalingInstancesInput{InstanceIds: []string{instance}})
if err != nil {
- return hostactors.Membership{}, err
+ return hostactors.Membership{RequestIDs: []string{failedID(err)}}, err
}
ec := ec2.NewFromConfig(r.cfg, func(o *ec2.Options) { o.Region = awsRegion })
eo, err := ec.DescribeInstances(ctx, &ec2.DescribeInstancesInput{Filters: []ec2types.Filter{{Name: aws.String("instance-id"), Values: []string{instance}}}})
if err != nil {
- return hostactors.Membership{}, err
+ return hostactors.Membership{RequestIDs: []string{requestID(ao.ResultMetadata), failedID(err)}}, err
}
return membershipFrom(instance, ao, eo), nil
}
+// failedID is the request id of a call that failed: the one the cloud gave
+// in its error response, or hostactors.NoRequestID when the call failed
+// before the cloud answered (no connection, a cancelled context).
+func failedID(err error) string {
+ var re *awshttp.ResponseError
+ if errors.As(err, &re) && re.ServiceRequestID() != "" {
+ return re.ServiceRequestID()
+ }
+ return hostactors.NoRequestID
+}
+
// membershipFrom reads the two replies. Anything but exactly the one
// instance asked about, in exactly one group, is no membership.
func membershipFrom(instance string, ao *autoscaling.DescribeAutoScalingInstancesOutput, eo *ec2.DescribeInstancesOutput) hostactors.Membership {
diff --git a/internal/hostactors/awsmembership/awsmembership_test.go b/internal/hostactors/awsmembership/awsmembership_test.go
index 8c2ab402d..183464251 100644
--- a/internal/hostactors/awsmembership/awsmembership_test.go
+++ b/internal/hostactors/awsmembership/awsmembership_test.go
@@ -1,7 +1,13 @@
package awsmembership
import (
+ "context"
+ "errors"
+ "fmt"
+ "io"
+ "net/http"
"reflect"
+ "strings"
"testing"
"github.com/aws/aws-sdk-go-v2/aws"
@@ -62,3 +68,67 @@ func TestZoneRegion(t *testing.T) {
}
}
}
+
+// cloudHTTP stands in for the network under the SDK: Auto Scaling and EC2
+// in one region. Each service answers its reply, an error response
+// carrying the cloud's request id, or (down) no answer at all.
+type cloudHTTP struct {
+ asg, ec2 string // "ok", "error" or "down"
+}
+
+func (h cloudHTTP) Do(r *http.Request) (*http.Response, error) {
+ mode, id, okBody, errBody := h.ec2, "ec2", `<DescribeInstancesResponse xmlns="http://ec2.amazonaws.com/doc/2016-11-15/"><requestId>ec2-req</requestId><reservationSet><item><ownerId>123</ownerId><instancesSet><item><instanceId>i-new</instanceId><instanceState><code>16</code><name>running</name></instanceState></item></instancesSet></item></reservationSet></DescribeInstancesResponse>`,
+ `<Response><Errors><Error><Code>UnauthorizedOperation</Code><Message>no</Message></Error></Errors><RequestID>ec2-err</RequestID></Response>`
+ if strings.HasPrefix(r.URL.Host, "autoscaling.") {
+ mode, id, okBody, errBody = h.asg, "asg", `<DescribeAutoScalingInstancesResponse xmlns="http://autoscaling.amazonaws.com/doc/2011-01-01/"><DescribeAutoScalingInstancesResult><AutoScalingInstances><member><InstanceId>i-new</InstanceId><AutoScalingGroupName>routers-ie</AutoScalingGroupName><AvailabilityZone>eu-west-1b</AvailabilityZone><LifecycleState>InService</LifecycleState><HealthStatus>HEALTHY</HealthStatus><ProtectedFromScaleIn>false</ProtectedFromScaleIn></member></AutoScalingInstances></DescribeAutoScalingInstancesResult><ResponseMetadata><RequestId>asg-req</RequestId></ResponseMetadata></DescribeAutoScalingInstancesResponse>`,
+ `<ErrorResponse><Error><Type>Sender</Type><Code>AccessDenied</Code><Message>no</Message></Error><RequestId>asg-err</RequestId></ErrorResponse>`
+ }
+ if r.Body != nil {
+ _, _ = io.Copy(io.Discard, r.Body)
+ }
+ if !strings.Contains(r.URL.Host, ".eu-west-1.") {
+ return nil, fmt.Errorf("a call to %s, not the attempt's AWS region", r.URL.Host)
+ }
+ resp := &http.Response{Header: http.Header{"Content-Type": {"text/xml"}}, Request: r, Proto: "HTTP/1.1", ProtoMajor: 1, ProtoMinor: 1}
+ switch mode {
+ case "down":
+ return nil, errors.New("dial tcp: connection refused")
+ case "error":
+ resp.StatusCode = http.StatusForbidden
+ resp.Header.Set("X-Amzn-Requestid", id+"-err")
+ resp.Body = io.NopCloser(strings.NewReader(errBody))
+ default:
+ resp.StatusCode = http.StatusOK
+ resp.Header.Set("X-Amzn-Requestid", id+"-req")
+ resp.Body = io.NopCloser(strings.NewReader(okBody))
+ }
+ return resp, nil
+}
+
+// Member, through the SDK: both ids on success, and on a failed read the
+// ids of the calls made, the failed call's from the cloud's error
+// response, or NoRequestID when the cloud never answered.
+func TestMemberKeepsTheIdsOfAFailedRead(t *testing.T) {
+ for name, c := range map[string]struct {
+ cloud cloudHTTP
+ ids []string
+ fails bool
+ }{
+ "both calls answer": {cloudHTTP{"ok", "ok"}, []string{"asg-req", "ec2-req"}, false},
+ "the second call fails": {cloudHTTP{"ok", "error"}, []string{"asg-req", "ec2-err"}, true},
+ "the first call fails": {cloudHTTP{"error", "ok"}, []string{"asg-err"}, true},
+ "the second call never reaches a cloud": {cloudHTTP{"ok", "down"}, []string{"asg-req", hostactors.NoRequestID}, true},
+ "the first call never reaches a cloud": {cloudHTTP{"down", "ok"}, []string{hostactors.NoRequestID}, true},
+ } {
+ t.Run(name, func(t *testing.T) {
+ r := &Reader{cfg: aws.Config{Region: "us-east-1", Credentials: aws.AnonymousCredentials{}, HTTPClient: c.cloud, Retryer: func() aws.Retryer { return aws.NopRetryer{} }}}
+ m, err := r.Member(context.Background(), "eu-west-1", "i-new")
+ if (err != nil) != c.fails || !reflect.DeepEqual(m.RequestIDs, c.ids) {
+ t.Fatalf("Member: %+v, %v; want ids %v", m, err, c.ids)
+ }
+ if !c.fails && (m.GroupName != "routers-ie" || m.InstanceState != "running" || m.Owner != "123" || m.AvailabilityZoneRegion != "eu-west-1") {
+ t.Fatalf("a member read as %+v", m)
+ }
+ })
+ }
+}
diff --git a/internal/hostactors/membershipread.go b/internal/hostactors/membershipread.go
index 442723acf..523638cf9 100644
--- a/internal/hostactors/membershipread.go
+++ b/internal/hostactors/membershipread.go
@@ -26,7 +26,9 @@ import (
type MembershipReader interface {
// Member describes instance in awsRegion: its group, if any, and its
// state. An instance in no group is a Membership with no GroupName. An
- // error is a read that failed, never an answer.
+ // error is a read that failed, never an answer; the Membership beside
+ // it then carries only the RequestIDs of the calls made, so the log
+ // keeps whatever ids the cloud gave.
Member(ctx context.Context, awsRegion, instance string) (Membership, error)
}
@@ -37,10 +39,18 @@ type Membership struct {
LifecycleState string
InstanceState string
Owner string
- // RequestIDs are the cloud's request ids for the reads, one per call.
+ // RequestIDs are the cloud's request ids for the reads, one per call
+ // made, in order: the id the cloud gave, in its answer or in its error
+ // response, or NoRequestID for a call that failed before the cloud
+ // answered.
RequestIDs []string
}
+// NoRequestID stands in a Membership's RequestIDs for a call that failed
+// before the cloud answered, so it gave no id. It is never an id: a read
+// that carries it is not an answer.
+const NoRequestID = "none"
+
// MembershipEvidence is what a confirmation read, for the console's log:
// the cloud's request ids, and when the read began.
type MembershipEvidence struct {
@@ -94,10 +104,22 @@ func confirmMembership(ctx context.Context, r MembershipReader, region Region, i
}
at := now().UTC()
m, err := r.Member(ctx, region.AWSRegion, instance)
+ // The ids the cloud gave are kept whatever came of the read, failed
+ // calls included, for the console's log.
+ c := membershipConfirmation{requestIDs: append([]string(nil), m.RequestIDs...), observedAt: at}
if err != nil {
- return membershipConfirmation{observedAt: at}, fmt.Errorf("%w: %v", ErrMembershipUnknown, err)
+ return c, fmt.Errorf("%w: %v", ErrMembershipUnknown, err)
+ }
+ // A reply without an id from each call is not an answer, whatever it
+ // claims, so it is never counted against the attempt.
+ if len(m.RequestIDs) < 2 {
+ return c, fmt.Errorf("%w: the cloud gave no request ids", ErrMembershipUnknown)
+ }
+ for _, id := range m.RequestIDs {
+ if id == "" || id == NoRequestID {
+ return c, fmt.Errorf("%w: the cloud gave a blank request id", ErrMembershipUnknown)
+ }
}
- c := membershipConfirmation{requestIDs: append([]string(nil), m.RequestIDs...), observedAt: at}
switch {
case m.GroupName != region.GroupName:
return c, fmt.Errorf("%w: it is in group %q", ErrNotMember, m.GroupName)
@@ -111,13 +133,6 @@ func confirmMembership(ctx context.Context, r MembershipReader, region Region, i
return c, fmt.Errorf("%w: it is %q", ErrNotMember, m.InstanceState)
case m.Owner != region.Account || m.AvailabilityZoneRegion != region.AWSRegion:
return c, fmt.Errorf("%w: it is in account %q, region %q", ErrNotMember, m.Owner, m.AvailabilityZoneRegion)
- case len(m.RequestIDs) < 2:
- return c, fmt.Errorf("%w: the cloud gave no request ids", ErrMembershipUnknown)
- }
- for _, id := range m.RequestIDs {
- if id == "" {
- return c, fmt.Errorf("%w: the cloud gave a blank request id", ErrMembershipUnknown)
- }
}
return c, nil
}
@@ -160,7 +175,7 @@ func (c membershipConfirmation) evidence() MembershipEvidence {
func writeBound(ctx context.Context, d Directory, c membershipConfirmation, now func() time.Time) (Directory, context.Context, context.CancelFunc) {
bctx, cancel := context.WithTimeout(ctx, c.deadline().Sub(now()))
bd := d
- bd.Store = deadlineStore{Store: d.Store, check: func(callCtx context.Context) error {
+ bd.Store = deadlineStore{inner: d.Store, check: func(callCtx context.Context) error {
if bctx.Err() != nil || callCtx.Err() != nil {
return fmt.Errorf("%w: the deadline passed", ErrWriteDeadline)
}
@@ -186,11 +201,17 @@ var storeCallTimeout = 15 * time.Second
// ErrWriteDeadline, unknown: a call cut off in flight, one that never
// answered, and one that answered, even successfully, only after the
// deadline.
+//
+// The store underneath is a named field, not embedded, and every method of
+// release.Store is written out here: a method added to the interface later
+// fails to compile instead of reaching the store past the bound.
type deadlineStore struct {
- release.Store
+ inner release.Store
check func(context.Context) error
}
+var _ release.Store = deadlineStore{}
+
// call runs one store call under the bound.
func (s deadlineStore) call(ctx context.Context, f func(context.Context) error) error {
if err := s.check(ctx); err != nil {
@@ -227,7 +248,7 @@ func (s deadlineStore) Get(ctx context.Context, key string) ([]byte, string, err
var etag string
err := s.call(ctx, func(cctx context.Context) error {
var err error
- body, etag, err = s.Store.Get(cctx, key)
+ body, etag, err = s.inner.Get(cctx, key)
return err
})
if err != nil {
@@ -240,7 +261,7 @@ func (s deadlineStore) Put(ctx context.Context, key string, body []byte, ifMatch
var etag string
err := s.call(ctx, func(cctx context.Context) error {
var err error
- etag, err = s.Store.Put(cctx, key, body, ifMatch)
+ etag, err = s.inner.Put(cctx, key, body, ifMatch)
return err
})
if err != nil {
@@ -253,7 +274,7 @@ func (s deadlineStore) List(ctx context.Context, prefix string) ([]string, error
var keys []string
err := s.call(ctx, func(cctx context.Context) error {
var err error
- keys, err = s.Store.List(cctx, prefix)
+ keys, err = s.inner.List(cctx, prefix)
return err
})
if err != nil {
@@ -266,7 +287,7 @@ func (s deadlineStore) ListDirs(ctx context.Context, prefix string) ([]string, e
var dirs []string
err := s.call(ctx, func(cctx context.Context) error {
var err error
- dirs, err = s.Store.ListDirs(cctx, prefix)
+ dirs, err = s.inner.ListDirs(cctx, prefix)
return err
})
if err != nil {
@@ -276,5 +297,5 @@ func (s deadlineStore) ListDirs(ctx context.Context, prefix string) ([]string, e
}
func (s deadlineStore) Delete(ctx context.Context, key string) error {
- return s.call(ctx, func(cctx context.Context) error { return s.Store.Delete(cctx, key) })
+ return s.call(ctx, func(cctx context.Context) error { return s.inner.Delete(cctx, key) })
}
diff --git a/internal/hostactors/membershipread_test.go b/internal/hostactors/membershipread_test.go
index 947111dea..18e8d1bdf 100644
--- a/internal/hostactors/membershipread_test.go
+++ b/internal/hostactors/membershipread_test.go
@@ -17,11 +17,13 @@ import (
)
// fakeMembership is the cloud, as the console's reader sees it: each
-// instance's membership. err fails every call; during runs inside each
-// call (a slow describe, say).
+// instance's membership. err fails every call, with failIDs the request
+// ids of the calls made; during runs inside each call (a slow describe,
+// say).
type fakeMembership struct {
members map[string]Membership
err error
+ failIDs []string
during func()
calls []string
}
@@ -32,7 +34,7 @@ func (f *fakeMembership) Member(_ context.Context, awsRegion, instance string) (
f.during()
}
if f.err != nil {
- return Membership{}, f.err
+ return Membership{RequestIDs: f.failIDs}, f.err
}
return f.members[instance], nil
}
@@ -156,14 +158,67 @@ func TestAMembershipReadThatIsNotAnAnswerIsUnknown(t *testing.T) {
m.RequestIDs = []string{"a", ""}
f.members["i-new"] = m
},
+ // A call that failed before the cloud answered has no id, so the
+ // reply is not an answer however it got this far.
+ "no id from a call": func(f *fakeMembership) {
+ m := f.members["i-new"]
+ m.RequestIDs = []string{"a", NoRequestID}
+ f.members["i-new"] = m
+ },
+ // The ids are checked before anything the reply claims: another
+ // group with one id is still no answer, not a refusal.
+ "another group, one request id": func(f *fakeMembership) {
+ m := f.members["i-new"]
+ m.GroupName, m.RequestIDs = "routers-uk", []string{"a"}
+ f.members["i-new"] = m
+ },
+ "no group and no ids": func(f *fakeMembership) {
+ f.members["i-new"] = Membership{}
+ },
} {
t.Run(name, func(t *testing.T) {
w := newMemberWorld(t, member("routers-ie"))
edit(w.cloud)
+ if _, _, err := w.d.CheckConfirmed(context.Background(), w.o, w.v, w.in, w.raw, "i-new", w.cloud, w.now); !errors.Is(err, ErrMembershipUnknown) || errors.Is(err, ErrNotMember) {
+ t.Fatalf("check: %v", err)
+ }
_, _, err := w.enroll()
if !errors.Is(err, ErrMembershipUnknown) || errors.Is(err, ErrNotMember) {
t.Fatalf("%v", err)
}
+ // The attempt is kept as it was, and nothing is written.
+ o, err := w.d.Read(context.Background())
+ a := o.Roster.EnrollmentAttempts[newRef]
+ if err != nil || o.Roster.Revision != w.o.Roster.Revision || a.Consumed || a.AttemptID != w.in.AttemptID || a.ConfirmationID != w.in.ConfirmationID {
+ t.Fatalf("an unknown read moved the attempt: %v, %+v", err, a)
+ }
+ })
+ }
+}
+
+// A read that failed still logs the ids the cloud gave: the call that
+// answered, and the failed call's own id from its error response.
+func TestAFailedReadKeepsTheIdsTheCloudGave(t *testing.T) {
+ for name, ids := range map[string][]string{
+ "the second call fails": {"asg-req", "ec2-err"},
+ "the first call fails": {"asg-err"},
+ "a call fails before the cloud answers": {"asg-req", NoRequestID},
+ } {
+ t.Run(name, func(t *testing.T) {
+ w := newMemberWorld(t, member("routers-ie"))
+ w.cloud.err, w.cloud.failIDs = errors.New("UnauthorizedOperation"), ids
+ _, ev, err := w.d.CheckConfirmed(context.Background(), w.o, w.v, w.in, w.raw, "i-new", w.cloud, w.now)
+ if !errors.Is(err, ErrMembershipUnknown) || !reflect.DeepEqual(ev.RequestIDs, ids) || !ev.ObservedAt.Equal(w.clock) {
+ t.Fatalf("check: %v, evidence %+v", err, ev)
+ }
+ _, ev, err = w.enroll()
+ if !errors.Is(err, ErrMembershipUnknown) || !reflect.DeepEqual(ev.RequestIDs, ids) {
+ t.Fatalf("enroll: %v, evidence %+v", err, ev)
+ }
+ c, err := confirmMembership(context.Background(), w.cloud, w.o.Roster.Regions["ie"], "i-new", w.now)
+ if !errors.Is(err, ErrMembershipUnknown) || !reflect.DeepEqual(c.requestIDs, ids) {
+ t.Fatalf("confirmMembership: %v, %v", err, c.requestIDs)
+ }
})
}
}
diff --git a/internal/provision/plan9_b8_census_test.go b/internal/provision/plan9_b8_census_test.go
index 280792462..6ef43ddcf 100644
--- a/internal/provision/plan9_b8_census_test.go
+++ b/internal/provision/plan9_b8_census_test.go
@@ -818,7 +818,7 @@ var plan9B8CensusInventory = []censusEntry{
// deadline; it forwards Put, refuses it once the deadline passes, gives
// it at most storeCallTimeout, and reports one that answers late as
// unknown.
- {"internal/hostactors/membershipread.go", "deadlineStore.Put", "nonrecord", "def:Put,sel:Put", "848f2b529d87e34f128dd481f14213e1cfaf8c54555806e6314864ef9a2dbb14"},
+ {"internal/hostactors/membershipread.go", "deadlineStore.Put", "nonrecord", "def:Put,sel:Put", "13b4bb3bcdd74076f4c1edece9b888fb8889fe3de459c468d61336240a42b685"},
{"internal/hostbox/regionalworker.go", "RegionalWorker.Census", "key-site", "lit:instances", "477bcb896b5ae8ec80af592d68eceb7992ad8a23cd6cff6043ad03b3ef2a8085"},
{"internal/hostbox/requeststore.go", "RequestStore.CopyVersion", "versioned-transport", "def:CopyVersion,sel:CopyVersion", "3a50049ce7ca7f2cbc770bef46218944cdc7000205a86c20e4932f0b510da28f"},
{"internal/hostbox/requeststore.go", "RequestStore.PutIfAbsent", "versioned-transport", "def:PutIfAbsent,sel:PutIfAbsent", "3666cbe5d2b91c1c652ffb4459c10a374fbd92e325e4e5c574fa3525d0ce2ffe"},
diff --git a/internal/provision/testdata/plan14-root-reads-region-ha-2b.tsv b/internal/provision/testdata/plan14-root-reads-region-ha-2b.tsv
index 4747fe0eb..8a254cd2f 100644
--- a/internal/provision/testdata/plan14-root-reads-region-ha-2b.tsv
+++ b/internal/provision/testdata/plan14-root-reads-region-ha-2b.tsv
@@ -12,5 +12,5 @@ internal/host/installchannel.go channelInstall.run authority F,J4 rev3=census-ru
internal/host/installchannel_linux.go slotState authority F,J4 rev3=census-ruling-region-ha-2b;basis=carlos system install-channel (root, Linux): opens one protected slot with O_NOFOLLOW (a symlink is never followed), fstats the descriptor and hashes from it, to compare with the sha256 of the signed, verified artifact; a slot holding anything else, a symlink or a non-regular file is refused and left as it is, and nothing in it is executed file:Open@66,file:Stat@388 bc533a29f92ae95d31123b551354ab9f8960576e5ffef1bd6a74a06df0b57a24
internal/host/installchannel_linux.go var openat2 authority F,J4 rev3=census-ruling-region-ha-2b;basis=carlos system install-channel (root, Linux): unix.Openat2 held in a variable so a test can make it fail; removeBeneath is its only user and has no path-based fallback when it fails file:Openat2@14 6081e86f758f4ae3948ec2d4f4c037888e1362f122a410c110e31290ba914816
internal/host/installchannel_linux.go removeBeneath authority F,J4 rev3=census-ruling-region-ha-2b;basis=carlos system install-channel (root, Linux): removes the agent's cache entry for the version, after a run that made it, only through the router directory held by descriptor (opened with no symlink anywhere on its path); its version directory is opened beneath it with RESOLVE_BENEATH|RESOLVE_NO_SYMLINKS and everything under it is unlinked by descriptor; a symlink is unlinked, never entered; when openat2 is unavailable or refuses, the removal refuses and leaves the tree, with no path-based fallback; its one read site lists the held directory's names file:Readdirnames@381 42732bb7b400abdb7f18ddbf5aacdfadc9496d8941e774dc01a471bf15c35f69
-@coverage internal/hostactors f6dbcea46bed878257c5b34f8caeef83e71cc995cae7b3a4ae59339ed0740c68
-@coverage internal/hostactors/awsmembership a3defad7179245492a3c1dcfce5e20f0f85db2e240b474c654acf211da16df98
+@coverage internal/hostactors be1ff5b27d88362d238ddd45d900a3ea9f6e4cbd3d6b9e5e6047b48a1ebca1c5
+@coverage internal/hostactors/awsmembership ff967730ddb8f17acb73e3351590ba11695e82341b195539da6fbd856ac39175