Cronjob migration #1856

kevinearls · 2022-04-13T13:08:41Z

Signed-off-by: Kevin Earls [email protected]

Which problem is this PR solving?

This resolves Migrate CronJob from batch/v1beta1 to batch/v1 #1585 migration of CronJob from batch/v1beta1 to batch/v1. This is not a pure migration as we will have situations where we need to create an image which will run both on versions of Kubernetes where batch/v1 is not yet available (11.9 and 1.20) and versions where batch/v1beta1 is no longer available. (1.25 and newer) See the Kubernetes Deprecated API Migration guide for details

Short description of the changes

The function detectCronjobsVersion has been added to pkg/autodetect/main to determine what version of the API is available. To simplify migration CronJobs are passed as runtime.Object and cast where needed.

Signed-off-by: Kevin Earls <[email protected]>

codecov · 2022-04-13T13:13:43Z

Codecov Report

Merging #1856 (0a6f84d) into main (c09b71e) will decrease coverage by 1.47%.
The diff coverage is 67.45%.

@@            Coverage Diff             @@
##             main    #1856      +/-   ##
==========================================
- Coverage   89.44%   87.96%   -1.48%     
==========================================
  Files         100      100              
  Lines        6128     6274     +146     
==========================================
+ Hits         5481     5519      +38     
- Misses        471      558      +87     
- Partials      176      197      +21

Impacted Files	Coverage Δ
apis/v1/jaeger_types.go	`87.50% <ø> (ø)`
pkg/controller/jaeger/cronjob.go	`47.45% <38.29%> (-30.33%)`	⬇️
pkg/strategy/production.go	`92.77% <53.84%> (-7.23%)`	⬇️
pkg/strategy/streaming.go	`94.48% <53.84%> (-4.07%)`	⬇️
pkg/cronjob/es_rollover.go	`78.75% <66.21%> (-17.19%)`	⬇️
pkg/inventory/cronjob.go	`71.42% <68.00%> (-28.58%)`	⬇️
pkg/cronjob/spark_dependencies.go	`83.20% <78.33%> (-9.40%)`	⬇️
pkg/autodetect/main.go	`86.51% <78.57%> (-0.68%)`	⬇️
pkg/cronjob/es_index_cleaner.go	`84.09% <80.64%> (-13.14%)`	⬇️
pkg/strategy/all_in_one.go	`95.74% <100.00%> (ø)`
... and 2 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 c09b71e...0a6f84d. Read the comment docs.

Signed-off-by: Kevin Earls <[email protected]>

…ator into cronjob-migration

Signed-off-by: Kevin Earls <[email protected]>

kevinearls · 2022-04-26T12:29:01Z

@rubenvp8510 @pavolloffay Please review. I realize this reduces code coverage slightly, but updating the tests to cover both batch/v1 and batch/v1beta1 would reuire lots of cut and paste or code changes that would be difficult to back out when we no longer need to support batch/v1beta1

frzifus

Looks good, i just have some formal questions :)

frzifus · 2022-04-26T13:07:43Z

pkg/autodetect/main.go

