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

USHIFT-2348: microshift y-2 upgrades #1562

Conversation

dhellmann
Copy link
Contributor

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 8, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 8, 2024

@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:

/assign @DanielFroehlich @pmtk @jerpeter1

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.

Copy link

@DanielFroehlich DanielFroehlich left a 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?

enhancements/microshift/y-2-upgrades.md Outdated Show resolved Hide resolved
enhancements/microshift/y-2-upgrades.md Outdated Show resolved Hide resolved
enhancements/microshift/y-2-upgrades.md Outdated Show resolved Hide resolved
enhancements/microshift/y-2-upgrades.md Outdated Show resolved Hide resolved
enhancements/microshift/y-2-upgrades.md Outdated Show resolved Hide resolved
enhancements/microshift/y-2-upgrades.md Outdated Show resolved Hide resolved
enhancements/microshift/y-2-upgrades.md Outdated Show resolved Hide resolved
enhancements/microshift/y-2-upgrades.md Outdated Show resolved Hide resolved
enhancements/microshift/y-2-upgrades.md Outdated Show resolved Hide resolved
enhancements/microshift/y-2-upgrades.md Outdated Show resolved Hide resolved
@dhellmann dhellmann force-pushed the USHIFT-2348-microshift-y-2-upgrades branch from 762ad26 to 45a9931 Compare February 8, 2024 19:09
@dhellmann
Copy link
Contributor Author

The latest draft addresses all of @DanielFroehlich's comments.

@dhellmann dhellmann force-pushed the USHIFT-2348-microshift-y-2-upgrades branch from 45a9931 to ee74a10 Compare February 8, 2024 19:26
@dhellmann
Copy link
Contributor Author

/assign @jogeo

@dhellmann dhellmann force-pushed the USHIFT-2348-microshift-y-2-upgrades branch 3 times, most recently from 7a9b0bb to 2d495da Compare February 12, 2024 20:27
@dhellmann
Copy link
Contributor Author

#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.

Copy link

@jogeo jogeo left a 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

enhancements/microshift/y-minus-2-upgrades.md Outdated Show resolved Hide resolved
enhancements/microshift/y-minus-2-upgrades.md Show resolved Hide resolved
enhancements/microshift/y-minus-2-upgrades.md Outdated Show resolved Hide resolved
enhancements/microshift/y-minus-2-upgrades.md Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Feb 16, 2024
@dhellmann dhellmann added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 16, 2024
@dhellmann
Copy link
Contributor Author

@jogeo I committed your suggestions, which removed the lgtm you applied. Could you re-apply it, please?

@dhellmann
Copy link
Contributor Author

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
Copy link
Member

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.

Copy link
Contributor Author

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.

@dhellmann dhellmann force-pushed the USHIFT-2348-microshift-y-2-upgrades branch from 5c78f32 to dd9aca3 Compare February 16, 2024 18:52

The main drawback to implementing this enhancement is the increased
test matrix for upgrades. We can automate those tests to minimize the
impact.
Copy link
Contributor

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:

  1. 4.14 -> 4.16 (y-2 released)
  2. 4.15 -> 4.17 (y-2 fake)
  3. 4.15 -> 4.16 (y-1 released)
  4. 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.

Copy link
Contributor Author

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

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.

Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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.

@pmtk
Copy link
Member

pmtk commented Feb 20, 2024

looks good to me

@dhellmann dhellmann force-pushed the USHIFT-2348-microshift-y-2-upgrades branch from dd9aca3 to f080d37 Compare February 20, 2024 16:03
@pmtk
Copy link
Member

pmtk commented Feb 20, 2024

+1

Copy link
Contributor

openshift-ci bot commented Feb 21, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 21, 2024
@pmtk
Copy link
Member

pmtk commented Feb 22, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2024
Copy link
Contributor

openshift-ci bot commented Feb 22, 2024

@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.

@openshift-merge-bot openshift-merge-bot bot merged commit 44489cd into openshift:master Feb 22, 2024
2 checks passed
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

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?

Copy link
Contributor Author

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

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?

Copy link
Contributor Author

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

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?

Copy link
Contributor Author

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

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?

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?

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants