Ceph » RADOS

I am now seeing this issue on pacific, 16.2.10 on rocky8 linux.

If I have a >2 level rule on an ec pool (6+2), such as:

rule mypoolname {
id -5
type erasure
step take myroot
step choose indep 4 type rack
step choose indep 2 type chassis
step chooseleaf indep 1 type host
step emit

myroot members are just a list of the rack buckets.

osdmaptool --upmap-cleanup gives:

check_pg_upmaps verify upmap of pool.pgid returning -22
verify_upmap number of buckets 8 exceeds desired 2

For each of my existing upmaps (I am changing my crushmap to this)

If, however, I use:

rule mypoolname {
id -5
type erasure
step take myroot
step choose indep 4 type rack
step chooseleaf indep 2 type chassis
step emit

osdmaptool --upmap-cleanup gives:

verify_upmap multiple osds N,M come from the same failure domain -382
check_pg_upmap verify upmap of pg poolid.pgid returning -22.

For about 1/8 of my upmaps. And it wants to remove these and and add about 100 more.
I actually expect this given that I am changing my rule from something else. But this rule
is only 2 levels, where as the first one is 3 levels, which seems to cause the problem.

But the first rule should NOT give the expected 2 got 8. I want 8 on a 6+2 EC pool. Thus, I think this problem still exists for a multi-level (>2) level
crush rule on ec pools.

This is a result of the following discussion, where Dan van der Ster tells me to report this here, as this appears to still be a bug.

https://lists.ceph.io/hyperkitty/list/ceph-users@ceph.io/thread/AZHAIGY3BIM4SGBUBKX5ZGYTXQWAJ7OO/

Please advise.

Just to clarify - the error "verify_upmap number of buckets X exceeds desired Y" comes from the C++ code in ceph-mon that tries to validate a newly inserted upmap. It is in CrushWrapper::verify_upmap(...):

    case CRUSH_RULE_CHOOSE_INDEP:
...
        if ((int)parents_of_type.size() > numrep) {
          lderr(cct) << __func__ << " number of buckets " 
                     << parents_of_type.size() << " exceeds desired " << numrep
                     << dendl;
          return -EINVAL;
        }
...

Andras,

Thanks for the extra info. This needs to be addressed. Anyone?

A note from bug scrub: this is going to be assigned tomorrow.

Thanks for the info! Laura, would you mind retaking a look?

Thanks Chris! @Radoslaw Smigielski I have been taking some time to analyze this scenario, and will post updates soon.

I believe I've reproduced the issue using the osdmaps that Chris provided.

First, I used the osdmaptool to run the upmap balancer on `osdmap.bad.bin`. I wanted it to create the same mapping Chris described: 2.1f 20 19.

I had to run the command multiple times to achieve this mapping, so I wrote a small script to do so. I set --upmap-deviation to 1 so the balancer would would actually try mappings, since this osdmap was already considered "balanced" with the default deviation of 5:

#!/bin/bash

while :
do
    ./bin/osdmaptool ~/osdmap.bad.bin --upmap out.txt --upmap-pool ectest --upmap-deviation 1
    if cat out.txt | grep "2.1f" 
    then
        break
    fi
done

With this test script, the osdmaptool eventually suggested the "2.1f 20 19" mapping:

[lflores@folio01 build]$ ~/bad_map.sh
./bin/osdmaptool: osdmap file '/home/lflores/osdmap.bad.bin'
writing upmap command output to: out.txt
checking for upmap cleanups
upmap, max-count 10, max deviation 1
 limiting to pools ectest ([2])
pools ectest 
prepared 10/10 changes
./bin/osdmaptool: osdmap file '/home/lflores/osdmap.bad.bin'
writing upmap command output to: out.txt
checking for upmap cleanups
upmap, max-count 10, max deviation 1
 limiting to pools ectest ([2])
pools ectest 
prepared 10/10 changes
ceph osd pg-upmap-items 2.1f 20 19

And here are the exact mappings it suggested:

ceph osd pg-upmap-items 2.2 18 22
ceph osd pg-upmap-items 2.4 21 16
ceph osd pg-upmap-items 2.7 8 13
ceph osd pg-upmap-items 2.8 0 3
ceph osd pg-upmap-items 2.a 11 9
ceph osd pg-upmap-items 2.19 11 14 21 16
ceph osd pg-upmap-items 2.1e 5 4 21 17
ceph osd pg-upmap-items 2.1f 20 19

As we can see, the osdmaptool decides that "2.1f 20 19" is a valid mapping. But at this point, we have not run the mapping through "verify_upmap".

Now, I'll save the results so we can use the `--upmap-cleanup` option, which will trigger "verify_upmap". This is the function where we see the error message "verify_upmap number of buckets 9 exceeds desired 3".

First, I copy "osdmap.bad.bin" into a new file, "osdmap.bad-save.bin".

cp ~/osdmap.bad.bin ~/osdmap.bad-save.bin

Next, I modify the script a bit so we're actually saving the results this time:

#!/bin/bash

while :
do
    ./bin/osdmaptool ~/osdmap.bad-save.bin --upmap out.txt --upmap-pool ectest --upmap-deviation 1 --save
    if cat out.txt | grep "2.1f" 
    then
        break
    fi
done

We hit the desired mapping:

[lflores@folio01 build]$ ~/bad_map_save.sh
./bin/osdmaptool: osdmap file '/home/lflores/osdmap.bad-save.bin'
writing upmap command output to: out.txt
checking for upmap cleanups
upmap, max-count 10, max deviation 1
 limiting to pools ectest ([2])
pools ectest 
prepared 10/10 changes
./bin/osdmaptool: writing epoch 397 to /home/lflores/osdmap.bad-save.bin
ceph osd pg-upmap-items 2.1f 20 19

If I print out `osdmap.bad-save.bin`, it will have saved the mapping. I've removed some details here in case they might be sensitive (although the map is attached to the tracker):

[lflores@folio01 build]$ ./bin/osdmaptool ~/osdmap.bad-save.bin --print
./bin/osdmaptool: osdmap file '/home/lflores/osdmap.bad-save.bin'

.....osdmap details....

pg_upmap_items 2.4 [11,14]
pg_upmap_items 2.7 [8,14]
pg_upmap_items 2.8 [0,3]
pg_upmap_items 2.c [21,16]
pg_upmap_items 2.d [5,4]
pg_upmap_items 2.e [8,13]
pg_upmap_items 2.17 [21,17]
pg_upmap_items 2.19 [11,9,21,16]
pg_upmap_items 2.1f [20,19] -------------------> here the mapping is saved
pg_temp 1.0 [8,3,7]

.... more osdmap details ...

Now, I'll run `--upmap-cleanup` to trigger the verify_upmap error message:

[lflores@folio01 build]$ ./bin/osdmaptool ~/osdmap.bad-save.bin --upmap-cleanup out.txt
./bin/osdmaptool: osdmap file '/home/lflores/osdmap.bad-save.bin'
writing upmap command output to: out.txt
checking for upmap cleanups
2022-11-08T19:16:33.797+0000 7fb80681a100 -1 verify_upmap number of buckets 3 exceeds desired 1
2022-11-08T19:16:33.797+0000 7fb80681a100  0 check_pg_upmaps verify_upmap of pg 2.4 returning -22
2022-11-08T19:16:33.797+0000 7fb80681a100 -1 verify_upmap number of buckets 3 exceeds desired 1
2022-11-08T19:16:33.797+0000 7fb80681a100  0 check_pg_upmaps verify_upmap of pg 2.7 returning -22
2022-11-08T19:16:33.797+0000 7fb80681a100 -1 verify_upmap number of buckets 3 exceeds desired 1
2022-11-08T19:16:33.797+0000 7fb80681a100  0 check_pg_upmaps verify_upmap of pg 2.8 returning -22
2022-11-08T19:16:33.797+0000 7fb80681a100 -1 verify_upmap number of buckets 3 exceeds desired 1
2022-11-08T19:16:33.797+0000 7fb80681a100  0 check_pg_upmaps verify_upmap of pg 2.c returning -22
2022-11-08T19:16:33.797+0000 7fb80681a100 -1 verify_upmap number of buckets 3 exceeds desired 1
2022-11-08T19:16:33.797+0000 7fb80681a100  0 check_pg_upmaps verify_upmap of pg 2.d returning -22
2022-11-08T19:16:33.797+0000 7fb80681a100 -1 verify_upmap number of buckets 3 exceeds desired 1
2022-11-08T19:16:33.797+0000 7fb80681a100  0 check_pg_upmaps verify_upmap of pg 2.e returning -22
2022-11-08T19:16:33.797+0000 7fb80681a100 -1 verify_upmap number of buckets 3 exceeds desired 1
2022-11-08T19:16:33.797+0000 7fb80681a100  0 check_pg_upmaps verify_upmap of pg 2.17 returning -22
2022-11-08T19:16:33.797+0000 7fb80681a100 -1 verify_upmap number of buckets 3 exceeds desired 1
2022-11-08T19:16:33.797+0000 7fb80681a100  0 check_pg_upmaps verify_upmap of pg 2.19 returning -22
2022-11-08T19:16:33.797+0000 7fb80681a100 -1 verify_upmap number of buckets 3 exceeds desired 1
2022-11-08T19:16:33.797+0000 7fb80681a100  0 check_pg_upmaps verify_upmap of pg 2.1f returning -22

And here, it gave us commands to remove mappings that it has deemed "invalid":

ceph osd rm-pg-upmap-items 2.4
ceph osd rm-pg-upmap-items 2.7
ceph osd rm-pg-upmap-items 2.8
ceph osd rm-pg-upmap-items 2.c
ceph osd rm-pg-upmap-items 2.d
ceph osd rm-pg-upmap-items 2.e
ceph osd rm-pg-upmap-items 2.17
ceph osd rm-pg-upmap-items 2.19
ceph osd rm-pg-upmap-items 2.1f

Now, I'm going to try this with "osdmap.good.bin".

If we dump "osdmap.good.bin", we can see that the desired mapping is already there, so there's no need for me to create a "saved" version:

[lflores@folio01 build]$ ./bin/osdmaptool ~/osdmap.good.bin --dump
./bin/osdmaptool: osdmap file '/home/lflores/osdmap.good.bin'
epoch 398

.... osdmap details ....

pg_upmap_items 2.1f [20,19] ---> here is the mapping
pg_temp 1.0 [8,3,7]

.... more osdmap details ....

Now, I'll try running "--upmap-cleanup" on this map to see if the same "verify_upmap" error message appears. It does not:

[lflores@folio01 build]$ ./bin/osdmaptool ~/osdmap.good.bin --upmap-cleanup out.txt
./bin/osdmaptool: osdmap file '/home/lflores/osdmap.good.bin'
writing upmap command output to: out.txt
checking for upmap cleanups

And the "out.txt" file is completely empty since "verify_upmaps" did not see any problems with the mapping. If it did, it would have suggested we remove the mapping with "ceph osd rm-pg-upmap-items 2.1f" as it did with the bad map.

I know some of this was redundant, but reproducing the issue means one step closer to me understanding if there's something wrong in verify_upmaps.

Thanks again for looking at this.

I haven't looked further, but I suspect the issue will come down to the variable 'numrep' in CrushWrapper:verify_upmap(...) that was posted above. (below for reference)
'numrep' is somehow not being set properly soemwhere else for a 3+ level ec rule.

   case CRUSH_RULE_CHOOSE_INDEP:
...
        if ((int)parents_of_type.size() > numrep) {
          lderr(cct) << __func__ << " number of buckets " 
                     << parents_of_type.size() << " exceeds desired " << numrep
                     << dendl;
          return -EINVAL;
        }
...

Thanks for formulating the hypothesis!

Just updating to keep this ticket in the front of the tracker.

Update on this Tracker: I am discussing this scenario with Josh Salomon, someone who is very knowledgeable about balancing. He and I recently did some work on the verify_upmaps() function, so I want to go over some things with him to reason where an issue might be in that function.

Is there any news on this? Thanks.

Hi Chris, yes, I will post another update soon with my findings.

Laura Flores wrote:

Hi Chris, yes, I will post another update soon with my findings.

Pinging for updates....

Hi Chris, this issue was actually discussed at Cephalocon. Looking at the verify_upmap code, it seems that we may need to add another case to handle 3+ level crush rules.

I am in progress working on a solution. When I have something, I will update the tracker with the PR so you and others involved can take a look.

I've landed on a potential fix for this problem. After evaluating the examples everyone provided and checking the verify_upmap logic, it looks like we need to multiply the "numrep" variable by the parent bucket value.

If we look at this example crush rule, the current logic causes valid upmaps to fail with "verify_upmap number of buckets 3 exceeds desired 1". However, this implies that we choose 3 racks, then 1 chassis from those 3 racks.

rule ectest {
id 2
type erasure
min_size 3
max_size 3
step set_chooseleaf_tries 5
step set_choose_tries 100
step take default_test
step choose indep 3 type rack
step choose indep 1 type chassis
step chooseleaf indep 1 type host
step emit
}

This is roughly what the fix will look like:

diff --git a/src/crush/CrushWrapper.cc b/src/crush/CrushWrapper.cc
index 530c2413933..dc75f00ba16 100644
--- a/src/crush/CrushWrapper.cc
+++ b/src/crush/CrushWrapper.cc
@@ -908,6 +908,7 @@ int CrushWrapper::verify_upmap(CephContext *cct,
   }
   int root_bucket = 0;
   int cursor = 0;
+  int curr_choose_numrep = 1;
   std::map<int, int> type_stack;
   for (unsigned step = 0; step < rule->len; ++step) {
     auto curstep = &rule->steps[step];
@@ -953,7 +954,8 @@ int CrushWrapper::verify_upmap(CephContext *cct,
     case CRUSH_RULE_CHOOSE_FIRSTN:
     case CRUSH_RULE_CHOOSE_INDEP:
       {
-        int numrep = curstep->arg1;
+        int numrep = curr_choose_numrep * curstep->arg1;
+        curr_choose_numrep = numrep;
         int type = curstep->arg2;
         if (numrep <= 0)
           numrep += pool_size;

Also to note, this may be a small change, but it will need to be well tested. I am in progress designing a unit test around this.

Hi Chris,

If possible, can you try this change with your crush rule?
https://github.com/ceph/ceph/compare/main...ljflores:ceph:wip-tracker-51729?expand=1

It would help to get some early feedback on the fix as I refine the unit test.

Hi Laura,

I'm returning to this issue since not being able to use upmaps in our cluster has been causing some headaches. I've experimented with the patch above and needed to modify it. Mainly due to the map type_stack which seems to want to have the per level (not cumulative) numrep's in it (they get multiplied up later in that function). My current patch is:

diff -up ./src/crush/CrushWrapper.cc.ORIG ./src/crush/CrushWrapper.cc
--- ./src/crush/CrushWrapper.cc.ORIG    2022-08-09 13:07:01.000000000 -0400
+++ ./src/crush/CrushWrapper.cc    2024-01-17 11:03:54.139061612 -0500
@@ -933,6 +933,7 @@ int CrushWrapper::verify_upmap(CephConte
   }
   int root_bucket = 0;
   int cursor = 0;
+  int total_numrep = 1;
   std::map<int, int> type_stack;
   for (unsigned step = 0; step < rule->len; ++step) {
     auto curstep = &rule->steps[step];
@@ -950,6 +951,7 @@ int CrushWrapper::verify_upmap(CephConte
         int type = curstep->arg2;
         if (numrep <= 0)
           numrep += pool_size;
+        total_numrep *= numrep;
         type_stack.emplace(type, numrep);
         if (type == 0) // osd
           break;
@@ -982,6 +984,7 @@ int CrushWrapper::verify_upmap(CephConte
         int type = curstep->arg2;
         if (numrep <= 0)
           numrep += pool_size;
+        total_numrep *= numrep;
         type_stack.emplace(type, numrep);
         if (type == 0) // osd
           break;
@@ -996,9 +999,10 @@ int CrushWrapper::verify_upmap(CephConte
                           << dendl;
           }
         }
-        if ((int)parents_of_type.size() > numrep) {
+        if ((int)parents_of_type.size() > total_numrep) {
           lderr(cct) << __func__ << " number of buckets " 
                      << parents_of_type.size() << " exceeds desired " << numrep
+             << " in step " << step
                      << dendl;
           return -EINVAL;
         }

rule disk-ec63-rule {
        id 2
        type erasure
        min_size 3
        max_size 9
        step set_chooseleaf_tries 5
        step set_choose_tries 100
        step take root-disk
        step choose indep 3 type pod
        step choose indep 3 type rack
        step chooseleaf indep 1 type osd
        step emit
}

Can you have a look and give me any comments?

Thanks,

Andras

Hi Andras,

Thanks for testing the patch with your test scenario. I will take a look at your additions to the patch.

Update on this bug:

We are pretty close to getting the fix out for this. Thanks all for waiting so long. In addition to multi-choose crush rules listed here, the changes to verify_upmap will also need to be validated against the new CRUSH_MSR changes that were merged in Reef as well as crush rules used in stretch clusters.

The work for the unit test is on this branch: https://github.com/ceph/ceph/compare/main...ljflores:ceph:wip-tracker-51729?expand=1