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

control-service: Cronjob API backwards compatibility #1580

Merged
merged 15 commits into from
Feb 22, 2023

Conversation

doks5
Copy link
Contributor

@doks5 doks5 commented Feb 1, 2023

Currently there is no backwards compatibility support when a user switches to the V1 Kubernetes Cronjob API (from the currently default V1beta1). This results in situations, where if there are deployed data jobs which use the v1beta1 API, and a switch is made to the V1 API, these jobs are suddenly shown as NOT DEPLOYED, and cannot be properly managed.

This change adds backwards compatibility support in the Kubernetes Service, to allow for a switch to V1 Cronjob API in clusters, where V1beta1 API cronjobs are deployed.

Testing Done: Unit and Integration tests (new and existing).

Signed-off-by: Andon Andonov andonova@vmware.com

@murphp15
Copy link
Collaborator

murphp15 commented Feb 1, 2023

Nice spot! I think it looks really good. However I think it would be soo much better if you are able to test it with an integration test using actual jobs.

Copy link
Collaborator

@antoniivanov antoniivanov left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this.

is there a Github issue created for this? Can you linked if yes.
If not please create and link it.

I agree about the integration tests. I think we will sleep lighter if we have a something close to end to end test that verifies the use-case of jobs in both new and old versions work.

@murphp15
Copy link
Collaborator

murphp15 commented Feb 2, 2023

@doks5 I actually think even if beta1 is enabled we should check for v1 jobs.
Because if we deploy a few jobs on v1 and need to roll back to betav1 for some unexpected reason then the issue will introduce its self again.
Plus it should make the code a bit simpler?

@doks5 doks5 force-pushed the person/andonova/backwards-compatibility branch 2 times, most recently from 387d360 to 0aaa6aa Compare February 3, 2023 14:57
@doks5 doks5 self-assigned this Feb 6, 2023
@doks5
Copy link
Contributor Author

doks5 commented Feb 6, 2023

Associated Github Issue: #1595

@doks5
Copy link
Contributor Author

doks5 commented Feb 6, 2023

Hi, @tozka and @murphp15

I agree that an integration test is necessary for this change, and I have been trying to come up with something for the past few days. However, I don't see a way how to switch the datajobs.control.k8s.k8sSupportsV1CronJob flag in the middle of a test. The flag can be switched at the beginning of the test, but that means all jobs would be deployed with the V1 API.
Do you have any ideas or suggestions how such a test could be implemented?

@murphp15
Copy link
Collaborator

murphp15 commented Feb 6, 2023

@doks5
What I would do is create a new class called LegacyDataJobKubernetesService.
It would extend DataJobKubernetesService and it would always set k8sSupportsV1CronJob to false.
in the test setup I would have k8sSupportsV1CronJob set to true.

Now you have one bean that you can use for legacy inserts/update/etc and one for new inserts updates etc..

Please let me know if that isn't clear as I am happy to help.

@doks5
Copy link
Contributor Author

doks5 commented Feb 7, 2023

This change is blocked by #1612

@doks5 doks5 force-pushed the person/andonova/backwards-compatibility branch 4 times, most recently from be9f0f0 to 4039ca6 Compare February 16, 2023 15:58
@doks5 doks5 force-pushed the person/andonova/backwards-compatibility branch from affbc4f to 7283ff0 Compare February 20, 2023 08:08
@doks5
Copy link
Contributor Author

doks5 commented Feb 20, 2023

@tozka, Can you take a look? I believe to have fixed all outstanding issues.

Copy link
Collaborator

@antoniivanov antoniivanov left a comment

Choose a reason for hiding this comment

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

If you have done any manual testing please include details on what you've exactly done.

Include comment in the Git description on why we decided not to make IT tests (you can include a link to the ticket where we discussed this (if it was in a ticket).

Looks good to me. Thanks for the change.

@doks5 doks5 force-pushed the person/andonova/backwards-compatibility branch from 3d3a7e1 to 4cff7ee Compare February 22, 2023 07:56
doks5 and others added 2 commits February 22, 2023 11:45
Currently there is no backwards compatibility support when a user
switches to the V1 Kubernetes Cronjob API (from the currently default
V1beta1). This results in situations, where if there are deployed data
jobs which use th v1beta1 API, and a switch is made to the V1 API, these
jobs are suddenly shown as `NOT DEPLOYED`, and cannot be properly managed.

This change adds backwards compatibility support in the Kubernetes Service,
to allow for a switch to V1 Cronjob API in clusters, where V1beta1 API cronjobs
are deployed.

Testing Done: Unit and Integration tests (new and existing).

Signed-off-by: Andon Andonov <andonova@vmware.com>
doks5 and others added 13 commits February 22, 2023 11:45
Signed-off-by: Andon Andonov <andonova@vmware.com>
Signed-off-by: Andon Andonov <andonova@vmware.com>
Signed-off-by: Andon Andonov <andonova@vmware.com>
Signed-off-by: Andon Andonov <andonova@vmware.com>
Signed-off-by: Andon Andonov <andonova@vmware.com>
Signed-off-by: Andon Andonov <andonova@vmware.com>
Signed-off-by: Andon Andonov <andonova@vmware.com>
@doks5 doks5 force-pushed the person/andonova/backwards-compatibility branch from 39c3dcb to ab82088 Compare February 22, 2023 09:46
@doks5 doks5 merged commit e538bc7 into main Feb 22, 2023
@doks5 doks5 deleted the person/andonova/backwards-compatibility branch February 22, 2023 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants