Skip to content

Commit

Permalink
Knock-on effects of deprecating azure.msi-resource
Browse files Browse the repository at this point in the history
Because we want to log if it is used, many places need to pass through a logger.

Also update the docs.
  • Loading branch information
bboreham committed Sep 2, 2022
1 parent b320aec commit ed80907
Show file tree
Hide file tree
Showing 16 changed files with 58 additions and 66 deletions.
3 changes: 2 additions & 1 deletion cmd/metaconvert/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,9 @@ func main() {
cfg := config{}

cfg.LogLevel.RegisterFlags(flag.CommandLine)
initLogger := log.NewDefaultLogger(cfg.LogLevel, cfg.LogFormat)
cfg.LogFormat.RegisterFlags(flag.CommandLine)
cfg.BucketConfig.RegisterFlags(flag.CommandLine)
cfg.BucketConfig.RegisterFlags(flag.CommandLine, initLogger)

flag.BoolVar(&cfg.DryRun, "dry-run", false, "Don't make changes; only report what needs to be done")
flag.StringVar(&cfg.Tenant, "tenant", "", "Tenant to process")
Expand Down
12 changes: 4 additions & 8 deletions cmd/mimir/config-descriptor.json
Original file line number Diff line number Diff line change
Expand Up @@ -4204,10 +4204,9 @@
"kind": "field",
"name": "msi_resource",
"required": false,
"desc": "If set, this URL is used instead of https://\u003cstorage-account-name\u003e.\u003cendpoint-suffix\u003e for obtaining ServicePrincipalToken from MSI.",
"desc": "",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "blocks-storage.azure.msi-resource",
"fieldType": "string",
"fieldCategory": "advanced"
},
Expand Down Expand Up @@ -8018,10 +8017,9 @@
"kind": "field",
"name": "msi_resource",
"required": false,
"desc": "If set, this URL is used instead of https://\u003cstorage-account-name\u003e.\u003cendpoint-suffix\u003e for obtaining ServicePrincipalToken from MSI.",
"desc": "",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "ruler-storage.azure.msi-resource",
"fieldType": "string",
"fieldCategory": "advanced"
},
Expand Down Expand Up @@ -9347,10 +9345,9 @@
"kind": "field",
"name": "msi_resource",
"required": false,
"desc": "If set, this URL is used instead of https://\u003cstorage-account-name\u003e.\u003cendpoint-suffix\u003e for obtaining ServicePrincipalToken from MSI.",
"desc": "",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "alertmanager-storage.azure.msi-resource",
"fieldType": "string",
"fieldCategory": "advanced"
},
Expand Down Expand Up @@ -10585,10 +10582,9 @@
"kind": "field",
"name": "msi_resource",
"required": false,
"desc": "If set, this URL is used instead of https://\u003cstorage-account-name\u003e.\u003cendpoint-suffix\u003e for obtaining ServicePrincipalToken from MSI.",
"desc": "",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "common.storage.azure.msi-resource",
"fieldType": "string",
"fieldCategory": "advanced"
},
Expand Down
8 changes: 0 additions & 8 deletions cmd/mimir/help-all.txt.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ Usage of ./cmd/mimir/mimir:
Azure storage endpoint suffix without schema. The account name will be prefixed to this value to create the FQDN. If set to empty string, default endpoint suffix is used.
-alertmanager-storage.azure.max-retries int
Number of retries for recoverable errors (default 20)
-alertmanager-storage.azure.msi-resource string
If set, this URL is used instead of https://<storage-account-name>.<endpoint-suffix> for obtaining ServicePrincipalToken from MSI.
-alertmanager-storage.azure.user-assigned-id string
User assigned identity. If empty, then System assigned identity is used.
-alertmanager-storage.backend string
Expand Down Expand Up @@ -258,8 +256,6 @@ Usage of ./cmd/mimir/mimir:
Azure storage endpoint suffix without schema. The account name will be prefixed to this value to create the FQDN. If set to empty string, default endpoint suffix is used.
-blocks-storage.azure.max-retries int
Number of retries for recoverable errors (default 20)
-blocks-storage.azure.msi-resource string
If set, this URL is used instead of https://<storage-account-name>.<endpoint-suffix> for obtaining ServicePrincipalToken from MSI.
-blocks-storage.azure.user-assigned-id string
User assigned identity. If empty, then System assigned identity is used.
-blocks-storage.backend string
Expand Down Expand Up @@ -531,8 +527,6 @@ Usage of ./cmd/mimir/mimir:
Azure storage endpoint suffix without schema. The account name will be prefixed to this value to create the FQDN. If set to empty string, default endpoint suffix is used.
-common.storage.azure.max-retries int
Number of retries for recoverable errors (default 20)
-common.storage.azure.msi-resource string
If set, this URL is used instead of https://<storage-account-name>.<endpoint-suffix> for obtaining ServicePrincipalToken from MSI.
-common.storage.azure.user-assigned-id string
User assigned identity. If empty, then System assigned identity is used.
-common.storage.backend string
Expand Down Expand Up @@ -1370,8 +1364,6 @@ Usage of ./cmd/mimir/mimir:
Azure storage endpoint suffix without schema. The account name will be prefixed to this value to create the FQDN. If set to empty string, default endpoint suffix is used.
-ruler-storage.azure.max-retries int
Number of retries for recoverable errors (default 20)
-ruler-storage.azure.msi-resource string
If set, this URL is used instead of https://<storage-account-name>.<endpoint-suffix> for obtaining ServicePrincipalToken from MSI.
-ruler-storage.azure.user-assigned-id string
User assigned identity. If empty, then System assigned identity is used.
-ruler-storage.backend string
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3567,10 +3567,7 @@ The `azure_storage_backend` block configures the connection to Azure object stor
# CLI flag: -<prefix>.azure.max-retries
[max_retries: <int> | default = 20]

