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

ceph: improve upgrade procedure #2901

Merged
merged 12 commits into from
Jul 17, 2019
Merged

ceph: improve upgrade procedure #2901

merged 12 commits into from
Jul 17, 2019

Conversation

leseb
Copy link
Member

@leseb leseb commented Mar 27, 2019

Description of your changes:

When a cluster is updated with a different image version, this triggers
a serialized restart of all the pods. Prior to this commit, no safety
check were performed and rook was hoping for the best outcome.

Now before doing restarting a daemon we check it can be restarted. Once
it's restarted we also check we can pursue with the rest of the
platform. For instance, with monitors we check that they are in quorum,
for OSD we check that PGs are clean and for MDS we make sure they are
active.

Fixes: #2889
Signed-off-by: Sébastien Han seb@redhat.com

Which issue is resolved by this Pull Request:
Resolves #2889

Checklist:

  • Documentation has been updated, if necessary.
  • Pending release notes updated with breaking and/or notable changes, if necessary.
  • Upgrade from previous release is tested and upgrade user guide is updated, if necessary.
  • Code generation (make codegen) has been run to update object specifications, if necessary.
  • Comments have been added or updated based on the standards set in CONTRIBUTING.md

[skip ci]
Known issue.

@leseb
Copy link
Member Author

leseb commented Mar 27, 2019

This is wip since it relies on ceph/ceph#27146. I'll finish this once it's merged.

@galexrt galexrt added ceph main ceph tag upgrade labels Apr 11, 2019
@leseb leseb force-pushed the improve-upgrade branch 3 times, most recently from e52c9fc to a5c3a19 Compare April 17, 2019 17:41
@leseb leseb marked this pull request as ready for review April 17, 2019 17:41
@leseb
Copy link
Member Author

leseb commented Apr 17, 2019

Ready for an initial review, I'll probably split the commit into multiple ones later.

@leseb leseb added this to the 1.0 milestone Apr 18, 2019
@travisn travisn mentioned this pull request Apr 18, 2019
5 tasks
pkg/operator/ceph/upgrade/upgrade.go Outdated Show resolved Hide resolved
pkg/operator/ceph/upgrade/upgrade.go Outdated Show resolved Hide resolved
pkg/operator/k8sutil/deployment.go Outdated Show resolved Hide resolved
pkg/daemon/ceph/config/config.go Outdated Show resolved Hide resolved
pkg/operator/ceph/cluster/mon/mon.go Outdated Show resolved Hide resolved
pkg/operator/ceph/cluster/mon/mon.go Show resolved Hide resolved
pkg/operator/ceph/upgrade/upgrade.go Outdated Show resolved Hide resolved
pkg/operator/k8sutil/deployment.go Outdated Show resolved Hide resolved
pkg/operator/k8sutil/deployment.go Outdated Show resolved Hide resolved
pkg/operator/k8sutil/deployment.go Outdated Show resolved Hide resolved
@BlaineEXE
Copy link
Member

I don't have good feelings about merging this right before the v1.0 release. Something that could be an issue that has been an issue for the mon init in the past is that Rook has to be able to get the cluster into a working state if the operator fails midway through any of these interactions, and that failure might cause mons or other daemons to be in unhealthy states. If Rook's actions allow a daemon to get messed up, it seems right to me that Rook should be able to rectify that.

Having a strict check for healthiness before continuing with an upgrade is something that is absolutely needed IMO, but I think there needs to be a lot of consideration and vetting before I would feel that what we have is comfortably stable and won't risk upgrade pains for users.

I would vote that we (1) wait until v1.0 is released before merging this, and I would vote that (2) this PR should be split into different PRs for health checking of different daemons so the changes are smaller and easier to reason through.

@leseb
Copy link
Member Author

leseb commented Apr 24, 2019

A simpler approach here: #3037
I keep this one around and will resume once 1.0 has merged.

@leseb leseb added the do-not-merge DO NOT MERGE :) label Apr 24, 2019
@leseb leseb modified the milestones: 1.0, 1.1 Apr 24, 2019
@BlaineEXE
Copy link
Member

