-
Notifications
You must be signed in to change notification settings - Fork 483
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
USHIFT-2348: microshift y-2 upgrades #1562
USHIFT-2348: microshift y-2 upgrades #1562
Conversation
@dhellmann: This pull request references USHIFT-2348 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some discussion in concerns of the underlying os, e.g. RHEL versions. We currently have increased test efforts with 4.14 being supported on 9.2 and 9.3. What would be the impact on Y+2 upgrades? Allowing both Y+2 and RHEL+1 sounds like a lot of combinations we might need to test.
There is an enhancement for the RHEL+1 support. How does that relate to this one? Would it also need to be updated?
762ad26
to
45a9931
Compare
The latest draft addresses all of @DanielFroehlich's comments. |
45a9931
to
ee74a10
Compare
/assign @jogeo |
7a9b0bb
to
2d495da
Compare
#1555 is changing the enhancement template in a way that will cause the header check in the linter job to fail for existing PRs. If this PR is merged within the development period for 4.16 you may override the linter if the only failures are caused by issues with the headers (please make sure the markdown formatting is correct). If this PR is not merged before 4.16 development closes, please update the enhancement to conform to the new template. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small changes
/lgtm
@jogeo I committed your suggestions, which removed the lgtm you applied. Could you re-apply it, please? |
The linter is failing because of the template change referenced in #1562 (comment) |
schema, and content). | ||
|
||
MicroShift does not automatically create `StorageVersionMigration` CRs | ||
to trigger data migration. The core kubernetes APIs are safe because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is true right now, if we were to incorporate something that did require data migrations in the future (e.g. a monitoring stack transitioning from using elasticsearch for log storage to loki), we might need to carry the same version of the monitoring stack through a couple of microshift releases in order to ensure that data migrations take place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the migration stack? I don't think monitoring is involved in the storage migration, right? At least not the way we're using it.
5c78f32
to
dd9aca3
Compare
|
||
The main drawback to implementing this enhancement is the increased | ||
test matrix for upgrades. We can automate those tests to minimize the | ||
impact. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider minimizing the testing effort by declaring y-2
upgrade support only between EUS releases?
As an example, in 4.15 / 4.16 releases we would have to set up the following tests:
- 4.14 -> 4.16 (y-2 released)
- 4.15 -> 4.17 (y-2 fake)
- 4.15 -> 4.16 (y-1 released)
- 4.15 -> 4.17 (y-2 fake)
If we only support EUS-to-EUS y-2 upgrades, we'd need tests 1 and 3 from the above list.
Note that we do not have to do anything "special" to limit our support to EUS releases only in the y-2 context - it can be purely declarative. Potentially, we can add a simple check in the code to only allow even version upgrade for y-2 scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not need the fake test scenarios. Those use the same set of code and only ensure that the version number prevents an upgrade. We have unit tests for testing that restriction.
After we have a 4.N+2 release, we could go back to the 4.N branch and add a new test that ensures that no changes go into 4.N that would prevent an upgrade to 4.N+2. However, given the fact that we do not make architectural or file format changes in stable branches, the risk of introducing such a breaking change is very low and adding such a test would add complexity to our test matrix for little benefit. Any such issue would be caught by the periodic job on the 4.N+2 branch.
If at some point there is an issue with upgrading from the code in version 4.N to 4.N+2 caused by introducing a breaking change into 4.N, we must fix the problem in 4.N+2 because we cannot ensure that users update to a new 4.N.z before trying to update to 4.N+2.
and also moving from the EUS version to non-EUS version. The aspects | ||
of testing the OS support during upgrades are orthogonal to the work | ||
for this enhancement, however, and should not require additional | ||
expansion of the test matrix, either in CI or by QE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we only support EUS-to-EUS upgrades for MicroShift, it would be in sync with EUS-to-EUS upgrades of the OS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect in practice most users will stick to EUS versions and update 2 at a time. The risk of only actually testing EUS upgrades is that we have to remember to do different tests for every other version of MicroShift. If we always test that upgrading 2 versions at a time work, we will always maintain support for it.
1. RHEL 9.2 / 4.14.latest → RHEL 9.4 / 4.16.Z | ||
1. RHEL 9.3 / 4.14.latest → RHEL 9.4 / 4.16.Z | ||
1. RHEL 9.2 / 4.15.latest → RHEL 9.4 / 4.16.Z | ||
1. RHEL 9.3 / 4.15.latest → RHEL 9.4 / 4.16.Z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to add a test for 4.15 -> 4.17 (fake), unless we support EUS-to-EUS only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The QE team should not be testing using fake packages like what we build in CI.
(4.14 to 4.16 would be OK, but 4.15 to 4.17 would not). This would | ||
make the version checking logic more complicated and would introduce | ||
opportunities for that skip-level upgrade process to be broken in a | ||
non-EUS version so that it has to be fixed before the next EUS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might want to make a difference between supported and tested upgrade version skews.
If there is a concern for breaking features between non-EUS upgrades, we can still test those in CI, but not declare suppport for those.
looks good to me |
dd9aca3
to
f080d37
Compare
+1 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pacevedom 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 |
/lgtm |
@dhellmann: all tests passed! 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/test-infra repository. I understand the commands that are listed here. |
installed. | ||
2. Software runs, time passes. | ||
3. Edge device administrator updates the host to run MicroShift 4.Y. | ||
* For ostree-based systems, the host is automatically rebooted as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where would it roll back to if the upgrade fails (for some other reason)? Does ostree have any validation of version skew?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rollback is always to the previous image that was installed on the host, regardless of what the new image contains or what causes the rollback.
not make a distinction between types of versions and requires stepping | ||
through one release at a time. MicroShift upgrades are significantly | ||
simpler because they are all single-node (so disruption is expected) | ||
and there are no operators for managing the host or cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can add-ons that are used through OLM be a problem here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just as with OCP it is possible for an OLM-installed operator to be an issue by not having compatibility with the version skew of MicroShift. It's less of a concern for MicroShift, because there are fewer core APIs and less likelihood of that incompatibility, but it's still there.
1. Initial cluster bring-up will be a mix of deployments from ISO | ||
installer and rpm-ostree upgrades from a bare RHEL host | ||
1. The following upgrade paths will be covered: | ||
1. RHEL 9.2 / 4.14.latest → RHEL 9.4 / 4.16.Z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "4.14.latest" going to start with the 4.14. z-stream version when the 4.16.0 version is released? So can users update from a z-stream prior, down to 4.14.0? Or does the Y-version check in reality ignore the z-stream skew?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In CI we test by starting from the latest from a z-stream. That ensures there is at least some path to update, and since it is extremely unlikely that we would introduce a change into the z-stream that would break updates the benefit of testing every combination of versions from the z-stream to the new release does not make the effort involved worth it.
@jogeo, I assume QE is taking a similar stance?
|
||
N/A | ||
|
||
### Tech Preview -> GA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the status of this feature in 4.16? TP or GA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love get get this GAed directly - as it makes sense for an EUS release to use directly on production systems. @dhellmann , any concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be GA directly.
/assign @DanielFroehlich @pmtk @jerpeter1