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.

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

Fix a bug with user-defined labels in priorityClass #3085

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Fix a bug with user-defined labels in priorityClass #3085

merged 1 commit into from
Sep 3, 2024

Conversation

nunnatsa
Copy link
Collaborator

@nunnatsa nunnatsa commented Sep 1, 2024

HCO already supports user-defined labels in priorityClass, but if another change is done to the priorityClass, either a change in a required label, or a change in the spec fields, the user-defined labels are deleted. This is also happens if the other modofication is done in different request.

This PR fixes this issue, and makes sure that user-defined labels are stay in place on update of the priorityClass.

What this PR does / why we need it:

Reviewer Checklist

Reviewers are supposed to review the PR for every aspect below one by one. To check an item means the PR is either "OK" or "Not Applicable" in terms of that item. All items are supposed to be checked before merging a PR.

  • PR Message
  • Commit Messages
  • How to test
  • Unit Tests
  • Functional Tests
  • User Documentation
  • Developer Documentation
  • Upgrade Scenario
  • Uninstallation Scenario
  • Backward Compatibility
  • Troubleshooting Friendly

Jira Ticket:

None

Release note:

Fix a bug with user-defined labels in priorityClass

@nunnatsa nunnatsa requested a review from tiraboschi September 1, 2024 14:47
@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Sep 1, 2024
@coveralls
Copy link
Collaborator

coveralls commented Sep 1, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 10679520351

