USHIFT-1085: feat: initial implementation of storage migration by eggfoobar · Pull Request #1956 · openshift/microshift

eggfoobar · 2023-06-23T17:44:32Z

Initial implementation for storage migrator

This currently only contains the code for the migrator itself. I tested a few ways of running this for performance, we're looking at around 2.8 seconds for a migration to run.

The calling code isn't present, I'll open up a new PR for that logic.

/assign @pmtk

openshift-ci-robot · 2023-06-23T17:44:37Z

@eggfoobar: This pull request references USHIFT-1085 which is a valid jira issue.

Details

In response to this:

Initial implementation for storage migrator

This currently only contains the code for the migrator itself. I tested a few ways of running this for performance, we're looking at around 2.8 seconds for a migration to run.

The calling code isn't present, I'll open up a new PR for that logic.

/assign @pmtk

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/test-infra repository.

openshift-ci · 2023-06-23T17:47:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eggfoobar

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [eggfoobar]

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

pmtk · 2023-06-26T10:42:13Z

What do you think about changing the order: first merging "partial start" (etcd+KAS) in places where migration would happen, and then adding code from this PR so migrator could already be executed?

pmtk · 2023-06-26T10:51:17Z

I'm wondering if, at this point, we shouldn't just call off the whole thing. We care for "all or nothing".
Do you foresee another component that would handle failure in results?

The failure state is something we need to think in terms of our resources and the customers, typically if it fails it means theres a compatibility error with the resource versions, we should catch that in our testing for our CRs, but if the user has applied different CRDs that fail migration, they should be notified to fix them manually. I was thinking of this function call giving a migrator failure (i.e. can't reach server can't list resources) and a resource migration error, that would be recoverable.

Data errors are recoverable, and it may make sense to collect all of them unless it makes the logic significantly more complicated.

pmtk · 2023-06-26T11:41:55Z

Not sure if we need that. Is there a way to log that some resource was migration from version A to B? We could log that in "real time", not packing all results into a collection and then iterating to log the items

Sure thing, we can do that. The primary reason it's here is just to give us raw access to write the information in any format we want if we need to store the results somewhere easily.

Is the pre-run phase logging to the standard MicroShift log, or do we see these errors in the greenboot health check log? It would be nice if the greenboot log could at least report "there was a data migration error, check the MicroShift logs for details".

It's currently just using klog, I'm not sure how we send logs to greenboot but I love the idea. We should make it do that

The klog output is going to go to stdout/stderr. I guess that's going to the systemd unit where pre-run is running, rather than a greenboot-specific log. Can we put these messages (and any others) somewhere for our health-check script to collect and report?

Sure, I added some files here to use in greenboot, https://github.com/openshift/microshift/pull/1956/files#diff-e78e17834c76663b756393d04ebe7aceb8ea4059d44b7d2ae3ee4e9999aead6eR23-R24

dhellmann · 2023-06-26T19:28:56Z

Data errors are recoverable, and it may make sense to collect all of them unless it makes the logic significantly more complicated.

dhellmann · 2023-06-26T19:37:42Z

This seems like a good opportunity to use a channel to communicate between the goroutines, instead managing a lock.

dhellmann · 2023-06-26T19:43:38Z

Is the pre-run phase logging to the standard MicroShift log, or do we see these errors in the greenboot health check log? It would be nice if the greenboot log could at least report "there was a data migration error, check the MicroShift logs for details".

Signed-off-by: ehila <ehila@redhat.com>

removing for now to remove complexity, no clear performance gain is present should revisit if performance issues crop up Signed-off-by: ehila <ehila@redhat.com>

Signed-off-by: ehila <ehila@redhat.com>

updated gather logic to be more inline with trigger controller in kube storage migrator Signed-off-by: ehila <ehila@redhat.com>

put in helper logic to write log and status to backup folder for uvalidation Signed-off-by: ehila <ehila@redhat.com>

Signed-off-by: ehila <ehila@redhat.com>

openshift-ci · 2023-07-06T18:03:04Z

@eggfoobar: 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/microshift-e2e-arm	`09a2556`	link	false	`/test microshift-e2e-arm`
ci/prow/e2e-openshift-conformance-reduced-arm	`09a2556`	link	false	`/test e2e-openshift-conformance-reduced-arm`

