adding priority-class for esIndexCleaner #1732

swapnilpotnis · 2022-02-01T11:03:33Z

Signed-off-by: Swapnil Potnis [email protected]

Which problem is this PR solving?

PriorityClass for jaeger-es-index-cleaner #1620

Short description of the changes

Adding the attribute of "priorityclassName" for "esIndexCleaner"

djsly · 2022-02-01T16:01:26Z

thanks @swapnilpotnis for this effort!

rubenvp8510 · 2022-02-02T00:58:40Z

The linter is complaining. You need to regenerate the bundle.

You can use the following command.
VERSION=1.30.0 USER=jaegertracing make bundle

Signed-off-by: Swapnil Potnis <[email protected]>

swapnilpotnis · 2022-02-02T12:29:26Z

@rubenvp8510 : Done.

rubenvp8510 · 2022-02-04T15:48:10Z

DCO is failing, You need to sign your commits :)

codecov · 2022-02-04T17:19:52Z

Codecov Report

Merging #1732 (c3728f5) into main (d60fada) will decrease coverage by 0.02%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main    #1732      +/-   ##
==========================================
- Coverage   89.45%   89.42%   -0.03%     
==========================================
  Files         100      100              
  Lines        6116     6118       +2     
==========================================
  Hits         5471     5471              
- Misses        470      471       +1     
- Partials      175      176       +1

Impacted Files	Coverage Δ
apis/v1/jaeger_types.go	`87.50% <ø> (ø)`
pkg/cronjob/es_index_cleaner.go	`97.22% <50.00%> (-2.78%)`	⬇️
pkg/deployment/query.go	`100.00% <100.00%> (ø)`

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 d60fada...c3728f5. Read the comment docs.

rubenvp8510 · 2022-02-11T01:24:34Z

Hi @swapnilpotnis are you still interesting on this one? If so please rebase and sign your commits I think after that it will be ready to be merged :)

Thanks for your contribution!

swapnilpotnis · 2022-02-11T03:57:21Z

Really sorry for the late response. Will fix it up by today itself and have the PR ready for review.

Signed-off-by: Swapnil Potnis <[email protected]>

swapnilpotnis · 2022-03-04T10:22:35Z

@rubenvp8510 : hey...I have added the unit test case as well as updated the PR with the latest branch commit. Can you please take a look?

swapnilpotnis · 2022-03-15T04:20:55Z

@rubenvp8510 : can you please allow the tests to be run over the PR again?

swapnilpotnis · 2022-03-17T09:40:49Z