Details

  • 49 of 53 (92.45%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 85.776%
Changes Missing Coverage Covered Lines Changed/Added Lines %
controllers/operands/kubevirt.go 24 28 85.71%
Totals Coverage Status
Change from base Build 10679428062: 0.02%
Covered Lines: 5337
Relevant Lines: 6222
💛 - Coveralls

@hco-bot
Copy link
Collaborator

hco-bot commented Sep 1, 2024

hco-e2e-kv-smoke-gcp lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-azure

@kubevirt-bot
Copy link
Contributor

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure

In response to this:

hco-e2e-kv-smoke-gcp lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@nunnatsa nunnatsa force-pushed the allow-custom-labels-pc branch from 49327c2 to bd8b47b Compare September 2, 2024 08:39
@kubevirt-bot kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL and removed size/L labels Sep 2, 2024
@nunnatsa nunnatsa force-pushed the allow-custom-labels-pc branch 2 times, most recently from 3665280 to e676ba6 Compare September 2, 2024 08:45
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2024
@nunnatsa nunnatsa force-pushed the allow-custom-labels-pc branch 2 times, most recently from 3f7d46f to 8504f77 Compare September 2, 2024 09:36
@nunnatsa nunnatsa force-pushed the allow-custom-labels-pc branch 2 times, most recently from 54965ec to 4935c9c Compare September 2, 2024 09:59
@hco-bot
Copy link
Collaborator

hco-bot commented Sep 2, 2024

hco-e2e-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-operator-sdk-sno-azure

@kubevirt-bot
Copy link
Contributor

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-sno-azure

In response to this:

hco-e2e-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-operator-sdk-sno-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@hco-bot
Copy link
Collaborator

hco-bot commented Sep 2, 2024

hco-e2e-upgrade-prev-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

@kubevirt-bot
Copy link
Contributor

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

In response to this:

hco-e2e-upgrade-prev-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@nunnatsa
Copy link
Collaborator Author

nunnatsa commented Sep 2, 2024

/retest

@hco-bot
Copy link
Collaborator

hco-bot commented Sep 2, 2024

hco-e2e-upgrade-prev-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

@kubevirt-bot
Copy link
Contributor

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

In response to this:

hco-e2e-upgrade-prev-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@hco-bot
Copy link
Collaborator

hco-bot commented Sep 2, 2024

hco-e2e-upgrade-prev-operator-sdk-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2024
@tiraboschi
Copy link
Member

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tiraboschi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2024
@hco-bot
Copy link
Collaborator

hco-bot commented Sep 3, 2024

hco-e2e-upgrade-operator-sdk-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-operator-sdk-azure

@kubevirt-bot
Copy link
Contributor

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-operator-sdk-azure

In response to this:

hco-e2e-upgrade-operator-sdk-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-operator-sdk-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@hco-bot
Copy link
Collaborator

hco-bot commented Sep 3, 2024

hco-e2e-consecutive-operator-sdk-upgrades-aws lane succeeded.
/override ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure

@kubevirt-bot
Copy link
Contributor

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure

In response to this:

hco-e2e-consecutive-operator-sdk-upgrades-aws lane succeeded.
/override ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@nunnatsa
Fix bug with user-defined labels in priorityClass
7da9ab0 
HCO already supports user-defined labels in priorityClass, but if
another change is done to the priorityClass, either a change in a
required label, or a change in the spec fields, the user-defined labels
are deleted. This is also happens if the other modofication is done in
different request.

This PR fixes this issue, and makes sure that user-defined labels are
stay in place on update of the priorityClass. As part of the fix, HCO
now does not remove and re-create the priorityClass, if only label were
changed, but updates it. As result, the update_priority_class was
removed as it is not relevant anymore.

Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>
@nunnatsa nunnatsa force-pushed the allow-custom-labels-pc branch from 400c0dd to 7da9ab0 Compare September 3, 2024 08:39
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2024
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Sep 3, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2024
@hco-bot
Copy link
Collaborator

hco-bot commented Sep 3, 2024

hco-e2e-upgrade-prev-operator-sdk-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure
hco-e2e-upgrade-prev-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

@kubevirt-bot
Copy link
Contributor

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure, ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

In response to this:

hco-e2e-upgrade-prev-operator-sdk-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure
hco-e2e-upgrade-prev-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@hco-bot
Copy link
Collaborator

hco-bot commented Sep 3, 2024

hco-e2e-upgrade-prev-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

@kubevirt-bot
Copy link
Contributor

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

In response to this:

hco-e2e-upgrade-prev-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@hco-bot
Copy link
Collaborator

hco-bot commented Sep 3, 2024

hco-e2e-consecutive-operator-sdk-upgrades-aws lane succeeded.
/override ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure

@kubevirt-bot
Copy link
Contributor

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure

In response to this:

hco-e2e-consecutive-operator-sdk-upgrades-aws lane succeeded.
/override ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@tiraboschi
Copy link
Member

/retest

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Sep 3, 2024 •
edited
Loading

@nunnatsa: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure 7da9ab0 link false /test hco-e2e-upgrade-prev-operator-sdk-sno-azure
ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure 7da9ab0 link true /test hco-e2e-consecutive-operator-sdk-upgrades-azure
ci/prow/hco-e2e-upgrade-operator-sdk-azure 7da9ab0 link true /test hco-e2e-upgrade-operator-sdk-azure
ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure 7da9ab0 link false /test hco-e2e-upgrade-operator-sdk-sno-azure

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@hco-bot
Copy link
Collaborator

hco-bot commented Sep 3, 2024

hco-e2e-upgrade-operator-sdk-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-operator-sdk-azure

@kubevirt-bot
Copy link
Contributor

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-operator-sdk-azure

In response to this:

hco-e2e-upgrade-operator-sdk-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-operator-sdk-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@hco-bot
Copy link
Collaborator

hco-bot commented Sep 3, 2024

hco-e2e-upgrade-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure

@kubevirt-bot
Copy link
Contributor

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure

In response to this:

hco-e2e-upgrade-operator-sdk-sno-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@kubevirt-bot kubevirt-bot merged commit 81eb8ba into kubevirt:main Sep 3, 2024
31 checks passed
@nunnatsa nunnatsa deleted the allow-custom-labels-pc branch September 3, 2024 14:21
@orenc1
Copy link
Collaborator

orenc1 commented Dec 26, 2024

/cherry-pick release-1.12

@kubevirt-bot
Copy link
Contributor

@orenc1: #3085 failed to apply on top of branch "release-1.12":

Applying: Fix bug with user-defined labels in priorityClass
Using index info to reconstruct a base tree...
M	controllers/operands/kubevirt.go
M	controllers/operands/kubevirt_test.go
M	deploy/cluster_role.yaml
A	deploy/index-image/community-kubevirt-hyperconverged/1.13.0/manifests/kubevirt-hyperconverged-operator.v1.13.0.clusterserviceversion.yaml
A	deploy/olm-catalog/community-kubevirt-hyperconverged/1.13.0/manifests/kubevirt-hyperconverged-operator.v1.13.0.clusterserviceversion.yaml
M	pkg/components/components.go
M	tests/func-tests/dependency_objects_test.go
M	tests/func-tests/update_priority_class_test.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): tests/func-tests/update_priority_class_test.go deleted in Fix bug with user-defined labels in priorityClass and modified in HEAD. Version HEAD of tests/func-tests/update_priority_class_test.go left in tree.
Auto-merging tests/func-tests/dependency_objects_test.go
CONFLICT (content): Merge conflict in tests/func-tests/dependency_objects_test.go
Auto-merging pkg/components/components.go
Auto-merging deploy/olm-catalog/community-kubevirt-hyperconverged/1.12.1/manifests/kubevirt-hyperconverged-operator.v1.12.1.clusterserviceversion.yaml
CONFLICT (content): Merge conflict in deploy/olm-catalog/community-kubevirt-hyperconverged/1.12.1/manifests/kubevirt-hyperconverged-operator.v1.12.1.clusterserviceversion.yaml
Auto-merging deploy/index-image/community-kubevirt-hyperconverged/1.12.1/manifests/kubevirt-hyperconverged-operator.v1.12.1.clusterserviceversion.yaml
Auto-merging deploy/cluster_role.yaml
Auto-merging controllers/operands/kubevirt_test.go
Auto-merging controllers/operands/kubevirt.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Fix bug with user-defined labels in priorityClass

In response to this:

/cherry-pick release-1.12

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

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

@tiraboschi tiraboschi tiraboschi approved these changes

@avlitman avlitman Awaiting requested review from avlitman

@machadovilaca machadovilaca Awaiting requested review from machadovilaca

Assignees

@tiraboschi tiraboschi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nunnatsa @coveralls @hco-bot @kubevirt-bot @tiraboschi @orenc1