8000 K8s Java client upgrade by utk-12 · Pull Request #3075 · azkaban/azkaban · GitHub
[go: up one dir, main page]
Skip to content

K8s Java client upgrade #3075

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

Jump to bottom
Merged
merged 7 commits into from
Apr 25, 2022
Merged

K8s Java client upgrade #3075

merged 7 commits into from
Apr 25, 2022

Conversation

utk-12
Copy link
Collaborator
@utk-12 utk-12 commented Apr 13, 2022 •
edited
Loading

Upgrading K8s Java client:

Why this change was required?
AKS and On-Prem K8s cluster upgrades are imminent and we will have breaking API compatibility as mentioned here,
https://github.com/kubernetes-client/java/wiki/2.-Versioning-and-Compatibility

Issues that triggered this investigation
K8s watch events were not being received after a few days on AKS.
After looking through the following issues kubernetes-client/java#1370 we realized our client version might become incompatible soon.

We still plan to keep the call/read/connect timeouts to the watch server.

High-level Changes :

  1. We now use offsetDateTime type for getting pod creation timestamps instead of getting the timestamp using System.currentTimeMillis()
  2. We now use Enums for RestartPolicy

Gotchas::
We are now using the GenericKubernetesApi instead of CoreV1APi for service deletion due to :
https://github.com/kubernetes-client/java/wiki/6.-Known-Issues

@codecov
Copy link
codecov bot commented Apr 13, 2022 •
edited
Loading

Codecov Report

Merging #3075 (d89fe8d) into master (38fe7ba) will increase coverage by 0.01%.
The diff coverage is 61.53%.

❗ Current head d89fe8d differs from pull request most recent head a401ce8. Consider uploading reports for the commit a401ce8 to get more accurate results

@@             Coverage Diff              @@
##             master    #3075      +/-   ##
============================================
+ Coverage     41.58%   41.59%   +0.01%     
+ Complexity     4658     4657       -1     
============================================
  Files           596      596              
  Lines         40094    40117      +23     
  Branches       4662     4665       +3     
============================================
+ Hits          16672    16687      +15     
- Misses        22038    22044       +6     
- Partials       1384     1386       +2
Impacted Files Coverage Δ
az-core/src/main/java/azkaban/Constants.java 21.42% <ø> (ø)
...an/executor/container/ContainerCleanupManager.java 43.92% <ø> (-1.71%) ⬇️
...xecutor/container/KubernetesContainerizedImpl.java 56.61% <20.00%> (-0.36%) ⬇️
...aban/executor/container/watch/KubernetesWatch.java 65.78% <83.33%> (+4.17%) ⬆️
...an/container/models/AzKubernetesV1SpecBuilder.java 90.65% <100.00%> (ø)
...executor/container/watch/AzPodStatusExtractor.java 79.10% <100.00%> (ø)
...java/azkaban/container/models/ImagePullPolicy.java 0.00% <0.00%> (-100.00%) ⬇️
...n-common/src/main/java/azkaban/user/UserUtils.java 64.51% <0.00%> (-3.23%) ⬇️
...n/java/azkaban/jobExecutor/AbstractProcessJob.java 48.45% <0.00%> (-2.07%) ⬇️
... and 4 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 38fe7ba...a401ce8. Read the comment docs.

@coveralls
Copy link
coveralls commented Apr 13, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 6811

  • 24 of 39 (61.54%) changed or added relevant lines in 4 files are covered.
  • 111 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.02%) to 45.273%
Changes Missing Coverage Covered Lines Changed/Added Lines %
azkaban-common/src/main/java/azkaban/executor/container/watch/KubernetesWatch.java 15 18 83.33%
azkaban-common/src/main/java/azkaban/executor/container/KubernetesContainerizedImpl.java 3 15 20.0%
Files with Coverage Reduction New Missed Lines %
azkaban-common/src/main/java/azkaban/user/UserUtils.java 1 74.19%
azkaban-common/src/main/java/azkaban/jobExecutor/AbstractProcessJob.java 2 52.58%
azkaban-common/src/main/java/azkaban/container/models/ImagePullPolicy.java 6 0%
azkaban-common/src/main/java/azkaban/executor/container/ContainerCleanupManager.java 31 47.66%
azkaban-exec-server/src/main/java/azkaban/execapp/FlowRunnerManager.java 71 17.72%
Totals Coverage Status
Change from base Build 6790: 0.02%
Covered Lines: 18057
Relevant Lines: 39885
💛 - Coveralls

svnarumugam
svnarumugam reviewed Apr 13, 2022
View reviewed changes
8000
Copy link
Contributor
@svnarumugam svnarumugam 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 making this change Utkarsh!

  1. Could you please add more context as we discussed offline?
  2. Can we test the functional use cases manually, and call out the use cases tested?
  3. Please call out the contract changes like RestartPolicyEnum, and the need for OffsetDateTime to inform the readers of this PR.

djaiswal83
djaiswal83 requested changes Apr 22, 2022
View reviewed changes
Copy link
Member
@djaiswal83 djaiswal83 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 working on the client upgrade. Overall the code looks good. I have left a few comments. Please address those.

sshardool
sshardool reviewed Apr 22, 2022
View reviewed changes
Copy link
Collaborator
@sshardool sshardool left a commen 8000 t

Choose a reason for hiding this comment

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

Thanks a lot for taking this up!

djaiswal83
djaiswal83 approved these changes Apr 25, 2022
View reviewed changes
Copy link
Member
@djaiswal83 djaiswal83 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.

sshardool
sshardool approved these changes Apr 25, 2022
View reviewed changes
Copy link
Collaborator
@sshardool sshardool 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 taking care of these. Due to the nature of the changes, please give them extra soak-time on test/dev setups.

jakhani
jakhani approved these changes Apr 25, 2022
View reviewed changes
Copy link
Contributor
@jakhani jakhani left a comment

Choose a reason for hiding this comment

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

Thank you for all the hard work to fix this issue.

@utk-12 utk-12 merged commit 3233a2d into azkaban:master Apr 25, 2022
NancyXie2022 pushed a commit that referenced this pull request May 26, 2023
@utk-12
K8s Java client upgrade (#3075)
88f7575 
* K8s Java client upgrade

* Removed unused imports

* Marked watch in listNamespacedPodCall back to true

* Changed  label value in listNamespacedPod back to original label value / fixed indentation

* Using GenericKubernetesApi for service deletion and adding config values for K8s watch timeout

* Removing extra log lines used for testing

* Addressing all comments on PR

(cherry-picked from commit 3233a2d)
PR Link : #3075

RB=3331598
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svnarumugam svnarumugam svnarumugam left review comments

@djaiswal83 djaiswal83 djaiswal83 approved these changes

@jakhani jakhani jakhani approved these changes

@sshardool sshardool sshardool approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@utk-12 @coveralls @djaiswal83 @jakhani @svnarumugam @sshardool
0