etcdutl Snapshot Status: TotalKey Calculation Inquiry

[jxustc]

Discussed in #19012

^{Originally posted by jxustc December 5, 2024}

Problem Description

When using the etcdutl snapshot status command, I've observed the following characteristics of TotalKey calculation:

• Current implementation counts keys across all buckets in the bbolt database
• Includes keys beyond visible etcd key-value data
• Key counts in other buckets may vary with different etcd versions

Case

During testing, I discovered that the TotalKey calculation includes keys from various internal buckets beyond the main key-value store:

Identified Included Keys

• In meta bucket:
  • term
  • scheduledCompactRev
  • confState
  • ...
• In auth bucket:
  • authRevision
• Keys from other internal buckets

Specific Questions

1. What is the precise definition of TotalKey?
2. Is the current calculation method appropriate?
3. Should the count be limited to etcd key-value entries?

Potential Implementation Approach

func (s *v3Manager) Status(dbPath string) (ds Status, err error) {
	...
	if err = db.View(func(tx *bolt.Tx) error {
		...
		c := tx.Cursor()
		for next, _ := c.First(); next != nil; next, _ = c.Next() {
			b := tx.Bucket(next)
			if b == nil {
				return fmt.Errorf("nil bucket: %q", string(next))
			}
			_, err = h.Write(next)
			if err != nil {
				return fmt.Errorf("cannot hash bucket name: %q err: %w", string(next), err)
			}

			iskeyb := (bytes.Equal(next, schema.Key.Name()))
			if err = b.ForEach(func(k, v []byte) error {
				_, err = h.Write(k)
				if err != nil {
					return fmt.Errorf("cannot hash bucket key: %q err: %w", k, err)
				}
				_, err = h.Write(v)
				if err != nil {
					return fmt.Errorf("cannot hash bucket key: %q value: %q err: %w", k, v, err)
				}
				if iskeyb {
					var rev mvcc.Revision
					rev, err = bytesToRev(k)
					if err != nil {
						return fmt.Errorf("cannot parse revision key: %q err: %w", k, err)
					}
					ds.Revision = rev.Main

					var kv mvccpb.KeyValue
					err = kv.Unmarshal(v)
					if err != nil {
						return fmt.Errorf("cannot unmarshal value, key: %q value: %q err: %w", k, v, err)
					}
                                         // move to here 
                                         ds.TotalKey++
				}
                                 // remove 
				ds.TotalKey++
				return nil
			}); err != nil {
				return fmt.Errorf("error during bucket key iteration, name: %q err: %w", string(next), err)
			}
		}
		return nil
	}); err != nil {
		return ds, err
	}
...
}

Besides this solution, could we possibly add a field to count the number of non-built-in key-value pairs in etcd?

Proposed Improvement

If appropriate, I am willing to submit a PR to implement this change.

[serathius]

Expect there is misaligment between definition of bbolt key and etcd key. As you mentioned it includes meta keys like scheduledCompactRev used for internal etcd purpose. I think it makes sense for etcd snapshot command to return keys in the etcd database, excluding the metadata keys. cc @ahrtr

[ahrtr]

I think it makes sense for etcd snapshot command to return keys in the etcd database, excluding the metadata keys. cc @ahrtr

YES, I agree. But unfortunately, it would be a breaking change as it has already been included in 3.5 for a long time. But I won't against to make a breaking change in 3.7 if there is no strong objection from users & contributors.

A better way is to run ./bbolt inspect to get the number of keys in each bucket,

$ ./bbolt inspect ../etcd/default.etcd/member/snap/db 
{
    "name": "root",
    "keyN": 0,
    "buckets": [
        {
            "name": "alarm",
            "keyN": 0
        },
        {
            "name": "auth",
            "keyN": 1
        },
        {
            "name": "authRoles",
            "keyN": 0
        },
        {
            "name": "authUsers",
            "keyN": 0
        },
        {
            "name": "cluster",
            "keyN": 1
        },
        {
            "name": "key",
            "keyN": 2
        },
        {
            "name": "lease",
            "keyN": 0
        },
        {
            "name": "members",
            "keyN": 1
        },
        {
            "name": "members_removed",
            "keyN": 0
        },
        {
            "name": "meta",
            "keyN": 4
        }
    ]
}

$ ./bin/etcdutl snapshot status ./default.etcd/member/snap/db 
663fd1f5, 3, 9, 98 kB, 3.6.0
F5WPP69Q7H:etcd wachao$ ./bin/etcdutl snapshot status ./default.etcd/member/snap/db  -w table
+----------+----------+------------+------------+---------+
|   HASH   | REVISION | TOTAL KEYS | TOTAL SIZE | VERSION |
+----------+----------+------------+------------+---------+
| 663fd1f5 |        3 |          9 |      98 kB |   3.6.0 |
+----------+----------+------------+------------+---------+

[serathius]

But unfortunately, it would be a breaking change as it has already been included in 3.5 for a long time

Could you clarify what you mean by breaking change? How you expect users depend on the value?

For me this is fix in metric value to be more accurate. There are no changes in structure, name or format of the metric that could impact how user interacts with it.

[ahrtr]

In theory, users may

• parse the json output
• or call (*v3Manager) Status directly.

Changing the value may break users's expectation.

In practice, I am not sure how many users use/depend on it, might not too many.

Overall, I am not against make this breaking change.

[ahrtr]

To be clearer, I agree that there is no format/struct change.

I was saying changing the value is also a breaking change. For example, starting a brand new etcd instance, the default number of key is always 7 even there is no any key/value write. Now we are going to change it to 0. We don't know how it may affect user's test or any expectation.

Again, overall, I am not against to make it more accurate.

[jxustc]

Thank you all for the discussion. If there are no objections, I'd like to submit a PR to the main branch to fix this issue by excluding built-in keys from the TotalKey calculation (without modifying any formatting or structural changes).

This change would provide more accurate metrics for monitoring and debugging purposes, better aligning with users' expectations.

Please let me know if there are any additional considerations I should be aware of.

[ahrtr]

Please note that internally etcd uses the revision as the key, but I think we should count the number of the real keys from users perspective.

For example, when you execute the same command ./bin/etcdctl put k1 v1 three times, from bbolt perspective, there are three keys in the key bucket. But from users perspective, there is only one key. We should output 1 instead of 3 in this example,

$ ./bbolt inspect ../etcd/default.etcd/member/snap/db 
{
    "name": "root",
    "keyN": 0,
    "buckets": [
        {
            "name": "alarm",
            "keyN": 0
        },
        {
            "name": "auth",
            "keyN": 1
        },
        {
            "name": "authRoles",
            "keyN": 0
        },
        {
            "name": "authUsers",
            "keyN": 0
        },
        {
            "name": "cluster",
            "keyN": 1
        },
        {
            "name": "key",
            "keyN": 3
        },
        {
            "name": "lease",
            "keyN": 0
        },
        {
            "name": "members",
            "keyN": 1
        },
        {
            "name": "members_removed",
            "keyN": 0
        },
        {
            "name": "meta",
            "keyN": 4
        }
    ]
}