+	for _, apiGroupVersion := range apiGroupVersions {
+		groupAPIList, err := b.dcl.ServerResourcesForGroupVersion(apiGroupVersion)
+		if err != nil {
+			log.Errorf("Error getting %s api list: %v", apiGroupVersion, err)


Is there a way to get a logger form context?

iam asking because of this #1817

Sorry, there might be, but I don't know how to do that.

I think its something like

import "sigs.k8s.io/controller-runtime/pkg/log" ... logger := log.FromContext(ctx)

Sure. Can we do that as part of #1817 though?

pkg/controller/jaeger/cronjob.go

frzifus · 2022-04-26T13:24:38Z

pkg/cronjob/es_rollover.go

+				Schedule:                   jaeger.Spec.Storage.EsRollover.Schedule,
+				SuccessfulJobsHistoryLimit: jaeger.Spec.Storage.EsRollover.SuccessfulJobsHistoryLimit,
+				JobTemplate: batchv1beta1.JobTemplateSpec{
+					Spec: batchv1.JobSpec{


Does it need to be a batchv1beta1.JobSpec here?

Another thing, i wonder if we could get rid of this huge if else thing. while scrolling up and down i noticed it a few times. From my point of view, its easy to make mistakes in case of adaption. On the other hand, if we want to support a 3rd version in future releases, it feels be hard to extend too.

if cronjobsVersion == v1.CronJobsVersionBatchV1Beta1 { .. } else { .. } // replaced by func newCronJob(cronJobVer, ObjectMeta, ...opts) (interface{}, error) // or a structure that can be converted into both? type ourCronJob struct {} func (ourCronJob) convert(cronJobVer) (any, bool)

What do you think?

Actually batchv1.JobSpec is available as in k8s 1.19 (See https://github.com/kubernetes/api/blob/release-1.19/batch/v1beta1/types.go#L53) I have extracted the JobSpec definition to make things a bit shorter.

I would love to get rid of the big if/else statements, and tried to do so as much as possible. However, I guess I tried to balance the amount of work vs. benefits. We're hopefully not going to be supporting two versions of the same API in one image for very long. Once 1.21 is the oldest API we have to support we can just rip all of this stuff out.

I'll take another look in the morning though and see if I can get anything working

frzifus · 2022-04-26T13:26:09Z

pkg/controller/jaeger/cronjob.go

+				return tracing.HandleError(err, span)
+			}
+		}
+	} else {


Signed-off-by: Kevin Earls <[email protected]>

pavolloffay · 2022-04-26T12:38:00Z

apis/v1/jaeger_types.go

@@ -17,6 +17,12 @@ type JaegerPhase string
 type JaegerStorageType string

 const (
+	// CronJobsVersionBatchV1 represents the batch/v1 version of the kubernetes CronJob api, available as of 1.21
+	CronJobsVersionBatchV1 = "batch/v1"


These (and the one below) env vars do not belong here.

This file/package should contain types are used in the CRD.

@pavolloffay where should I define these?

Actually FlagPlatformOpenShift is defined in this file as well so for consistency we can keep it here.

It the batch version is exposed as flag (similar to platform) then I would suggest aligning the name to Flag as well.

Signed-off-by: Kevin Earls <[email protected]>

kevinearls · 2022-04-28T08:09:14Z

@rubenvp8510 @pavolloffay Please review

frzifus

up to the others :)

kevinearls · 2022-05-03T08:01:20Z

@rubenvp8510 Thanks for the approval. Can you merge this?

kevinearls added 2 commits April 13, 2022 14:55

Start migration of the CronJobs API from batch/v1beta1 to batchv1

198e682

Signed-off-by: Kevin Earls <[email protected]>

Revert local changes

688607e

Signed-off-by: Kevin Earls <[email protected]>

kevinearls added 9 commits April 14, 2022 09:57

Trying to stay out of rebase hell

b04d4be

Signed-off-by: Kevin Earls <[email protected]>

Try running just on k8s 1.19

fb7e2f5

Signed-off-by: Kevin Earls <[email protected]>

Merge branch 'main' into cronjob-migration

8c1d664

Fix lists of existing cronjobs

a36df0e

Signed-off-by: Kevin Earls <[email protected]>

Merge branch 'cronjob-migration' of github.com:kevinearls/jaeger-oper…

30b3a7c

…ator into cronjob-migration

Reinstate 1.23 tests

c8ceea0

Signed-off-by: Kevin Earls <[email protected]>

Merge branch 'main' into cronjob-migration

7f3b7ad

Added test for autoDetectCronJobsVersion, cleanup

18dfc7f

Signed-off-by: Kevin Earls <[email protected]>

Make lint happy

88c4a21

Signed-off-by: Kevin Earls <[email protected]>

kevinearls marked this pull request as ready for review April 26, 2022 12:26

frzifus reviewed Apr 26, 2022

View reviewed changes

Cleanup following Bene's comments

227d6fe

Signed-off-by: Kevin Earls <[email protected]>

pavolloffay reviewed Apr 26, 2022

View reviewed changes

kevinearls added 2 commits April 27, 2022 16:27

Rename constants, add 1 new constant

67f70c2

Signed-off-by: Kevin Earls <[email protected]>

Merge branch 'main' into cronjob-migration

95a5405

frzifus approved these changes Apr 29, 2022

View reviewed changes

Merge branch 'main' into cronjob-migration

0a6f84d

rubenvp8510 approved these changes May 3, 2022

View reviewed changes

rubenvp8510 merged commit 32e6ca7 into jaegertracing:main May 3, 2022

kevinearls deleted the cronjob-migration branch May 3, 2022 15:16

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

Cronjob migration #1856

Cronjob migration #1856

kevinearls commented Apr 13, 2022

codecov bot commented Apr 13, 2022 •

edited

Loading

kevinearls commented Apr 26, 2022

frzifus left a comment

frzifus Apr 26, 2022

kevinearls Apr 26, 2022

frzifus Apr 27, 2022 •

edited

Loading

kevinearls Apr 28, 2022

frzifus Apr 29, 2022

frzifus Apr 26, 2022

kevinearls Apr 26, 2022

frzifus Apr 26, 2022

pavolloffay Apr 26, 2022

kevinearls Apr 27, 2022

pavolloffay Apr 27, 2022

kevinearls commented Apr 28, 2022

frzifus left a comment

kevinearls commented May 3, 2022

Cronjob migration #1856

Cronjob migration #1856

Conversation

kevinearls commented Apr 13, 2022

Which problem is this PR solving?

Short description of the changes

codecov bot commented Apr 13, 2022 • edited Loading

Codecov Report

kevinearls commented Apr 26, 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

frzifus Apr 27, 2022 • edited Loading

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

kevinearls commented Apr 28, 2022

frzifus left a comment

Choose a reason for hiding this comment

kevinearls commented May 3, 2022

codecov bot commented Apr 13, 2022 •

edited

Loading

frzifus Apr 27, 2022 •

edited

Loading