@rubenvp8510 : the earlier check seemed to have failed with the error "Build failed: the api.md file has been changed but the generated api.md file isn't up to date. Run 'make api-docs' and update your PR." (https://github.com/jaegertracing/jaeger-operator/runs/5548885043?check_suite_focus=true#step:6:171) but running the command states that no rule has been found to the target "api-docs". I am a bit confused as to how to deal with the error and google wasn't helpful as well in that aspect. Can you please provide me some pointers??

swapnilpotnis · 2022-03-22T04:36:03Z

@rubenvp8510 : the earlier check seemed to have failed with the error "Build failed: the api.md file has been changed but the generated api.md file isn't up to date. Run 'make api-docs' and update your PR." (https://github.com/jaegertracing/jaeger-operator/runs/5548885043?check_suite_focus=true#step:6:171) but running the command states that no rule has been found to the target "api-docs". I am a bit confused as to how to deal with the error and google wasn't helpful as well in that aspect. Can you please provide me some pointers??

@rubenvp8510 : can you please help me with this??

rubenvp8510 · 2022-03-23T02:30:14Z

@rubenvp8510 : the earlier check seemed to have failed with the error "Build failed: the api.md file has been changed but the generated api.md file isn't up to date. Run 'make api-docs' and update your PR." (https://github.com/jaegertracing/jaeger-operator/runs/5548885043?check_suite_focus=true#step:6:171) but running the command states that no rule has been found to the target "api-docs". I am a bit confused as to how to deal with the error and google wasn't helpful as well in that aspect. Can you please provide me some pointers??

@rubenvp8510 : can you please help me with this??

Hi did you rebase your PR? try to run it with the PR rebased on top of main. I checkout your branch and I was able to run it and generated the doc file. I can also push a commit with the change if you want.

You should see this rule on your Makefile: https://github.com/jaegertracing/jaeger-operator/blob/main/Makefile#L496

Signed-off-by: Swapnil Potnis <[email protected]>

swapnilpotnis · 2022-03-23T13:07:45Z

@rubenvp8510 : thanks for your help. I have updated the PR with the required changes. Can you please run the checks??

Signed-off-by: Swapnil Potnis <[email protected]>

swapnilpotnis · 2022-03-23T13:43:31Z

@rubenvp8510 : Forgive me as I wasn't aware of the naming convention that was suppose to be followed for which the earlier check had failed. Can you please re-run the tests as I have rectified the variable name as part of the latest commit?

rubenvp8510 · 2022-03-23T14:20:44Z

@rubenvp8510 : Forgive me as I wasn't aware of the naming convention that was suppose to be followed for which the earlier check had failed. Can you please re-run the tests as I have rectified the variable name as part of the latest commit?

No problem :)

rubenvp8510 · 2022-03-23T14:34:17Z

2022-03-23T14:25:57.3007810Z --- FAIL: TestPriorityClassName (0.00s) :(

Signed-off-by: Swapnil Potnis <[email protected]>

swapnilpotnis · 2022-04-01T12:58:49Z

@rubenvp8510 : can you please run the tests for the latest commit ?

swapnilpotnis · 2022-04-01T23:15:55Z

@rubenvp8510 : alas..!!! The pipelines finally Green :)
Can you also please review the PR whenever you have some bandwidth??

frzifus

hi @swapnilpotnis, thanks for your pr. I have only 2 last questions :)

apis/v1/jaeger_types.go

frzifus · 2022-04-04T09:53:05Z

pkg/cronjob/es_index_cleaner.go

@@ -45,6 +45,11 @@ func CreateEsIndexCleaner(jaeger *v1.Jaeger) *batchv1beta1.CronJob {

 	ca.Update(jaeger, commonSpec)

+	priorityClassName := ""
+	if jaeger.Spec.Storage.EsIndexCleaner.PriorityClassName != "" {


Do we need this condition?
if the field is not empty, we update priorityClassName. But if its empty, we dont update priorityClassName that is initialized with an empty string.

means it is equal to just priorityClassName := jaeger.Spec.Storage.EsIndexCleaner.PriorityClassName right?

Would it then make sense to assign it directly in the metav1 object?

Agree on this comment.

Actually I looked at other parts of the code where priorityClass has been reference and this was the general approach followed elsewhere as well. Having to go with the same flow, I implemented the same set of changes

ref:

https://github.com/jaegertracing/jaeger-operator/blob/main/pkg/deployment/agent.go#L92

https://github.com/jaegertracing/jaeger-operator/blob/main/pkg/deployment/query.go#L89

Let me know if you guys wish to have the assignment in the metav1 object itself so that I can make the changes. Also let me know if you guys wish to have the same set of changes made at other places as well. If that be the case, should I do it in the same PR or create a new PR altogether??

@rubenvp8510 / @frzifus : Guys, can you please share your inputs over it?

Sorry for the long response. From my point of view it would make sense to change it at least in this pr. But feel free to change it on the other places too in this or a new pr. What do you think?

Sure. Will make the necessary change only for the priorityClass which is part of this PR.
And would create a new PR for the other 2 PriorityClass.

I would say only do the change related to this PR, we can do other changes in a separate PR

still a bit confused, isnt it the same like?

Suggested change

if jaeger.Spec.Storage.EsIndexCleaner.PriorityClassName != "" {

priorityClassName := "jaeger.Spec.Storage.EsIndexCleaner.PriorityClassName"

up to you @rubenvp8510

I agree with the change, but for not blocking this PR we can do the changes in a follow up PR.

Signed-off-by: Swapnil Potnis <[email protected]>

swapnilpotnis · 2022-04-12T13:09:21Z

@rubenvp8510 / @frzifus : Can you guys please review my PR?

swapnilpotnis · 2022-04-13T05:53:20Z

@rubenvp8510: thanks alot :)
will soon create a PR for the priorityClass change at other 2 places

adding priority-class for esIndexCleaner and fixing lint error

f9e8d67

Signed-off-by: Swapnil Potnis <[email protected]>

swapnilpotnis force-pushed the priorityClass-esIndexCleaner branch from 1c067dd to f9e8d67 Compare February 2, 2022 12:22

swapnilpotnis force-pushed the priorityClass-esIndexCleaner branch from c34fd9b to f76e31a Compare February 4, 2022 06:07

swapnilpotnis force-pushed the priorityClass-esIndexCleaner branch from ddf6137 to 6f4c3f0 Compare February 11, 2022 04:25

reverting back the operator-sdk version and rebasing

f6ee0dd

Signed-off-by: Swapnil Potnis <[email protected]>

swapnilpotnis force-pushed the priorityClass-esIndexCleaner branch 3 times, most recently from 6ce380f to 5646a21 Compare February 12, 2022 07:20

swapnilpotnis force-pushed the priorityClass-esIndexCleaner branch from 7b3e94e to c4aa417 Compare March 4, 2022 10:19

unit test case for priorityClass

bff72dd

Signed-off-by: Swapnil Potnis <[email protected]>

swapnilpotnis force-pushed the priorityClass-esIndexCleaner branch from c4aa417 to bff72dd Compare March 4, 2022 10:20

Merge branch 'main' into priorityClass-esIndexCleaner

91137fc

swapnilpotnis added 2 commits March 4, 2022 19:31

Merge branch 'main' into priorityClass-esIndexCleaner

04166c4

Merge branch 'main' into priorityClass-esIndexCleaner

c8fa1da

Merge branch 'main' into priorityClass-esIndexCleaner

41825c8

swapnilpotnis and others added 2 commits March 22, 2022 10:06

Merge branch 'main' into priorityClass-esIndexCleaner

801c5dc

Merge branch 'main' into priorityClass-esIndexCleaner

f5b93ca

swapnilpotnis force-pushed the priorityClass-esIndexCleaner branch from 230d838 to 7a8f8f5 Compare March 23, 2022 13:02

api doc changes

980c6c5

Signed-off-by: Swapnil Potnis <[email protected]>

swapnilpotnis force-pushed the priorityClass-esIndexCleaner branch from 7a8f8f5 to 980c6c5 Compare March 23, 2022 13:06

changes to variable name as per required convention

a7b3109

Signed-off-by: Swapnil Potnis <[email protected]>

swapnilpotnis and others added 4 commits March 31, 2022 20:19

Merge branch 'main' into priorityClass-esIndexCleaner

cbc24fd

Merge branch 'main' into priorityClass-esIndexCleaner

324d061

fixing test case failure for priorityClass

6aee06d

Signed-off-by: Swapnil Potnis <[email protected]>

Merge branch 'main' into priorityClass-esIndexCleaner

d4f7147

frzifus reviewed Apr 4, 2022

View reviewed changes

swapnilpotnis added 3 commits April 7, 2022 15:39

Merge branch 'main' into priorityClass-esIndexCleaner

9af705e

Merge branch 'main' into priorityClass-esIndexCleaner

5b023b6

changes as per review comments

f7f292b

Signed-off-by: Swapnil Potnis <[email protected]>

swapnilpotnis force-pushed the priorityClass-esIndexCleaner branch from 084c7ef to f7f292b Compare April 12, 2022 12:28

rubenvp8510 approved these changes Apr 12, 2022

View reviewed changes

rubenvp8510 added 2 commits April 12, 2022 23:59

Merge branch 'main' into priorityClass-esIndexCleaner

1b11bee

Merge branch 'main' into priorityClass-esIndexCleaner

c3728f5

rubenvp8510 enabled auto-merge (squash) April 13, 2022 05:13

rubenvp8510 merged commit 2969950 into jaegertracing:main Apr 13, 2022

frzifus mentioned this pull request Apr 13, 2022

Preparing release v1.33.0 #1853

Merged

Nerkho mentioned this pull request Jun 21, 2022

feat(tracing-apps): update jaeger-operator from 2.29.* to 2.32.2 adfinis/helm-charts#733

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding priority-class for esIndexCleaner #1732

adding priority-class for esIndexCleaner #1732

swapnilpotnis commented Feb 1, 2022 •

edited

Loading

djsly commented Feb 1, 2022

rubenvp8510 commented Feb 2, 2022

swapnilpotnis commented Feb 2, 2022

rubenvp8510 commented Feb 4, 2022

codecov bot commented Feb 4, 2022 •

edited

Loading

rubenvp8510 commented Feb 11, 2022

swapnilpotnis commented Feb 11, 2022

swapnilpotnis commented Mar 4, 2022

swapnilpotnis commented Mar 15, 2022

swapnilpotnis commented Mar 17, 2022

swapnilpotnis commented Mar 22, 2022

rubenvp8510 commented Mar 23, 2022 •

edited

Loading

swapnilpotnis commented Mar 23, 2022

swapnilpotnis commented Mar 23, 2022

rubenvp8510 commented Mar 23, 2022

rubenvp8510 commented Mar 23, 2022

swapnilpotnis commented Apr 1, 2022

swapnilpotnis commented Apr 1, 2022

frzifus left a comment

frzifus Apr 4, 2022

rubenvp8510 Apr 6, 2022

swapnilpotnis Apr 7, 2022

swapnilpotnis Apr 7, 2022

swapnilpotnis Apr 11, 2022

frzifus Apr 11, 2022

swapnilpotnis Apr 11, 2022

rubenvp8510 Apr 11, 2022

frzifus Apr 12, 2022

rubenvp8510 Apr 12, 2022

swapnilpotnis commented Apr 12, 2022

swapnilpotnis commented Apr 13, 2022

	if jaeger.Spec.Storage.EsIndexCleaner.PriorityClassName != "" {
	priorityClassName := "jaeger.Spec.Storage.EsIndexCleaner.PriorityClassName"

adding priority-class for esIndexCleaner #1732

adding priority-class for esIndexCleaner #1732

Conversation

swapnilpotnis commented Feb 1, 2022 • edited Loading

Which problem is this PR solving?

Short description of the changes

djsly commented Feb 1, 2022

rubenvp8510 commented Feb 2, 2022

swapnilpotnis commented Feb 2, 2022

rubenvp8510 commented Feb 4, 2022

codecov bot commented Feb 4, 2022 • edited Loading

Codecov Report

rubenvp8510 commented Feb 11, 2022

swapnilpotnis commented Feb 11, 2022

swapnilpotnis commented Mar 4, 2022

swapnilpotnis commented Mar 15, 2022

swapnilpotnis commented Mar 17, 2022

swapnilpotnis commented Mar 22, 2022

rubenvp8510 commented Mar 23, 2022 • edited Loading

swapnilpotnis commented Mar 23, 2022

swapnilpotnis commented Mar 23, 2022

rubenvp8510 commented Mar 23, 2022

rubenvp8510 commented Mar 23, 2022

swapnilpotnis commented Apr 1, 2022

swapnilpotnis commented Apr 1, 2022

frzifus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swapnilpotnis commented Apr 12, 2022

swapnilpotnis commented Apr 13, 2022

swapnilpotnis commented Feb 1, 2022 •

edited

Loading

codecov bot commented Feb 4, 2022 •

edited

Loading

rubenvp8510 commented Mar 23, 2022 •

edited

Loading