Docs: Proceeding from one node to another during a rolling upgrade

[jseldess]

Our rolling upgrade instructions say:

Upgrade only one node at a time, and wait at least one minute after a node rejoins the cluster to upgrade the next node. Simultaneously upgrading more than one node increases the risk that ranges will lose a majority of their replicas and cause cluster unavailability.

Also, refrain from starting schema changes during the upgrade process. Schema changes are complex operations that involve coordination across nodes and can increase the potential for unexpected behavior during an upgrade.

In our Kubernetes-specific upgrade instructions, we just say to change the Docker image in the helm chart or Statefulset, which triggers a restart of the pods sequentially, but without any controlled waiting or checks in-between.

A customer noticed this discrepancy and asked about the wait period, which lead to the following guidance from @andy-kimball:

1. Wait until select count(*) FROM crdb_internal.gossip_nodes where is_live=false; is zero. This is simple, and relies on the assumption is that if all your nodes are live, then there is very low risk of under-replicated ranges. During discussions, we couldn’t confirm any relevant cases where under-replicated ranges would occur on a fully-live cluster.
2. Shut down next node using cockroach quit or SIGTERM. There is some danger that you’d see higher query tail latencies if all leaseholders cannot be drained within a 5s period of time. If in practice you’re seeing this be a problem, we can make this timeout be configurable, and you could set it to 1m or 5m, or whatever.
3. Bring node back up after upgrade.
4. Proceed to next node by going back to step #1.

We should think about updating the upgrade instructions to reflect this guidance. We should also update the Kubernetes-specific instructions to follow a similar approach. The CC team is already doing something similar, according to @DuskEagle:

Instead of using select count(*) FROM crdb_internal.gossip_nodes where is_live=false; to detect when a node is live, we are using a SQL query similar to the one used by cockroach node status --ranges to compute the underreplicated ranges in the cluster, and only proceeding when the number of underrepicated ranges is zero: select sum((metrics->>'ranges.underreplicated')::DECIMAL)::INT AS ranges_underreplicated from crdb_internal.kv_store_status;

besides that, when updating the statefulset with a new version of CockroachDB, we also set the spec.updateStrategy.rollingUpdate.partition value: https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#partitions. If we have 3 pods in the statefulset, named crdb-0, crdb-1, and crdb-2, we will first set the partition value to 2. This means that only crdb-2 will be updated to the new Cockroach version.

We then wait until the pod is healthy, and the number of underreplicated ranges is 0, and then reduce the partition number by 1. This causes crdb-1 to be updated to the new version. And so on.

[jseldess]

Follow-up comment from @andy-kimball suggests that CC approach is preferable:

I think what you're doing (using kv_store_status) will work well, better than what I suggested, since it handles underreplicated edge cases.

Also, @joshimhoff suggested that the Kubernetes operator being built by @vladdy will improve this process. @vladdy and @kannanlakshmi, where can we find more details about this operator? It's not on the 20.1 roadmap, from what I can see, though I have heard about it in passing at certain points?

[kannanlakshmi]

This isn't on the roadmap from a user point of view but the SREs are building it as part of tooling that makes some of the K8s automation work easy (hence not showing up in Airtable). At the moment, its not user facing but I believe the plan is to eventually open source it (once implemented, rolled out to our clusters etc.)
@vladdy with the idea that eventually we want to open source it, is there intermediate work that can be shared with the docs team or the rest of the company? Lots of people are interested in this as you know.

[jseldess]

Thanks for the clarification, @kannanlakshmi. We're definitely interested in learning more, when that makes sense. We've also heard from the SE team that an operator is far preferable to the current approach we take in our docs (configs or helm). @johnrk, perhaps this eventually official K8s operator should be on the SKO backlog?

[vladdy]

I just wanted to add that the operator is expected to make operations of CRDB in Kubernetes easier. However, we still need one source of the truth about how it is recommended to handle the updates for all environments. It could be manual steps/some simple bash scripts in the docs, which would help everybody to be in sync on what is the safest and proper way to do them.

[johnrk]

@jseldess , it's now in the backlog: https://airtable.com/tblD3oZPLJgGhCmch/viwsaabU5mS5NCizt/recyDZ3jhsnQxfGWw?blocks=hide

[taroface]

Product will still need to establish our best practices for "rolling restarts" and/or "repaving" per OKS product council discussion.

cc @johnrk

[johnrk]

Correct. We will establish best practices for this.

[taroface]

Hi @knz, I wanted to follow up here on a few points you brought up in your recent email (on verifying that it’s safe to shut down the next node in a rolling restart):

1. wait until the node is ready to accept a SQL connection (before that point, it's definitely not ready)

What is the most straightforward way to tell this? Would it be indicated by the standard output?

1. wait until the memory usage has reached at least 70% of expected baseline load (which should have been established prior to restart)

Same question, and I wonder if the Admin UI could be used? Also, could you say more about “should have been established prior to restart”?

1. wait until the node is green in the admin UI

By this you mean checking for node liveness, right?

• Would one of the SQL queries listed in the related docs issue satisfy this need?
  These will only satisfy point (3) not the other two points.

There are two SQL queries here. Does one seem preferable to the other for this purpose?

select count(*) FROM crdb_internal.gossip_nodes where is_live=false;

select sum((metrics->>'ranges.underreplicated')::DECIMAL)::INT AS ranges_underreplicated from crdb_internal.kv_store_status;

Thank you!