gophercloud / utils

Apache License 2.0
20 stars 55 forks source link

Fixed gnocchi metrics and resources list pager #126

Closed tzmtl closed 4 years ago

tzmtl commented 4 years ago

In the current implementation, pager type of gnocchi metrics and resources list is SinglePageBase, which is wrong. In Gnocchi, query results in one page are limited to max_limit value set in the configuration file (default 1000). With the current implementation, AllPages() method can only show the first page of query result (the first 1000 metrics, for example).

The pager type should be MarkerPageBase and LastMarker() method needs to be implemented.

jtopjian commented 4 years ago

@tzmtl thank you for submitting this. Are you able to show some kind of failure with the original code and success with the new code? Or API JSON responses that have paging enabled?

tzmtl commented 4 years ago

I tested in a openstack test environment, with code below:

package main

import (
    "fmt"
    "github.com/gophercloud/gophercloud/pagination"
    "github.com/gophercloud/utils/openstack/clientconfig"
    "github.com/gophercloud/utils/gnocchi/metric/v1/metrics"
    "github.com/gophercloud/utils/gnocchi/metric/v1/resources"
)

func main() {
    //Create endpoints
    opts := &clientconfig.ClientOpts{
        Cloud: "oi-admin",
    }

    gnocchiClient, err := clientconfig.NewServiceClient("gnocchi", opts)
    if err != nil {
        panic(err)
    }

    fmt.Printf("Testing metrics list: \n")
    listOpts := metrics.ListOpts{
        Limit: 500,
    }

    fmt.Printf("Each page: \n")
    err = metrics.List(gnocchiClient, listOpts).EachPage(func(page pagination.Page) (bool, error) {
        s, err := metrics.ExtractMetrics(page)
        if err != nil {
            return false, err
        }

        fmt.Printf("Metric number in this page: %d\n", len(s))
        return true, nil
    })

    fmt.Printf("All page: \n")
    allPages, err := metrics.List(gnocchiClient, listOpts).AllPages()
    if err != nil {
        panic(err)
    }

    allMetrics, err := metrics.ExtractMetrics(allPages)
    if err != nil {
        panic(err)
    }

    fmt.Printf("Metric number in all pages: %d\n", len(allMetrics))

    fmt.Printf("Testing resources list: \n")
    rListOpts := resources.ListOpts{
        Details: true,
        Limit:   200,
    }

    fmt.Printf("Each page: \n")
    err = resources.List(gnocchiClient, rListOpts, "generic").EachPage(func(page pagination.Page) (bool, error) {
        s, err := resources.ExtractResources(page)
        if err != nil {
            return false, err
        }

        fmt.Printf("Resource number in this page: %d\n", len(s))
        return true, nil
    })

    if err != nil {
        panic(err)
    }

    fmt.Printf("All page: \n")
    allPages, err = resources.List(gnocchiClient, rListOpts, "generic").AllPages()
    if err != nil {
        panic(err)
    }

    allResources, err := resources.ExtractResources(allPages)
    if err != nil {
        panic(err)
    }

    fmt.Printf("Resource number in all pages: %d\n", len(allResources))
}

With original implementation, it returns only the first page:

test ./gnocchi-pager-test          
Testing metrics list: 
Each page: 
Metric number in this page: 500
All page: 
Metric number in all pages: 500
Testing resources list: 
Each page: 
Resource number in this page: 200
All page: 
Resource number in all pages: 200

With my fix, now it can return every page properly:

  test ./gnocchi-pager-test 
Testing metrics list: 
Each page: 
Metric number in this page: 500
Metric number in this page: 500
Metric number in this page: 500
Metric number in this page: 500
Metric number in this page: 500
Metric number in this page: 499
All page: 
Metric number in all pages: 2999
Testing resources list: 
Each page: 
Resource number in this page: 200
Resource number in this page: 200
Resource number in this page: 179
All page: 
Resource number in all pages: 579
tzmtl commented 4 years ago

result from openstack cli:

~ openstack metric list -f value | wc -l
    2999
~ openstack metric resource list -f value | wc -l
     579
jtopjian commented 4 years ago

Thank you for confirming 🙂