I went through some logic in the past to see if we could rely on health checks to make sure mons are healthy before proceeding with upgrades, and it's tricky stuff. It could be possible for the mons to get into a state where they aren't healthy and where Rook won't correct an issue it is theoretically able to solve because it is waiting on a prerequisite status that would be resolved by continuing to orchestrate.

The code here looks good, and I think it makes sense, but I want to make sure we aren't missing anything. What can we do to make sure we aren't allowing the mons to be deadlocked in an error state?

I think I would be in favor of merging this code early in the v1.1 development phase so we can hopefully see any wrinkles that need fixed before release, because I'm not sure if we can really catch all the little issues. In practicality, I think we could stand to redesign Rook's monitor bootstrap/managment/upgrades from the ground up and focus on (1) creating an idempotent orchestration and (2) testing what happens when the operator crashes at each step so that we can be more sure that we can't get into a deadlocked state.

@leseb
Copy link
Member Author

leseb commented Jun 4, 2019

@BlaineEXE thanks for your feedback, let me do more tests this week so hopefully, we can move forward.

@leseb leseb force-pushed the improve-upgrade branch 2 times, most recently from 896beef to 8fa5bc9 Compare June 5, 2019 14:40
These were used on a previous patch as a debug, let's remove them. They
are not intended to be displayed.

Signed-off-by: Sébastien Han <seb@redhat.com>
If the cluster shows a state of HEALTH_ERR, Rook will refuse to upgrade
it if the image version changed.

Signed-off-by: Sébastien Han <seb@redhat.com>
Add more details on how upgrades are performed, as well as updating the
release note.

Signed-off-by: Sébastien Han <seb@redhat.com>
Do not consider clusterInfo Ceph version since it's the one picked by
the spec image.
The CephVersion provided here needs to be the current running version
of Ceph for when it is later passed to OkToStop and checked with
IsAtLeast. If you are upgrading from a pre 14.2.1 version to a
version 14.2.1 or newer, IsAtLeast will incorrectly return true and
then fail trying to run mon ok-to-stop on a mon pod running a pre 14.2.1 version.

Signed-off-by: Sébastien Han <seb@redhat.com>
If we successfully changed all the Ceph version of all the daemon we
consider the upgrade complete and successfully, thus we display a
message.

Signed-off-by: Sébastien Han <seb@redhat.com>
Logs will be lost if we collect them after pods are uninstalled.
So we need to collect them before purging.

Signed-off-by: Sébastien Han <seb@redhat.com>
If we detect all OSDs are running on the same host, we don't perform any
checks for upgrade.

Signed-off-by: Sébastien Han <seb@redhat.com>
This commit makes OnAdd() and OnUpdate() consistent in behavior. They
will do their best detecting if we can upgrade, continue an upgrade or
refuse to do one. It also refuses to downgrade to any version.
It also load ClusterInfo earlier in the controller so that we can
compare the spec version versus the cluster running one.

Signed-off-by: Sébastien Han <seb@redhat.com>
We do not determine the daemon type and name by string parsing anymore
but passing that info directly in the update call, it's more reliable.
Also removing a bunch of unused functions.

Signed-off-by: Sébastien Han <seb@redhat.com>
@leseb
Copy link
Member Author

leseb commented Jul 17, 2019

Triggered the CI 3 times since yesterday and always randomly fails on different jobs. So merging like this.

@leseb leseb merged commit 50ef726 into rook:master Jul 17, 2019
@leseb leseb deleted the improve-upgrade branch July 17, 2019 09:54
travisn added a commit to travisn/rook that referenced this pull request Jul 25, 2019
The upgrade changes for rook#2901 added the check for the correct version
of the ceph image before continuing with an upgrade. This commit is
to refactor that change to work with the new code path to validate
the ceph version.

Signed-off-by: travisn <tnielsen@redhat.com>
travisn added a commit to travisn/rook that referenced this pull request Jul 26, 2019
The upgrade changes for rook#2901 added the check for the correct version
of the ceph image before continuing with an upgrade. This commit is
to refactor that change to work with the new code path to validate
the ceph version.