# (advanced) If set, this URL is used instead of
# https://<storage-account-name>.<endpoint-suffix> for obtaining
# ServicePrincipalToken from MSI.
# CLI flag: -<prefix>.azure.msi-resource
# (advanced)
[msi_resource: <string> | default = ""]

# (advanced) User assigned identity. If empty, then System assigned identity is
Expand Down
6 changes: 4 additions & 2 deletions pkg/alertmanager/alertstore/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ package alertstore
import (
"flag"

"github.com/go-kit/log"

"github.com/grafana/mimir/pkg/alertmanager/alertstore/local"
"github.com/grafana/mimir/pkg/storage/bucket"
)
Expand All @@ -19,10 +21,10 @@ type Config struct {
}

// RegisterFlags registers the backend storage config.
func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
func (cfg *Config) RegisterFlags(f *flag.FlagSet, logger log.Logger) {
prefix := "alertmanager-storage."

cfg.StorageBackendConfig.ExtraBackends = []string{local.Name}
cfg.Local.RegisterFlagsWithPrefix(prefix, f)
cfg.RegisterFlagsWithPrefixAndDefaultDirectory(prefix, "alertmanager", f)
cfg.RegisterFlagsWithPrefixAndDefaultDirectory(prefix, "alertmanager", f, logger)
}
12 changes: 6 additions & 6 deletions pkg/mimir/mimir.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,22 +154,22 @@ func (c *Config) RegisterFlags(f *flag.FlagSet, logger log.Logger) {
c.LimitsConfig.RegisterFlags(f)
c.Worker.RegisterFlags(f)
c.Frontend.RegisterFlags(f, logger)
c.BlocksStorage.RegisterFlags(f)
c.BlocksStorage.RegisterFlags(f, logger)
c.Compactor.RegisterFlags(f, logger)
c.StoreGateway.RegisterFlags(f, logger)
c.TenantFederation.RegisterFlags(f)

c.Ruler.RegisterFlags(f, logger)
c.RulerStorage.RegisterFlags(f)
c.RulerStorage.RegisterFlags(f, logger)
c.Alertmanager.RegisterFlags(f, logger)
c.AlertmanagerStorage.RegisterFlags(f)
c.AlertmanagerStorage.RegisterFlags(f, logger)
c.RuntimeConfig.RegisterFlags(f)
c.MemberlistKV.RegisterFlags(f)
c.ActivityTracker.RegisterFlags(f)
c.QueryScheduler.RegisterFlags(f)
c.UsageStats.RegisterFlags(f)

c.Common.RegisterFlags(f)
c.Common.RegisterFlags(f, logger)
}

