Skip to content

[fix][fn] Fix k8s merge runtime opts bug#19481

Merged
michaeljmarshall merged 1 commit intoapache:masterfrom
michaeljmarshall:fix-k8s-runtime-options-merge
Feb 10, 2023
Merged

[fix][fn] Fix k8s merge runtime opts bug#19481
michaeljmarshall merged 1 commit intoapache:masterfrom
michaeljmarshall:fix-k8s-runtime-options-merge

Conversation

@michaeljmarshall
Copy link
Copy Markdown
Member

Fixes: #19478

Motivation

See issue for additional context. Essentially, we are doing a shallow clone when we needed a deep clone. The consequence is leaked labels, annotations, and tolerations.

Modifications

  • Add a deepClone method to the BasicKubernetesManifestCustomizer.RuntimeOpts method. Note that this method is not technically a deep clone for the k8s objects. However, based on the way we "merge" these objects, it is sufficient to copy references to the objects.

Verifying this change

Added a test that fails before the change and passes afterwards.

Documentation

  • doc-not-needed

This is an internal bug fix. No docs needed.

Matching PR in forked repository

PR in forked repository: michaeljmarshall#27

Copy link
Copy Markdown
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.

Good catch

@lhotari
Copy link
Copy Markdown
Member

lhotari commented Feb 10, 2023

/pulsarbot rerun-failure-checks

@michaeljmarshall michaeljmarshall changed the title [fix][function] Fix k8s merge runtime opts bug [fix][fn] Fix k8s merge runtime opts bug Feb 10, 2023
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 10, 2023

Codecov Report

❌ Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 63.89%. Comparing base (016e7f0) to head (4c39986).
⚠️ Report is 2858 commits behind head on master.

Files with missing lines Patch % Lines
.../kubernetes/BasicKubernetesManifestCustomizer.java 87.50% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #19481       +/-   ##
=============================================
+ Coverage     24.75%   63.89%   +39.13%     
- Complexity      288    26098    +25810     
=============================================
  Files          1579     1832      +253     
  Lines        121841   134149    +12308     
  Branches      13304    14760     +1456     
=============================================
+ Hits          30164    85710    +55546     
+ Misses        87240    40624    -46616     
- Partials       4437     7815     +3378     
Flag Coverage Δ
inttests 24.87% <0.00%> (+0.12%) ⬆️
systests 25.40% <0.00%> (?)
unittests 61.16% <87.50%> (?)

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

Files with missing lines Coverage Δ
.../kubernetes/BasicKubernetesManifestCustomizer.java 80.99% <87.50%> (+80.99%) ⬆️

... and 1410 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@michaeljmarshall michaeljmarshall merged commit 0205148 into apache:master Feb 10, 2023
michaeljmarshall added a commit that referenced this pull request Feb 10, 2023
Fixes: #19478

### Motivation

See issue for additional context. Essentially, we are doing a shallow clone when we needed a deep clone. The consequence is leaked labels, annotations, and tolerations.

### Modifications

* Add a `deepClone` method to the `BasicKubernetesManifestCustomizer.RuntimeOpts` method. Note that this method is not technically a deep clone for the k8s objects. However, based on the way we "merge" these objects, it is sufficient to copy references to the objects.

### Verifying this change

Added a test that fails before the change and passes afterwards.

### Documentation

- [x] `doc-not-needed`

This is an internal bug fix. No docs needed.

### Matching PR in forked repository

PR in forked repository: michaeljmarshall#27

(cherry picked from commit 0205148)
michaeljmarshall added a commit that referenced this pull request Feb 10, 2023
Fixes: #19478

### Motivation

See issue for additional context. Essentially, we are doing a shallow clone when we needed a deep clone. The consequence is leaked labels, annotations, and tolerations.

### Modifications

* Add a `deepClone` method to the `BasicKubernetesManifestCustomizer.RuntimeOpts` method. Note that this method is not technically a deep clone for the k8s objects. However, based on the way we "merge" these objects, it is sufficient to copy references to the objects.

### Verifying this change

Added a test that fails before the change and passes afterwards.

### Documentation

- [x] `doc-not-needed`

This is an internal bug fix. No docs needed.

### Matching PR in forked repository

PR in forked repository: michaeljmarshall#27

(cherry picked from commit 0205148)
michaeljmarshall added a commit that referenced this pull request Feb 10, 2023
Fixes: #19478

### Motivation

See issue for additional context. Essentially, we are doing a shallow clone when we needed a deep clone. The consequence is leaked labels, annotations, and tolerations.

### Modifications

* Add a `deepClone` method to the `BasicKubernetesManifestCustomizer.RuntimeOpts` method. Note that this method is not technically a deep clone for the k8s objects. However, based on the way we "merge" these objects, it is sufficient to copy references to the objects.

### Verifying this change

Added a test that fails before the change and passes afterwards.

### Documentation

- [x] `doc-not-needed`

This is an internal bug fix. No docs needed.

### Matching PR in forked repository

PR in forked repository: michaeljmarshall#27

(cherry picked from commit 0205148)
michaeljmarshall added a commit that referenced this pull request Feb 10, 2023
Fixes: #19478

### Motivation

See issue for additional context. Essentially, we are doing a shallow clone when we needed a deep clone. The consequence is leaked labels, annotations, and tolerations.

### Modifications

* Add a `deepClone` method to the `BasicKubernetesManifestCustomizer.RuntimeOpts` method. Note that this method is not technically a deep clone for the k8s objects. However, based on the way we "merge" these objects, it is sufficient to copy references to the objects.

### Verifying this change

Added a test that fails before the change and passes afterwards.

### Documentation

- [x] `doc-not-needed`

This is an internal bug fix. No docs needed.

### Matching PR in forked repository

PR in forked repository: michaeljmarshall#27

(cherry picked from commit 0205148)
michaeljmarshall added a commit to datastax/pulsar that referenced this pull request Feb 13, 2023
Fixes: apache#19478

### Motivation

See issue for additional context. Essentially, we are doing a shallow clone when we needed a deep clone. The consequence is leaked labels, annotations, and tolerations.

### Modifications

* Add a `deepClone` method to the `BasicKubernetesManifestCustomizer.RuntimeOpts` method. Note that this method is not technically a deep clone for the k8s objects. However, based on the way we "merge" these objects, it is sufficient to copy references to the objects.

### Verifying this change

Added a test that fails before the change and passes afterwards.

### Documentation

- [x] `doc-not-needed`

This is an internal bug fix. No docs needed.

### Matching PR in forked repository

PR in forked repository: michaeljmarshall#27

(cherry picked from commit 0205148)
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.

[Bug] Unexpected labels on function in K8s runtime

4 participants