Signed-off-by: travisn <tnielsen@redhat.com>
travisn added a commit to travisn/rook that referenced this pull request Jul 27, 2019
The upgrade changes for rook#2901 added the check for the correct version
of the ceph image before continuing with an upgrade. This commit is
to refactor that change to work with the new code path to validate
the ceph version.

Signed-off-by: travisn <tnielsen@redhat.com>
travisn added a commit to travisn/rook that referenced this pull request Aug 1, 2019
The upgrade changes for rook#2901 added the check for the correct version
of the ceph image before continuing with an upgrade. This commit is
to refactor that change to work with the new code path to validate
the ceph version.

Signed-off-by: travisn <tnielsen@redhat.com>
travisn added a commit to travisn/rook that referenced this pull request Aug 3, 2019
The upgrade changes for rook#2901 added the check for the correct version
of the ceph image before continuing with an upgrade. This commit is
to refactor that change to work with the new code path to validate
the ceph version.

Signed-off-by: travisn <tnielsen@redhat.com>
travisn added a commit to travisn/rook that referenced this pull request Aug 5, 2019
The upgrade changes for rook#2901 added the check for the correct version
of the ceph image before continuing with an upgrade. This commit is
to refactor that change to work with the new code path to validate
the ceph version.

Signed-off-by: travisn <tnielsen@redhat.com>
travisn added a commit to travisn/rook that referenced this pull request Aug 7, 2019
The upgrade changes for rook#2901 added the check for the correct version
of the ceph image before continuing with an upgrade. This commit is
to refactor that change to work with the new code path to validate
the ceph version.

Signed-off-by: travisn <tnielsen@redhat.com>
travisn added a commit to travisn/rook that referenced this pull request Aug 10, 2019
The upgrade changes for rook#2901 added the check for the correct version
of the ceph image before continuing with an upgrade. This commit is
to refactor that change to work with the new code path to validate
the ceph version.

Signed-off-by: travisn <tnielsen@redhat.com>
travisn added a commit to travisn/rook that referenced this pull request Aug 11, 2019
The upgrade changes for rook#2901 added the check for the correct version
of the ceph image before continuing with an upgrade. This commit is
to refactor that change to work with the new code path to validate
the ceph version.

Signed-off-by: travisn <tnielsen@redhat.com>
travisn added a commit to travisn/rook that referenced this pull request Aug 12, 2019
The upgrade changes for rook#2901 added the check for the correct version
of the ceph image before continuing with an upgrade. This commit is
to refactor that change to work with the new code path to validate
the ceph version.

Signed-off-by: travisn <tnielsen@redhat.com>
travisn added a commit to travisn/rook that referenced this pull request Aug 12, 2019
The upgrade changes for rook#2901 added the check for the correct version
of the ceph image before continuing with an upgrade. This commit is
to refactor that change to work with the new code path to validate
the ceph version.

Signed-off-by: travisn <tnielsen@redhat.com>
travisn added a commit to travisn/rook that referenced this pull request Aug 13, 2019
The upgrade changes for rook#2901 added the check for the correct version
of the ceph image before continuing with an upgrade. This commit is
to refactor that change to work with the new code path to validate
the ceph version.

Signed-off-by: travisn <tnielsen@redhat.com>
travisn added a commit to travisn/rook that referenced this pull request Aug 13, 2019
The upgrade changes for rook#2901 added the check for the correct version
of the ceph image before continuing with an upgrade. This commit is
to refactor that change to work with the new code path to validate
the ceph version.

Signed-off-by: travisn <tnielsen@redhat.com>
leseb pushed a commit to leseb/rook that referenced this pull request Oct 3, 2019
The upgrade changes for rook#2901 added the check for the correct version
of the ceph image before continuing with an upgrade. This commit is
to refactor that change to work with the new code path to validate
the ceph version.

Signed-off-by: travisn <tnielsen@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ceph main ceph tag upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Ceph upgrade pod recreation gates
6 participants