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.

Sign up for GitHub

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

adding priority-class for esIndexCleaner #1732

Merged
merged 20 commits into from
Apr 13, 2022
Merged

adding priority-class for esIndexCleaner #1732

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
f9e8d67
adding priority-class for esIndexCleaner and fixing lint error
swapnilpotnis Feb 1, 2022
f6ee0dd
reverting back the operator-sdk version and rebasing
swapnilpotnis Feb 4, 2022
bff72dd
unit test case for priorityClass
swapnilpotnis Mar 4, 2022
91137fc
Merge branch 'main' into priorityClass-esIndexCleaner
swapnilpotnis Mar 4, 2022
04166c4
Merge branch 'main' into priorityClass-esIndexCleaner
swapnilpotnis Mar 4, 2022
c8fa1da
Merge branch 'main' into priorityClass-esIndexCleaner
swapnilpotnis Mar 15, 2022
41825c8
Merge branch 'main' into priorityClass-esIndexCleaner
swapnilpotnis Mar 17, 2022
801c5dc
Merge branch 'main' into priorityClass-esIndexCleaner
swapnilpotnis Mar 22, 2022
f5b93ca
Merge branch 'main' into priorityClass-esIndexCleaner
rubenvp8510 Mar 23, 2022
980c6c5
api doc changes
swapnilpotnis Mar 23, 2022
a7b3109
changes to variable name as per required convention
swapnilpotnis Mar 23, 2022
cbc24fd
Merge branch 'main' into priorityClass-esIndexCleaner
swapnilpotnis Mar 31, 2022
324d061
Merge branch 'main' into priorityClass-esIndexCleaner
rubenvp8510 Apr 1, 2022
6aee06d
fixing test case failure for priorityClass
swapnilpotnis Apr 1, 2022
d4f7147
Merge branch 'main' into priorityClass-esIndexCleaner
swapnilpotnis Apr 1, 2022
9af705e
Merge branch 'main' into priorityClass-esIndexCleaner
swapnilpotnis Apr 7, 2022
5b023b6
Merge branch 'main' into priorityClass-esIndexCleaner
swapnilpotnis Apr 11, 2022
f7f292b
changes as per review comments
swapnilpotnis Apr 12, 2022
1b11bee
Merge branch 'main' into priorityClass-esIndexCleaner
rubenvp8510 Apr 13, 2022
c3728f5
Merge branch 'main' into priorityClass-esIndexCleaner
rubenvp8510 Apr 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions apis/v1/jaeger_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,6 +681,9 @@ type JaegerEsIndexCleanerSpec struct {

// +optional
JaegerCommonSpec `json:",inline,omitempty"`

// +optional
PriorityClassName string `json:"priorityClassName,omitempty"`
swapnilpotnis marked this conversation as resolved.
Show resolved Hide resolved
}

// JaegerEsRolloverSpec holds the options related to es-rollover
Expand Down
2 changes: 2 additions & 0 deletions bundle/manifests/jaegertracing.io_jaegers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10029,6 +10029,8 @@ spec:
type: object
numberOfDays:
type: integer
priorityClassName:
type: string
resources:
nullable: true
properties:
Expand Down
2 changes: 2 additions & 0 deletions config/crd/bases/jaegertracing.io_jaegers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10027,6 +10027,8 @@ spec:
type: object
numberOfDays:
type: integer
priorityClassName:
type: string
resources:
nullable: true
properties:
Expand Down
7 changes: 7 additions & 0 deletions docs/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37720,6 +37720,13 @@ Resource Types:
<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>priorityClassName</b></td>
<td>string</td>
<td>
<br/>
</td>
<td>false</td>
</tr><tr>
<td><b><a href="#jaegerspecstorageesindexcleanerresources">resources</a></b></td>
<td>object</td>
Expand Down
6 changes: 6 additions & 0 deletions pkg/cronjob/es_index_cleaner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ func CreateEsIndexCleaner(jaeger *v1.Jaeger) *batchv1beta1.CronJob {

ca.Update(jaeger, commonSpec)

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

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree on this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

priorityClassName = jaeger.Spec.Storage.EsIndexCleaner.PriorityClassName
}

return &batchv1beta1.CronJob{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Expand Down Expand Up @@ -88,6 +93,7 @@ func CreateEsIndexCleaner(jaeger *v1.Jaeger) *batchv1beta1.CronJob {
SecurityContext: commonSpec.SecurityContext,
ServiceAccountName: account.JaegerServiceAccountFor(jaeger, account.EsIndexCleanerComponent),
Volumes: commonSpec.Volumes,
PriorityClassName: priorityClassName,
},
ObjectMeta: metav1.ObjectMeta{
Labels: commonSpec.Labels,
Expand Down
9 changes: 9 additions & 0 deletions pkg/cronjob/es_index_cleaner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,3 +232,12 @@ func TestCustomEsIndexCleanerImage(t *testing.T) {
assert.Empty(t, jaeger.Spec.Storage.EsIndexCleaner.Image)
assert.Equal(t, "org/custom-es-index-cleaner-image:"+version.Get().Jaeger, cjob.Spec.JobTemplate.Spec.Template.Spec.Containers[0].Image)
}

// Test Case for PriorityClassName
func TestPriorityClassName(t *testing.T) {
jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestPriorityClassName"})

priorityClassNameVal := ""

assert.Equal(t, priorityClassNameVal, jaeger.Spec.Storage.EsIndexCleaner.PriorityClassName)
}
5 changes: 1 addition & 4 deletions pkg/deployment/query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,7 @@ func (q *Query) Get() *appsv1.Deployment {
// see https://github.com/jaegertracing/jaeger-operator/issues/334
sort.Strings(options)

priorityClassName := ""
if q.jaeger.Spec.Query.PriorityClassName != "" {
priorityClassName = q.jaeger.Spec.Query.PriorityClassName
}
priorityClassName := q.jaeger.Spec.Query.PriorityClassName

strategy := appsv1.DeploymentStrategy{
Type: appsv1.RecreateDeploymentStrategyType,
Expand Down