Full PR test history. Your PR dashboard.

Details

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/test-infra repository. I understand the commands that are listed here.

dhellmann · 2023-06-29T19:21:57Z

Naming the variable the same as the package caught my eye here. If there's another reason to update the PR, you could consider renaming the variable to something like dynamicClient.

dhellmann · 2023-06-29T19:22:47Z

What's the extra level of indirection for here?

eggfoobar · 2023-07-08T21:21:30Z

/close

Closing PR in favor of other approaches. In this PR we were trying to implement this feature with out the cluster being fully up, upon further investigation, we will need the cluster up in order to fully support CRDs and their migration webhooks.

openshift-ci · 2023-07-08T21:21:41Z

@eggfoobar: Closed this PR.

Details

In response to this:

/close

Closing PR in favor of other approaches. In this PR we were trying to implement this feature with out the cluster being fully up, upon further investigation, we will need the cluster up in order to fully support CRDs and their migration webhooks.

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/test-infra repository.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 23, 2023

openshift-ci Bot assigned pmtk Jun 23, 2023

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 23, 2023

openshift-ci Bot requested review from jerpeter1 and pacevedom June 23, 2023 17:47

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2023

eggfoobar force-pushed the storage-migration branch 2 times, most recently from b556240 to c23955d Compare June 23, 2023 20:48

pmtk reviewed Jun 26, 2023

View reviewed changes

dhellmann reviewed Jun 26, 2023

View reviewed changes

eggfoobar force-pushed the storage-migration branch 2 times, most recently from 1ccbbd9 to e55e400 Compare June 29, 2023 14:48

eggfoobar mentioned this pull request Jun 30, 2023

USHIFT-1085: feat: support prerun with minimal services #1978

Merged

eggfoobar added 7 commits July 6, 2023 09:56

feat: initial implementation of storage migration

3c6a6b4

Signed-off-by: ehila <ehila@redhat.com>

upkeep: updated to include suggested changes

fe450aa

Signed-off-by: ehila <ehila@redhat.com>

upkeep: remove concurrency

aa46030

removing for now to remove complexity, no clear performance gain is present should revisit if performance issues crop up Signed-off-by: ehila <ehila@redhat.com>

feat: add continue token check for list

afd6c80

Signed-off-by: ehila <ehila@redhat.com>

fix: golangci linter errors

0886da1

Signed-off-by: ehila <ehila@redhat.com>

feat: change to gather logic for resources

326d890

updated gather logic to be more inline with trigger controller in kube storage migrator Signed-off-by: ehila <ehila@redhat.com>

feat: added call to migration in prerun and output files

f999ea3

put in helper logic to write log and status to backup folder for uvalidation Signed-off-by: ehila <ehila@redhat.com>

eggfoobar force-pushed the storage-migration branch from e55e400 to f999ea3 Compare July 6, 2023 13:57

eggfoobar changed the title ~~[WIP] USHIFT-1085: feat: initial implementation of storage migration~~ USHIFT-1085: feat: initial implementation of storage migration Jul 6, 2023

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 6, 2023

fix: linter errors

09a2556

Signed-off-by: ehila <ehila@redhat.com>

dhellmann reviewed Jul 6, 2023

View reviewed changes

eggfoobar mentioned this pull request Jul 8, 2023

USHIFT-1085: Storage Migration Controller Implementation #2019

Merged

openshift-ci Bot closed this Jul 8, 2023

This was referenced May 5, 2026

NO-ISSUE: rebase-release-5.0-5.0.0-0.nightly-2026-05-04-160406_amd64-2026-05-04_arm64-2026-05-04 #6623

Closed

NO-ISSUE: rebase-release-5.0-5.0.0-0.nightly-2026-05-04-160406_amd64-2026-05-04_arm64-2026-05-05 #6630

Merged

Conversation

eggfoobar commented Jun 23, 2023

Uh oh!

openshift-ci-robot commented Jun 23, 2023 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Jun 23, 2023

Uh oh!

pmtk commented Jun 26, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Jul 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eggfoobar commented Jul 8, 2023

Uh oh!

openshift-ci Bot commented Jul 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

openshift-ci-robot commented Jun 23, 2023 •

edited by openshift-ci Bot

Loading