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: using ownerRef to control the removal of storageversionmigrations. #342

Conversation

xuezhaojun
Copy link
Member

Summary

When create a StorageVersionMigration, using the cluster-manager as the ownerReference. When cluster-manager is removed, all Migrations will also be removed.

Related issue(s)

Previously, it uses a remove function that depending on:

From release to release the files list are changed, but during the upgrade the cluster-manager keep exisitng, so that old storageversionmigration CRs won't get removed ever.

Fixes #

@xuezhaojun
Copy link
Member Author

/assign @qiujian16
/assign @haoqing0110

Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Attention: 106 lines in your changes are missing coverage. Please review.

Comparison is base (f89d535) 61.75% compared to head (f8e911f) 61.52%.
Report is 9 commits behind head on main.

Files Patch % Lines
...ollers/migrationcontroller/migration_controller.go 56.09% 50 Missing and 4 partials ⚠️
pkg/registration/hub/lease/clocksynccontroller.go 42.85% 47 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #342      +/-   ##
==========================================
- Coverage   61.75%   61.52%   -0.24%     
==========================================
  Files         132      133       +1     
  Lines       13992    14189     +197     
==========================================
+ Hits         8641     8730      +89     
- Misses       4585     4691     +106     
- Partials      766      768       +2     
Flag Coverage Δ
unit 61.52% <50.46%> (-0.24%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xuezhaojun xuezhaojun closed this Jan 10, 2024
@xuezhaojun xuezhaojun reopened this Jan 10, 2024
@qiujian16
Copy link
Member

but the clustermanager will not be removed during upgrade, only uninstall will remove cluster manager. Is it what you are expecting?

@xuezhaojun
Copy link
Member Author

xuezhaojun commented Jan 10, 2024

but the clustermanager will not be removed during upgrade, only uninstall will remove cluster manager. Is it what you are expecting?

Yes, keep them won't harm anything. This PR just make sure when we uninstall clustermanager, everything we installed will be cleaned up.

Whether we want change to "removing migrations during the upgrade", I think it's worth further discuession combined with other migrations topics like CRDs of addons.

@xuezhaojun xuezhaojun closed this Jan 10, 2024
@xuezhaojun xuezhaojun reopened this Jan 10, 2024
@qiujian16
Copy link
Member

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 10, 2024
Copy link
Contributor

openshift-ci bot commented Jan 10, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, xuezhaojun

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

@openshift-merge-bot openshift-merge-bot bot merged commit 4b6e12a into open-cluster-management-io:main Jan 10, 2024
37 of 40 checks passed
@xuezhaojun xuezhaojun deleted the fix-storageversionmigration-clean-up branch January 10, 2024 04:03
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.

3 participants