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

[fix][fn] Make KubernetesRuntime translate characters in function tenant, namespace, and name during function removal to avoid label errors #19584

Conversation

csthomas1
Copy link
Contributor

@csthomas1 csthomas1 commented Feb 21, 2023

Make KubernetesRuntime translate tenant, namespace, and function name characters that are not allowed within Kubernetes labels as part of function removal.

Fixes #19583

Motivation

See linked issue ticket.

Modifications

Updated KubernetesRuntime.deleteStatefulSet() to pass function tenant, namespace, and name values through the pre-existing getLabels() function to replace disallowed characters.

Verifying this change

  • Make sure that the change passes the CI checks.

Adds the following test cases:

  • KubernetesRuntimeTest:
    • testDeleteStatefulSetWithTranslatedKubernetesLabelChars(): Verifies that characters not supported by Kubernetes naming requirements are translated to acceptable forms

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: (csthomas1#1)

(All tests passed -- the Flaky tests failure occurs after execution of the tests themselves and appears to be due to a failed CodeCov upload attempt)

…d function name characters that are not allowed within Kubernetes labels as part of function removal
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 21, 2023
@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution!

@lhotari
Copy link
Member

lhotari commented Apr 5, 2023

Closing and reopening to get a new build with a fresh merge commit for master branch in the build.

@lhotari lhotari closed this Apr 5, 2023
@lhotari lhotari reopened this Apr 5, 2023
@lhotari lhotari added this to the 3.0.0 milestone Apr 5, 2023
@lhotari lhotari requested a review from cbornet April 5, 2023 06:36
Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

good catch! would you mind to add a test case?

@github-actions github-actions bot removed the Stale label Apr 6, 2023
@csthomas1
Copy link
Contributor Author

csthomas1 commented Apr 6, 2023 via email

@csthomas1 csthomas1 force-pushed the issue#19583/fix-function-labels-invalid-chars-on-delete branch from b9202f9 to 8084c8f Compare April 7, 2023 19:05
@poorbarcode poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023
@lhotari
Copy link
Member

lhotari commented May 10, 2023

Closing and re-opening to trigger a new CI build with changes from master branch.

@lhotari lhotari closed this May 10, 2023
@lhotari lhotari reopened this May 10, 2023
@lhotari
Copy link
Member

lhotari commented May 10, 2023

@csthomas1 would you mind merging changes from master (or rebasing the changes) and fixing the issues with current master?

Kubernetes client version was upgraded from 12.0.1 version to 18.0.0 version in 93fb4f8 . There are minor API changes which need to be addressed.

Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:testCompile (default-testCompile) on project pulsar-functions-runtime: Compilation failure
Error:  /home/runner/work/pulsar/pulsar/pulsar-functions/runtime/src/test/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntimeTest.java:[1313,21] method readNamespacedStatefulSet in class io.kubernetes.client.openapi.apis.AppsV1Api cannot be applied to given types;
Error:    required: java.lang.String,java.lang.String,java.lang.String
Error:    found:    java.lang.String,java.lang.String,java.lang.Object,java.lang.Object,java.lang.Object
Error:    reason: actual and formal argument lists differ in length
Error:  -> [Help 1]

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #19584 (83c5752) into master (51c2bb4) will increase coverage by 39.52%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #19584       +/-   ##
=============================================
+ Coverage     33.50%   73.02%   +39.52%     
- Complexity    12053    31941    +19888     
=============================================
  Files          1613     1867      +254     
  Lines        126120   138591    +12471     
  Branches      13749    15222     +1473     
=============================================
+ Hits          42254   101210    +58956     
+ Misses        78332    29345    -48987     
- Partials       5534     8036     +2502     
Flag Coverage Δ
inttests 24.25% <0.00%> (+0.08%) ⬆️
systests 25.09% <0.00%> (?)
unittests 72.27% <100.00%> (+40.35%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...unctions/runtime/kubernetes/KubernetesRuntime.java 49.16% <100.00%> (+49.16%) ⬆️

... and 1521 files with indirect coverage changes

@tisonkun
Copy link
Member

Merging...

Thank you!

@tisonkun tisonkun merged commit 9340d6e into apache:master Jun 13, 2023
43 checks passed
Technoboy- pushed a commit that referenced this pull request Jun 14, 2023
…ant, namespace, and name during function removal to avoid label errors (#19584)

Co-authored-by: tison <wander4096@gmail.com>
Technoboy- pushed a commit that referenced this pull request Jun 14, 2023
…ant, namespace, and name during function removal to avoid label errors (#19584)

Co-authored-by: tison <wander4096@gmail.com>
liangyepianzhou pushed a commit that referenced this pull request Jul 7, 2023
…ant, namespace, and name during function removal to avoid label errors (#19584)

Co-authored-by: tison <wander4096@gmail.com>
(cherry picked from commit 9340d6e)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jul 17, 2023
…ant, namespace, and name during function removal to avoid label errors (apache#19584)

Co-authored-by: tison <wander4096@gmail.com>
(cherry picked from commit 9340d6e)
(cherry picked from commit 3a3c6f3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants