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: update PodDisruptionBudget from v1beta1 to v1 #7977

Merged
merged 1 commit into from Jul 22, 2021

Conversation

parth-gr
Copy link
Member

@parth-gr parth-gr commented May 24, 2021

This commit update the PodDisruptionBudget policy to use version v1
Updated to policy/v1 as policy/v1beta1 PodDisruptionBudget is deprecated in v1.21+

Closes: #7917
Signed-off-by: parth-gr paarora@redhat.com

Description of your changes:

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Skip Tests for Docs: Add the flag for skipping the build if this is only a documentation change. See here for the flag.
  • Skip Unrelated Tests: Add a flag to run tests for a specific storage provider. See test options.
  • Reviewed the developer guide on Submitting a Pull Request
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, 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.

@mergify mergify bot added the ceph main ceph tag label May 24, 2021
@parth-gr
Copy link
Member Author

As discuss in this thread we need to wait for controller runtime 0.9 version for using v1.21 dependencies.

@parth-gr parth-gr force-pushed the update-PodDisruptionBudget branch from 87f5914 to 0cd49ab Compare May 24, 2021 14:59
@parth-gr parth-gr requested a review from subhamkrai May 24, 2021 15:01
@parth-gr parth-gr marked this pull request as draft May 24, 2021 15:02
@parth-gr parth-gr requested a review from sp98 May 24, 2021 15:06
@parth-gr parth-gr force-pushed the update-PodDisruptionBudget branch from 0cd49ab to 66e5098 Compare May 25, 2021 14:52
@travisn
Copy link
Member

travisn commented May 26, 2021

For the PDB version we likely need a similar approach to the CronJob, see this comment

@leseb
Copy link
Member

leseb commented Jun 8, 2021

FYI https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.9.0

@subhamkrai
Copy link
Contributor

#8087

@parth-gr parth-gr force-pushed the update-PodDisruptionBudget branch 2 times, most recently from 916cc38 to 83e223f Compare June 9, 2021 13:19
@parth-gr parth-gr marked this pull request as ready for review June 9, 2021 13:19
Copy link
Contributor

@subhamkrai subhamkrai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to handle v1beta1 too.

@parth-gr parth-gr added the WIP Work in Progress label Jun 23, 2021
@parth-gr parth-gr force-pushed the update-PodDisruptionBudget branch 3 times, most recently from c89fd4b to df6e787 Compare June 29, 2021 15:05
Copy link
Contributor

@subhamkrai subhamkrai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unit test failing

pkg/operator/ceph/cluster/mon/drain.go Outdated Show resolved Hide resolved
pkg/operator/ceph/cluster/mon/drain.go Outdated Show resolved Hide resolved
pkg/operator/ceph/disruption/clusterdisruption/add.go Outdated Show resolved Hide resolved
pkg/operator/ceph/disruption/clusterdisruption/osd.go Outdated Show resolved Hide resolved
pkg/operator/ceph/disruption/clusterdisruption/osd.go Outdated Show resolved Hide resolved
pkg/operator/ceph/disruption/clusterdisruption/osd.go Outdated Show resolved Hide resolved
@parth-gr parth-gr force-pushed the update-PodDisruptionBudget branch from df6e787 to 28680f2 Compare July 2, 2021 14:56
@parth-gr parth-gr removed the WIP Work in Progress label Jul 2, 2021
@parth-gr parth-gr force-pushed the update-PodDisruptionBudget branch 3 times, most recently from 93d47c2 to 44bc2e0 Compare July 8, 2021 12:02
Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI looks green, almost there!

pkg/operator/ceph/disruption/clusterdisruption/add.go Outdated Show resolved Hide resolved
@@ -262,7 +263,17 @@ func (r *ReconcileClusterDisruption) deleteDrainCanaryPods(namespace string) err
}

func (r *ReconcileClusterDisruption) deleteLegacyPDBForOSD(namespace string) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sp98 When did we get rid of the legacy PDBs? I think we can get rid of this method now since it's already been at least a release since we removed the legacy PDBs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We added this in rook 1.5 in this PR. So should be safe to remove now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sp98 We want to remove it in this PR or maybe in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

