From cab720d15676f112a16ef7ac5f63ac12c4aa34f8 Mon Sep 17 00:00:00 2001
From: John Spray <john@neon.tech>
Date: Tue, 25 Jun 2024 16:34:31 +0100
Subject: [PATCH] pageserver: make clearer that Generation::none shouldn't be
 used on tenants

---
 libs/utils/src/generation.rs                       | 14 +++++---------
 pageserver/src/tenant.rs                           |  2 ++
 .../src/tenant/secondary/heatmap_uploader.rs       |  5 ++---
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/libs/utils/src/generation.rs b/libs/utils/src/generation.rs
index 0362110c21e2..5970836033c8 100644
--- a/libs/utils/src/generation.rs
+++ b/libs/utils/src/generation.rs
@@ -9,15 +9,11 @@ use serde::{Deserialize, Serialize};
 /// numbers are used.
 #[derive(Copy, Clone, Eq, PartialEq, PartialOrd, Ord, Hash)]
 pub enum Generation {
-    // Generations with this magic value will not add a suffix to S3 keys, and will not
-    // be included in persisted index_part.json.  This value is only to be used
-    // during migration from pre-generation metadata to generation-aware metadata,
-    // and should eventually go away.
-    //
-    // A special Generation is used rather than always wrapping Generation in an Option,
-    // so that code handling generations doesn't have to be aware of the legacy
-    // case everywhere it touches a generation.
+    // The None Generation is used in the metadata of layers written before generations were
+    // introduced.  A running Tenant always has a valid generation, but the layer metadata may
+    // include None generations.
     None,
+
     Valid(u32),
 }
 
@@ -113,7 +109,7 @@ impl Serialize for Generation {
         if let Self::Valid(v) = self {
             v.serialize(serializer)
         } else {
-            // We should never be asked to serialize a None or Broken.  Structures
+            // We should never be asked to serialize a None. Structures
             // that include an optional generation should convert None to an
             // Option<Generation>::None
             Err(serde::ser::Error::custom(
diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs
index b39c07208657..05d349526746 100644
--- a/pageserver/src/tenant.rs
+++ b/pageserver/src/tenant.rs
@@ -2470,6 +2470,8 @@ impl Tenant {
         remote_storage: GenericRemoteStorage,
         deletion_queue_client: DeletionQueueClient,
     ) -> Tenant {
+        debug_assert!(!attached_conf.location.generation.is_none());
+
         let (state, mut rx) = watch::channel(state);
 
         tokio::spawn(async move {
diff --git a/pageserver/src/tenant/secondary/heatmap_uploader.rs b/pageserver/src/tenant/secondary/heatmap_uploader.rs
index 9c7a9c4234e5..f1ecc6200c79 100644
--- a/pageserver/src/tenant/secondary/heatmap_uploader.rs
+++ b/pageserver/src/tenant/secondary/heatmap_uploader.rs
@@ -368,9 +368,8 @@ async fn upload_tenant_heatmap(
 
     let generation = tenant.get_generation();
     if generation.is_none() {
-        // We do not expect this: generations were implemented before heatmap uploads.  However,
-        // handle it so that we don't have to make the generation in the heatmap an Option<>
-        // (Generation::none is not serializable)
+        // We do not expect this: None generations should only appear in historic layer metadata, not in running Tenants
+        debug_assert!(generation.is_none());
         tracing::warn!("Skipping heatmap upload for tenant with generation==None");
         return Ok(UploadHeatmapOutcome::Skipped);
     }