Skip to content

Commit

Permalink
Merge #54544 #54812
Browse files Browse the repository at this point in the history
54544: kvserver: add assertions for invariants around liveness records r=irfansharif a=irfansharif

Now that we have #53842, we maintain the invariant that there always
exists a liveness record for any given node. We can now simplify our
handling of liveness records internally: where previously we had code to
handle the possibility of empty liveness records (we created a new one
on the fly), we can change them to assertions to verify that's no longer
possible.

When retrieving the liveness record from our in-memory cache,
it's possible for us to not find anything due to gossip delays. Instead
of simply giving up then, now we can read the records directly from KV
(and update our caches while in the area). This PR introduces this
mechanism through usage of `getLivenessRecordFromKV`.

Finally, as a bonus, this PR also surfaces a better error when trying to
decommission non-existent nodes. We're able to do this because now we
can always assume that a missing liveness record, as seen in the
decommission codepath, implies that the user is trying to decommission a
non-existent node.

---

We don't intend to backport this to 20.2 due to the hazard described in
\#54216. We want this PR to bake on master and (possibly) trip up the
assertions added above if we've missed anything. They're the only ones
checking for the invariant we've introduced around liveness records.
That invariant will be depended on for long running migrations, so
better to shake things out early.

Release note: None


54812: docker: Base the docker image on RedHat UBI r=bdarnell,DuskEagle a=jlinder

Before: The docker image was based on Debian 9.12 slim.

Why: This change will help on-prem customers from a security and
compliance perspective. It also aligns with our publishing images into
the RedHat Marketplace.

Now: Published docker images are based on the RedHat UBI 8 base image.

Fixes: #49643

Release note (backward-incompatible change): CockroachDB Docker images
are now based on the RedHat ubi8/ubi base image instead of Debian 9.12
slim. This will help on-prem customers from a security and compliance
perspective.

Co-authored-by: irfan sharif <[email protected]>
Co-authored-by: James H. Linder <[email protected]>
  • Loading branch information
3 people committed Sep 30, 2020
3 parents 445ced1 + 785aea7 + 24f5b76 commit 1ac75dd
Show file tree
Hide file tree
Showing 17 changed files with 338 additions and 283 deletions.
18 changes: 8 additions & 10 deletions build/deploy/Dockerfile
Original file line number Diff line number Diff line change
@@ -1,28 +1,26 @@
FROM debian:9.12-slim
FROM registry.access.redhat.com/ubi8/ubi