separate PR, IMO.

Copy link
Contributor

@sp98 sp98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits

pkg/operator/ceph/cluster/mon/drain.go Outdated Show resolved Hide resolved
pkg/operator/ceph/cluster/mon/drain.go Outdated Show resolved Hide resolved
@sp98
Copy link
Contributor

sp98 commented Jul 15, 2021

also, you need to see the upgrade scenario, see this comment from @sp98 on cronJob pr

@subhamkrai I have tested this situation by upgrading the k8s version(case 1.20.0 to 1.21.0).

The operator logs don't show any such Warnings.
But while calling the pdb pods it shows this :

@parth-gr - Were the pdbs moved from v1beta1 to v1 after you upgraded to 1.21?

**kubectl get pdb -nrook-ceph**
Warning: policy/v1beta1 PodDisruptionBudget is deprecated in v1.21+, unavailable in v1.25+; use policy/v1 PodDisruptionBudget
NAME                MIN AVAILABLE   MAX UNAVAILABLE   ALLOWED DISRUPTIONS   AGE
rook-ceph-mon-pdb   N/A             1                 1                     14m
rook-ceph-osd       N/A             1                 1                     18m

But after few minutes it goes away, I assume it is there till the time the cluster reconciles completely.
And rook-ceph-osd remains as it is.

EDIT : I looked into the operator log I see this warning too, as expected as we can not connect through the client while the cluster is down.

W0709 14:14:20.091591       9 reflector.go:436] pkg/mod/k8s.io/client-go@v0.21.2/tools/cache/reflector.go:167: watch of *v1.PodDisruptionBudget ended with: an error on the server ("unable to decode an event from the watch stream: http2: client connection lost") has prevented the request from succeeding
W0709 14:14:20.092998       9 reflector.go:436] pkg/mod/k8s.io/client-go@v0.21.2/tools/cache/reflector.go:167: watch of *v1beta1.PodDisruptionBudget ended with: an error on the server ("unable to decode an event from the watch stream: http2: client connection lost") has prevented the request from succeeding 

And these also goes away after the reconcile is completed.

@parth-gr
Copy link
Member Author

@parth-gr - Were the pdbs moved from v1beta1 to v1 after you upgraded to 1.21?

Yes @sp98, pdbs are getting upgraded to v1.

 "apiVersion": "policy/v1",
  "kind": "PodDisruptionBudget",

But it takes one reconcile time to get the cluster in the ready state as we are upgrading, so the operator restarts and in between it's shows warning.

pkg/operator/ceph/disruption/clusterdisruption/pools.go Outdated Show resolved Hide resolved
pkg/operator/ceph/disruption/clusterdisruption/pools.go Outdated Show resolved Hide resolved
pkg/operator/k8sutil/k8sutil.go Outdated Show resolved Hide resolved
Copy link
Member

@leseb leseb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

pkg/operator/ceph/cluster/mon/drain.go Outdated Show resolved Hide resolved
pkg/operator/ceph/disruption/clusterdisruption/add.go Outdated Show resolved Hide resolved
pkg/operator/ceph/disruption/clusterdisruption/osd.go Outdated Show resolved Hide resolved
pkg/operator/ceph/disruption/clusterdisruption/osd.go Outdated Show resolved Hide resolved
pkg/operator/ceph/disruption/clusterdisruption/osd.go Outdated Show resolved Hide resolved
pkg/operator/ceph/disruption/clusterdisruption/pools.go Outdated Show resolved Hide resolved
pkg/operator/k8sutil/k8sutil.go Outdated Show resolved Hide resolved
This commit update the PodDisruptionBudget policy to use version v1
Updated to policy/v1 as policy/v1beta1 PodDisruptionBudget is deprecated in v1.21+

Closes: rook#7917
Signed-off-by: parth-gr <paarora@redhat.com>
@travisn travisn merged commit ab728e0 into rook:master Jul 22, 2021
mergify bot added a commit that referenced this pull request Jul 22, 2021
ceph: update PodDisruptionBudget from v1beta1 to v1 (backport #7977)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ceph main ceph tag
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use policy/v1 PodDisruptionBudget
5 participants