func (c *Config) CommonConfigInheritance() CommonConfigInheritance {
Expand Down Expand Up @@ -576,8 +576,8 @@ type CommonConfigInheritance struct {
}

// RegisterFlags registers flag.
func (c *CommonConfig) RegisterFlags(f *flag.FlagSet) {
c.Storage.RegisterFlagsWithPrefix("common.storage.", f)
func (c *CommonConfig) RegisterFlags(f *flag.FlagSet, logger log.Logger) {
c.Storage.RegisterFlagsWithPrefix("common.storage.", f, logger)
}

// configWithCustomCommonUnmarshaler unmarshals config with custom unmarshaler for the `common` field.
Expand Down
2 changes: 1 addition & 1 deletion pkg/mimir/mimir_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ type customExtendedConfig struct {

func (c *customExtendedConfig) RegisterFlags(f *flag.FlagSet, logger log.Logger) {
c.MimirConfig.RegisterFlags(f, logger)
c.CustomStorage.RegisterFlagsWithPrefix("custom-storage", f)
c.CustomStorage.RegisterFlagsWithPrefix("custom-storage", f, logger)
}

func (c *customExtendedConfig) CommonConfigInheritance() mimir.CommonConfigInheritance {
Expand Down
14 changes: 7 additions & 7 deletions pkg/mimirtool/commands/bucket_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,19 +105,19 @@ func (b *BucketValidationCommand) Register(app *kingpin.Application, _ EnvVarNam
}

func (b *BucketValidationCommand) validate(k *kingpin.ParseContext) error {
b.logger = log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr))
if b.bucketConfigHelp {
b.printBucketConfigHelp()
b.printBucketConfigHelp(b.logger)
return nil
}

err := b.parseBucketConfig()
err := b.parseBucketConfig(b.logger)
if err != nil {
return errors.Wrap(err, "error when parsing bucket config")
}

b.setObjectNames()
b.objectContent = "testData"
b.logger = log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr))
ctx := context.Background()

bucketClient, err := bucket.NewClient(ctx, b.cfg, "testClient", b.logger, nil)
Expand Down Expand Up @@ -163,9 +163,9 @@ func (b *BucketValidationCommand) validate(k *kingpin.ParseContext) error {
return nil
}

func (b *BucketValidationCommand) printBucketConfigHelp() {
func (b *BucketValidationCommand) printBucketConfigHelp(logger log.Logger) {
fs := flag.NewFlagSet("bucket-config", flag.ContinueOnError)
b.cfg.RegisterFlags(fs)
b.cfg.RegisterFlags(fs, logger)

fmt.Fprintf(fs.Output(), `
The following help text describes the arguments
Expand All @@ -179,9 +179,9 @@ mimirtool bucket-validation --bucket-config='-backend=s3 -s3.endpoint=localhost:
fs.Usage()
}

func (b *BucketValidationCommand) parseBucketConfig() error {
func (b *BucketValidationCommand) parseBucketConfig(logger log.Logger) error {
fs := flag.NewFlagSet("bucket-config", flag.ContinueOnError)
b.cfg.RegisterFlags(fs)
b.cfg.RegisterFlags(fs, logger)
err := fs.Parse(strings.Split(b.bucketConfig, " "))
if err != nil {
return err
Expand Down
5 changes: 3 additions & 2 deletions pkg/ruler/rulestore/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"flag"
"reflect"

"github.com/go-kit/log"
"github.com/grafana/dskit/flagext"

"github.com/grafana/mimir/pkg/ruler/rulestore/local"
Expand All @@ -22,12 +23,12 @@ type Config struct {
}

// RegisterFlags registers the backend storage config.
func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
func (cfg *Config) RegisterFlags(f *flag.FlagSet, logger log.Logger) {
prefix := "ruler-storage."

cfg.StorageBackendConfig.ExtraBackends = []string{local.Name}
cfg.Local.RegisterFlagsWithPrefix(prefix, f)
cfg.RegisterFlagsWithPrefixAndDefaultDirectory(prefix, "ruler", f)
cfg.RegisterFlagsWithPrefixAndDefaultDirectory(prefix, "ruler", f, logger)
}

// IsDefaults returns true if the storage options have not been set.
Expand Down
24 changes: 12 additions & 12 deletions pkg/storage/bucket/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,24 +81,24 @@ func (cfg *StorageBackendConfig) supportedBackends() []string {
}

// RegisterFlags registers the backend storage config.
func (cfg *StorageBackendConfig) RegisterFlags(f *flag.FlagSet) {
cfg.RegisterFlagsWithPrefix("", f)
func (cfg *StorageBackendConfig) RegisterFlags(f *flag.FlagSet, logger log.Logger) {
cfg.RegisterFlagsWithPrefix("", f, logger)
}

func (cfg *StorageBackendConfig) RegisterFlagsWithPrefixAndDefaultDirectory(prefix, dir string, f *flag.FlagSet) {
func (cfg *StorageBackendConfig) RegisterFlagsWithPrefixAndDefaultDirectory(prefix, dir string, f *flag.FlagSet, logger log.Logger) {
cfg.RegisteredFlags = util.TrackRegisteredFlags(prefix, f, func(prefix string, f *flag.FlagSet) {
cfg.S3.RegisterFlagsWithPrefix(prefix, f)
cfg.GCS.RegisterFlagsWithPrefix(prefix, f)
cfg.Azure.RegisterFlagsWithPrefix(prefix, f)
cfg.Azure.RegisterFlagsWithPrefix(prefix, f, logger)
cfg.Swift.RegisterFlagsWithPrefix(prefix, f)
cfg.Filesystem.RegisterFlagsWithPrefixAndDefaultDirectory(prefix, dir, f)

f.StringVar(&cfg.Backend, prefix+"backend", Filesystem, fmt.Sprintf("Backend storage to use. Supported backends are: %s.", strings.Join(cfg.supportedBackends(), ", ")))
})
}

func (cfg *StorageBackendConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
cfg.RegisterFlagsWithPrefixAndDefaultDirectory(prefix, "", f)
func (cfg *StorageBackendConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet, logger log.Logger) {
cfg.RegisterFlagsWithPrefixAndDefaultDirectory(prefix, "", f, logger)
}

func (cfg *StorageBackendConfig) Validate() error {
Expand Down Expand Up @@ -127,17 +127,17 @@ type Config struct {
}

// RegisterFlags registers the backend storage config.
func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
cfg.RegisterFlagsWithPrefix("", f)
func (cfg *Config) RegisterFlags(f *flag.FlagSet, logger log.Logger) {
cfg.RegisterFlagsWithPrefix("", f, logger)
}

func (cfg *Config) RegisterFlagsWithPrefixAndDefaultDirectory(prefix, dir string, f *flag.FlagSet) {
cfg.StorageBackendConfig.RegisterFlagsWithPrefixAndDefaultDirectory(prefix, dir, f)
func (cfg *Config) RegisterFlagsWithPrefixAndDefaultDirectory(prefix, dir string, f *flag.FlagSet, logger log.Logger) {
cfg.StorageBackendConfig.RegisterFlagsWithPrefixAndDefaultDirectory(prefix, dir, f, logger)
f.StringVar(&cfg.StoragePrefix, prefix+"storage-prefix", "", "Prefix for all objects stored in the backend storage. For simplicity, it may only contain digits and English alphabet letters.")
}

func (cfg *Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
cfg.RegisterFlagsWithPrefixAndDefaultDirectory(prefix, "", f)
func (cfg *Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet, logger log.Logger) {
cfg.RegisterFlagsWithPrefixAndDefaultDirectory(prefix, "", f, logger)
}

func (cfg *Config) Validate() error {
Expand Down
5 changes: 3 additions & 2 deletions pkg/storage/tsdb/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"time"

"github.com/alecthomas/units"
"github.com/go-kit/log"
"github.com/pkg/errors"
"github.com/prometheus/prometheus/tsdb/chunks"
"github.com/prometheus/prometheus/tsdb/wal"
Expand Down Expand Up @@ -124,8 +125,8 @@ func (d *DurationList) ToMilliseconds() []int64 {
}

// RegisterFlags registers the TSDB flags
func (cfg *BlocksStorageConfig) RegisterFlags(f *flag.FlagSet) {
cfg.Bucket.RegisterFlagsWithPrefixAndDefaultDirectory("blocks-storage.", "blocks", f)
func (cfg *BlocksStorageConfig) RegisterFlags(f *flag.FlagSet, logger log.Logger) {
cfg.Bucket.RegisterFlagsWithPrefixAndDefaultDirectory("blocks-storage.", "blocks", f, logger)
cfg.BucketStore.RegisterFlags(f)
cfg.TSDB.RegisterFlags(f)
}
Expand Down
8 changes: 5 additions & 3 deletions pkg/storage/tsdb/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"testing"
"time"

"github.com/go-kit/log"
"github.com/grafana/dskit/flagext"
"github.com/stretchr/testify/assert"

Expand Down Expand Up @@ -138,6 +139,7 @@ func TestConfig_Validate(t *testing.T) {

func TestConfig_DurationList(t *testing.T) {
t.Parallel()
nopLogger := log.NewNopLogger()

tests := map[string]struct {
cfg BlocksStorageConfig
Expand All @@ -148,7 +150,7 @@ func TestConfig_DurationList(t *testing.T) {
cfg: BlocksStorageConfig{},
expectedRanges: []int64{7200000},
f: func(c *BlocksStorageConfig) {
c.RegisterFlags(&flag.FlagSet{})
c.RegisterFlags(&flag.FlagSet{}, nopLogger)
},
},
"parse ranges correctly": {
Expand All @@ -168,8 +170,8 @@ func TestConfig_DurationList(t *testing.T) {
cfg: BlocksStorageConfig{},
expectedRanges: []int64{7200000},
f: func(c *BlocksStorageConfig) {
c.RegisterFlags(&flag.FlagSet{})
c.RegisterFlags(&flag.FlagSet{})
c.RegisterFlags(&flag.FlagSet{}, nopLogger)
c.RegisterFlags(&flag.FlagSet{}, nopLogger)
},
},
}
Expand Down
6 changes: 3 additions & 3 deletions tools/compaction-planner/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,10 @@ func main() {
sorting string
}{}

logger := gokitlog.NewNopLogger()

// Loads bucket index, and plans compaction for all loaded meta files.
cfg.bucket.RegisterFlags(flag.CommandLine)
cfg.bucket.RegisterFlags(flag.CommandLine, logger)
cfg.blockRanges = mimir_tsdb.DurationList{2 * time.Hour, 12 * time.Hour, 24 * time.Hour}
flag.Var(&cfg.blockRanges, "block-ranges", "List of compaction time ranges.")
flag.StringVar(&cfg.userID, "user", "", "User (tenant)")
Expand All @@ -52,8 +54,6 @@ func main() {
log.Fatalln("no user specified")
}

logger := gokitlog.NewNopLogger()

ctx, cancel := signal.NotifyContext(context.Background(), syscall.SIGINT)
defer cancel()

Expand Down
4 changes: 2 additions & 2 deletions tools/list-deduplicated-blocks/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ func main() {
showCreationTime bool
}{}

cfg.bucket.RegisterFlags(flag.CommandLine)
logger := gokitlog.NewNopLogger()
cfg.bucket.RegisterFlags(flag.CommandLine, logger)
flag.StringVar(&cfg.userID, "user", "", "User (tenant)")
flag.Parse()

Expand All @@ -51,7 +52,6 @@ func main() {
ctx, cancel := signal.NotifyContext(context.Background(), syscall.SIGINT)
defer cancel()

logger := gokitlog.NewNopLogger()
bkt, err := bucket.NewClient(ctx, cfg.bucket, "bucket", logger, nil)
if err != nil {
log.Fatalln("failed to create bucket:", err)
Expand Down

0 comments on commit ed80907

Please sign in to comment.