# For deployment, we need
# libc6 - dynamically linked by cockroach binary
# For deployment, we need the following installed (they are installed
# by default in RedHat UBI standard):
# glibc - dynamically linked by cockroach binary
# ca-certificates - to authenticate TLS connections for telemetry and
# bulk-io with S3/GCS/Azure
# tzdata - for time zone functions
RUN apt-get update && \
apt-get -y upgrade && \
apt-get install -y libc6 ca-certificates tzdata && \
rm -rf /var/lib/apt/lists/*
RUN yum update --disablerepo=* --enablerepo=ubi-8-appstream --enablerepo=ubi-8-baseos -y && rm -rf /var/cache/yum

# Install GEOS libraries.
RUN mkdir /usr/local/lib/cockroach
COPY libgeos.so libgeos_c.so /usr/local/lib/cockroach/

RUN mkdir -p /cockroach
COPY cockroach.sh cockroach /cockroach/

# Set working directory so that relative paths
# are resolved appropriately when passed as args.
WORKDIR /cockroach/

# Include the directory into the path
# to make it easier to invoke commands
# via Docker
# Include the directory in the path to make it easier to invoke
# commands via Docker
ENV PATH=/cockroach:$PATH

ENV COCKROACH_CHANNEL=official-docker
Expand Down
22 changes: 20 additions & 2 deletions pkg/cli/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,17 @@ func runDecommissionNode(cmd *cobra.Command, args []string) error {
}

c := serverpb.NewAdminClient(conn)
return runDecommissionNodeImpl(ctx, c, nodeCtx.nodeDecommissionWait, nodeIDs)
if err := runDecommissionNodeImpl(ctx, c, nodeCtx.nodeDecommissionWait, nodeIDs); err != nil {
cause := errors.UnwrapAll(err)
if s, ok := status.FromError(cause); ok && s.Code() == codes.NotFound {
// Are we trying to decommision a node that does not
// exist? See Server.Decommission for where this specific grpc error
// code is generated.
return errors.New("node does not exist")
}
return err
}
return nil
}

func handleNodeDecommissionSelf(
Expand Down Expand Up @@ -566,13 +576,21 @@ func runRecommissionNode(cmd *cobra.Command, args []string) error {
}
resp, err := c.Decommission(ctx, req)
if err != nil {
cause := errors.UnwrapAll(err)
// If it's a specific illegal membership transition error, we try to
// surface a more readable message to the user. See
// ValidateLivenessTransition in kvserverpb/liveness.go for where this
// error is generated.
if s, ok := status.FromError(err); ok && s.Code() == codes.FailedPrecondition {
if s, ok := status.FromError(cause); ok && s.Code() == codes.FailedPrecondition {
return errors.Newf("%s", s.Message())
}
if s, ok := status.FromError(cause); ok && s.Code() == codes.NotFound {
// Are we trying to recommission node that does not
// exist? See Server.Decommission for where this specific grpc error
// code is generated.
fmt.Fprintln(stderr)
return errors.New("node does not exist")
}
return err
}
return printDecommissionStatus(*resp)
Expand Down
4 changes: 2 additions & 2 deletions pkg/jobs/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,14 @@ func (*FakeNodeLiveness) ModuleTestingKnobs() {}
// Self implements the implicit storage.NodeLiveness interface. It uses NodeID
// as the node ID. On every call, a nonblocking send is performed over nl.ch to
// allow tests to execute a callback.
func (nl *FakeNodeLiveness) Self() (kvserverpb.Liveness, error) {
func (nl *FakeNodeLiveness) Self() (kvserverpb.Liveness, bool) {
select {
case nl.SelfCalledCh <- struct{}{}:
default:
}
nl.mu.Lock()
defer nl.mu.Unlock()
return *nl.mu.livenessMap[FakeNodeID.Get()], nil
return *nl.mu.livenessMap[FakeNodeID.Get()], true
}

// GetLivenesses implements the implicit storage.NodeLiveness interface.
Expand Down
6 changes: 3 additions & 3 deletions pkg/jobs/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -784,10 +784,10 @@ func (r *Registry) maybeCancelJobsDeprecated(
// https://github.com/cockroachdb/cockroach/issues/47892
return
}
liveness, err := nl.Self()
if err != nil {
liveness, ok := nl.Self()
if !ok {
if nodeLivenessLogLimiter.ShouldLog() {
log.Warningf(ctx, "unable to get node liveness: %s", err)
log.Warning(ctx, "own liveness record not found")
}
// Conservatively assume our lease has expired. Abort all jobs.
r.deprecatedCancelAll(ctx)
Expand Down
7 changes: 4 additions & 3 deletions pkg/kv/kvserver/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1435,11 +1435,12 @@ func (m *multiTestContext) heartbeatLiveness(ctx context.Context, store int) err
m.mu.RLock()
nl := m.nodeLivenesses[store]
m.mu.RUnlock()
l, err := nl.Self()
if err != nil {
return err
l, ok := nl.Self()
if !ok {
return errors.New("liveness not found")
}

var err error
for r := retry.StartWithCtx(ctx, retry.Options{MaxRetries: 5}); r.Next(); {
if err = nl.Heartbeat(ctx, l); !errors.Is(err, kvserver.ErrEpochIncremented) {
break
Expand Down
7 changes: 2 additions & 5 deletions pkg/kv/kvserver/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -514,12 +514,9 @@ func (nl *NodeLiveness) SetDrainingInternal(
}

func (nl *NodeLiveness) SetDecommissioningInternal(
ctx context.Context,
nodeID roachpb.NodeID,
liveness LivenessRecord,
targetStatus kvserverpb.MembershipStatus,
ctx context.Context, oldLivenessRec LivenessRecord, targetStatus kvserverpb.MembershipStatus,
) (changeCommitted bool, err error) {
return nl.setMembershipStatusInternal(ctx, nodeID, liveness, targetStatus)
return nl.setMembershipStatusInternal(ctx, oldLivenessRec, targetStatus)
}

// GetCircuitBreaker returns the circuit breaker controlling
Expand Down
Loading

0 comments on commit 1ac75dd

Please sign in to comment.