Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate elasticsearch rollover to go #3242

Merged
merged 31 commits into from
Sep 9, 2021

Conversation

rubenvp8510
Copy link
Contributor

@rubenvp8510 rubenvp8510 commented Sep 2, 2021

Part of the work for migrate away from curator #3179

Which problem is this PR solving?

  • This is a first PR of the work to migrate the rollover to golang

Short description of the changes

  • Extend the index client created on the index cleaner PR, added functionality to rollover, and alias handling.
  • Implemented the three actions of the original rollover python script: init, rollover, lookback
  • Added unit tests

Future PRs:

  • add docker build and include this in ES integration tests
  • remove old python implementation

@rubenvp8510 rubenvp8510 requested a review from a team as a code owner September 2, 2021 07:18
cmd/es-rollover/app/flags.go Outdated Show resolved Hide resolved
cmd/es-rollover/app/index_set.go Outdated Show resolved Hide resolved
cmd/es-rollover/app/index_set.go Outdated Show resolved Hide resolved
cmd/es-rollover/app/index_set.go Outdated Show resolved Hide resolved
cmd/es-rollover/app/initialize/cmd.go Outdated Show resolved Hide resolved
cmd/es-rollover/main.go Outdated Show resolved Hide resolved
cmd/es-rollover/app/basic_auth.go Outdated Show resolved Hide resolved
cmd/es-rollover/app/alias_filter.go Outdated Show resolved Hide resolved
cmd/es-rollover/app/initialize/cmd.go Outdated Show resolved Hide resolved
cmd/es-rollover/app/initialize/cmd.go Outdated Show resolved Hide resolved
@rubenvp8510 rubenvp8510 force-pushed the es-rollover-init branch 2 times, most recently from 75894e5 to 00ace9f Compare September 3, 2021 05:52
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good progress.

We should fix the PR to pass the CI and add tests.

cmd/es-rollover/app/flags.go Outdated Show resolved Hide resolved
cmd/es-rollover/app/init/flags.go Outdated Show resolved Hide resolved
cmd/es-rollover/app/lookback/action.go Outdated Show resolved Hide resolved
cmd/es-rollover/app/lookback/flags.go Outdated Show resolved Hide resolved
}

// InitFromViper initializes config from viper.Viper.
func (c *Config) InitFromViper(v *viper.Viper) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how are the common flags initialized?

@rubenvp8510 rubenvp8510 force-pushed the es-rollover-init branch 3 times, most recently from 292f897 to deaa39b Compare September 6, 2021 05:24
@codecov
Copy link

codecov bot commented Sep 6, 2021

Codecov Report

Merging #3242 (8778c27) into master (8427e01) will decrease coverage by 0.14%.
The diff coverage is 95.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3242      +/-   ##
==========================================
- Coverage   95.99%   95.85%   -0.15%     
==========================================
  Files         242      259      +17     
  Lines       14815    15398     +583     
==========================================
+ Hits        14221    14759     +538     
- Misses        514      550      +36     
- Partials       80       89       +9     
Impacted Files Coverage Δ
cmd/collector/app/handler/grpc_handler.go 85.71% <ø> (ø)
pkg/es/client/client.go 87.03% <87.03%> (ø)
pkg/es/client/index_client.go 92.71% <92.71%> (ø)
cmd/es-rollover/app/init/action.go 93.75% <93.75%> (ø)
cmd/es-index-cleaner/app/index_filter.go 100.00% <100.00%> (ø)
cmd/es-rollover/app/actions.go 100.00% <100.00%> (ø)
cmd/es-rollover/app/flags.go 100.00% <100.00%> (ø)
cmd/es-rollover/app/index_options.go 100.00% <100.00%> (ø)
cmd/es-rollover/app/init/flags.go 100.00% <100.00%> (ø)
cmd/es-rollover/app/lookback/action.go 100.00% <100.00%> (ø)
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7453387...8778c27. Read the comment docs.

Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, now we should increase the test coverage.

cmd/es-rollover/app/init/flags.go Outdated Show resolved Hide resolved
@rubenvp8510 rubenvp8510 force-pushed the es-rollover-init branch 2 times, most recently from 5d43322 to 8795973 Compare September 6, 2021 23:36
cmd/es-rollover/app/actions_test.go Outdated Show resolved Hide resolved
cmd/es-rollover/app/flags_test.go Outdated Show resolved Hide resolved
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
…onality

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@pavolloffay pavolloffay changed the title [WIP] Migrate elasticsearch rollover to go Migrate elasticsearch rollover to go Sep 7, 2021
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
cmd/es-rollover/main.go Outdated Show resolved Hide resolved
)
}

func addRootFlags(v *viper.Viper, tlsFlags *tlscfg.ClientFlagsConfig, rootCmd *cobra.Command) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would rename this to align with cobra terminology - persistent flags and the function could have the same signature as config.AddFlags.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better if the action config structs called added flags on the genetric config and do the same for the init viper.

// AddFlags adds flags for TLS to the FlagSet.
func (c *Config) AddFlags(flags *flag.FlagSet) {
	flags.String(unit, defaultUnit, "used with lookback to remove indices from read alias e.g, days, weeks, months, years")
	flags.Int(unitCount, defaultUnitCount, "count of UNITs")
	c.Config.AddFlags(flags)
}

// InitFromViper initializes config from viper.Viper.
func (c *Config) InitFromViper(v *viper.Viper) {
	c.Unit = v.GetString(unit)
	c.UnitCount = v.GetInt(unitCount)
	c.InitFromViper(v)
}

this way caller can be sure that the object is initalized property.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried in this way and for some reason the repeated flags binds to different commands are not recognized, I was able to pass the flag, but wasn't able to recovery the value. So I leave as it is, using root persistent flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed the function and changed a little bit the signature, but still use persistent flags, so I add global flags to root, and call AddFlags for other subcommands config.

cmd/es-rollover/app/rollover/action.go Outdated Show resolved Hide resolved
cmd/es-rollover/app/lookback/action.go Outdated Show resolved Hide resolved
pkg/es/client/client.go Outdated Show resolved Hide resolved
pkg/es/client/index_client.go Show resolved Hide resolved
pkg/es/client/mocks/ilm_client.go Show resolved Hide resolved
@rubenvp8510 rubenvp8510 force-pushed the es-rollover-init branch 3 times, most recently from 64999f6 to 9638822 Compare September 9, 2021 01:44
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@pavolloffay pavolloffay merged commit f5bb954 into jaegertracing:master Sep 9, 2021
@jpkrohling jpkrohling added this to the v1.27.0 milestone Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants