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.

Sign up for GitHub

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

Jump to bottom

ceph: fix panic when recreating the csidriver object #8582

Merged
merged 3 commits into from Aug 24, 2021
Merged

ceph: fix panic when recreating the csidriver object #8582

merged 3 commits into from Aug 24, 2021

Conversation

Madhu-1
Copy link
Member

@Madhu-1 Madhu-1 commented Aug 24, 2021

Description of your changes:

This PR fixes the below items

  • fix panic in reCreateCSIDriverInfo
    Initialize the client and the csidriver object before calling the reCreateCSIDriverInfo function.
  • Reuse defined csiDriver object
    reuse already defined csiDriver object instead of referring to the method receiver
  • fix logging of csidriver
    log the successful message about starting CSIDriver after creating the daemonset and deployment objects.

Signed-off-by: Madhu Rajanna madhupr007@gmail.com

Note: the issue was reported at slack https://rook-io.slack.com/archives/CK9CF5H2R/p1629776387282600

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.

@Madhu-1
ceph: fix panic in reCreateCSIDriverInfo
d48f67a 
Initialize the client and the csidriver object
before calling the reCreateCSIDriverInfo
function.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@Madhu-1
Copy link
Member Author

Madhu-1 commented Aug 24, 2021

Note:- This needs to be backported to 1.6 and 1.7

@mergify mergify bot added the ceph main ceph tag label Aug 24, 2021
@Madhu-1 Madhu-1 mentioned this pull request Aug 24, 2021
Merged
10 tasks
@Madhu-1 Madhu-1 requested a review from leseb August 24, 2021 12:47
travisn
travisn requested changes Aug 24, 2021
View reviewed changes
pkg/operator/ceph/csi/spec.go
@@ -584,6 +583,7 @@ func startDrivers(clientset kubernetes.Interface, rookclientset rookclient.Inter
return errors.Wrapf(err, "failed to start cephfs provisioner deployment: %+v", cephfsProvisionerDeployment)
}
k8sutil.AddRookVersionLabelToDeployment(cephfsProvisionerDeployment)
logger.Info("successfully started CSI CephFS driver")
Copy link
Member

Choose a reason for hiding this comment

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

Shall we have a separate message for the volume plugin (line 555) and the provisioner here?

Suggested change
logger.Info("successfully started CSI CephFS driver")
logger.Info("successfully started CSI CephFS provisioner")

Copy link
Member Author

Choose a reason for hiding this comment

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

@travisn as we are starting both provisioner and daemonset I left the logging as it was previously

pkg/operator/ceph/csi/spec.go
@@ -516,6 +514,7 @@ func startDrivers(clientset kubernetes.Interface, rookclientset rookclient.Inter
return errors.Wrapf(err, "failed to start rbd provisioner deployment: %+v", rbdProvisionerDeployment)
}
k8sutil.AddRookVersionLabelToDeployment(rbdProvisionerDeployment)
logger.Info("successfully started CSI Ceph RBD driver")
Copy link
Member

Choose a reason for hiding this comment

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

Same suggestion as below

Suggested change
logger.Info("successfully started CSI Ceph RBD driver")
logger.Info("successfully started CSI Ceph RBD provisioner")

pkg/operator/ceph/csi/csidriver.go Show resolved Hide resolved
@Madhu-1
ceph: fix logging of csidriver
e2dc418 
log the successful message about
starting CSIDriver after creating
the daemonset and deployment objects.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@Madhu-1
ceph: remove variable declaration
d246dde 
remove csiDriver and csiClient variables
creation and reuse them from the method
receivers.
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@Madhu-1 Madhu-1 requested a review from travisn August 24, 2021 14:14
travisn
travisn approved these changes Aug 24, 2021
View reviewed changes
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.

Were you able to repro before the fix, or this is based on the stack trace?

@Madhu-1
Copy link
Member Author

Madhu-1 commented Aug 24, 2021

Were you able to repro before the fix, or this is based on the stack trace?

Reproduced and tested the fix

@travisn travisn merged commit 9a96afa into rook:master Aug 24, 2021
This was referenced Aug 24, 2021
Merged
Merged
@Madhu-1 Madhu-1 mentioned this pull request Aug 26, 2021
Closed
travisn added a commit that referenced this pull request Aug 26, 2021
@travisn
Merge pull request #8585 from rook/mergify/bp/release-1.6/pr-8582
d96c48b 
ceph: fix panic when recreating the csidriver object (backport #8582)
travisn added a commit that referenced this pull request Aug 26, 2021
@travisn
Merge pull request #8586 from rook/mergify/bp/release-1.7/pr-8582
6ca2e00 
ceph: fix panic when recreating the csidriver object (backport #8582)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@travisn travisn travisn approved these changes

@Yuggupta27 Yuggupta27 Yuggupta27 approved these changes

@leseb leseb Awaiting requested review from leseb

Assignees
No one assigned
Labels
ceph main ceph tag
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Madhu-1 @travisn @Yuggupta27 @leseb