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

[OSD on PVC] add kubernetes version check. #4009

Merged
merged 1 commit into from
Oct 21, 2019
Merged

[OSD on PVC] add kubernetes version check. #4009

merged 1 commit into from
Oct 21, 2019

Conversation

sp98
Copy link
Contributor

@sp98 sp98 commented Sep 27, 2019

Signed-off-by: Santosh Pillai sapillai@redhat.com

Description of your changes:
OSD on PVC provisioning fails if k8s version is less than 1.13.0

  • updated document to reflect minimum k8s version required.
  • Added check to skip OSD on pvc provisoning if the minimum k8s version requirement is not met.

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

Checklist:

  • 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.
  • Comments have been added or updated based on the standards set in CONTRIBUTING.md
  • Add the flag for skipping the CI if this PR does not require a build. See here for more details.

[test ceph]

pkg/operator/ceph/cluster/osd/osd.go Outdated Show resolved Hide resolved
@sp98 sp98 changed the title [OSD on PVC][WIP] add kubernetes version check. [OSD on PVC] add kubernetes version check. Sep 27, 2019
@sp98 sp98 requested a review from travisn September 27, 2019 14:16
pkg/operator/ceph/cluster/osd/osd.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, once last thing, thanks.

pkg/operator/ceph/cluster/osd/osd.go Outdated Show resolved Hide resolved
@@ -43,6 +43,7 @@ spec:

## PVC-based Cluster

**NOTE** Kubernetes version should be 1.13.0 or greater to provision OSD on PVC.
Copy link
Member

Choose a reason for hiding this comment

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

suggestion:
**NOTE** Kubernetes version 1.13.0 or greater is required to provision OSDs on PVCs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

//check k8s version
k8sVersion, err := k8sutil.GetK8SVersion(c.context.Clientset)
if err != nil {
logger.Warningf("error finding Kubernetes version. %+v", err)
Copy link
Member

Choose a reason for hiding this comment

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

this should be an error if we're aborting the provisioning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this as a error in the config.

return
}
if !k8sVersion.AtLeast(version.MustParseSemantic("v1.13.0")) {
logger.Warningf("skipping OSD on PVC provisioning. Minimum Kubernetes version required: 1.13.0. Actual version: %s", k8sVersion.String())
Copy link
Member

Choose a reason for hiding this comment

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

We need to think about the right time to log this message.

  • If the user doesn't have any settings in the CR for provisioning OSDs on PVs, we shouldn't even need to add this check. Should we add this check around line 252 below after we check the volume sources?
  • While we're fixing the warnings, I think the one line 247 should just be an info and could be simplified to: No volume sources specified for creating OSDs on PVCs. The rest of the message doesn't even make sense to me now.

Copy link
Member

Choose a reason for hiding this comment

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

Ditto Travis' comment about this being an error message also.

Copy link
Contributor Author

@sp98 sp98 Oct 3, 2019

Choose a reason for hiding this comment

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

We need to think about the right time to log this message.

  • If the user doesn't have any settings in the CR for provisioning OSDs on PVs, we shouldn't even need to add this check. Should we add this check around line 252 below after we check the volume sources?

yes, it would be more appropriate to add this check after checking the volume sources.
Updated the code with this change.

  • While we're fixing the warnings, I think the one line 247 should just be an info and could be simplified to: No volume sources specified for creating OSDs on PVCs. The rest of the message doesn't even make sense to me now.

Updated the message. Should this still be a warning or just an info?

@BlaineEXE BlaineEXE added ceph main ceph tag ceph-osd labels Oct 1, 2019
@ghost
Copy link

ghost commented Oct 3, 2019

There were the following issues with this Pull Request

  • Commit: 4cd69cc
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

  - OSD on PVC provisioning fails if k8s version is less than 1.13.0
  - updated document to reflect minimum k8s version required.
  - Added check to skip OSD on pvc provisoning if the minimum k8s version requirement is not met.

Signed-off-by: Santosh Pillai <sapillai@redhat.com>
@leseb leseb merged commit 1081c58 into rook:master Oct 21, 2019
mergify bot added a commit that referenced this pull request Oct 22, 2019
[OSD on PVC] add kubernetes version check. (bp #4009)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ceph main ceph tag ceph-osd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[OSD on PVC] Add check for minimum k8s version
4 participants