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: correctly return if multus was applied #6152

Merged
merged 1 commit into from
Aug 25, 2020
Merged

Conversation

leseb
Copy link
Member

@leseb leseb commented Aug 25, 2020

Description of your changes:

We must only return if multus got configured successfully.

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

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

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.

We must only return if multus got configured successfully.

Signed-off-by: Sébastien Han <seb@redhat.com>
@leseb leseb requested review from travisn and Madhu-1 August 25, 2020 07:56
@mergify mergify bot added the ceph main ceph tag label Aug 25, 2020
@leseb leseb mentioned this pull request Aug 25, 2020
10 tasks
@@ -523,6 +523,7 @@ func deleteCSIDriverResources(
}

func applyCephClusterNetworkConfig(objectMeta *metav1.ObjectMeta, rookclientset rookclient.Interface) (bool, error) {
var isMultusApplied bool
cephClusters, err := rookclientset.CephV1().CephClusters(objectMeta.Namespace).List(metav1.ListOptions{})
Copy link
Member

Choose a reason for hiding this comment

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

not related to this PR, but still a question objectMeta.Namespace is the namespace of the operator as csi pods will start in rook operator namespace. will this check passes if we have ceph clusters in different namespaces?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rohan47 is working on a fix for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

So if the CephCluster is not in the operator namespace this won't work.

Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

LGTM

}
}

return true, nil
return isMultusApplied, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

@leseb do we need this bool ? or cant we just survive with 'error' return in caller for nil , or error ? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

The bool checks whether multus is applied and error checks for any underlaying error so I think it's cleaner this way :)

@leseb leseb merged commit 69f6e87 into rook:master Aug 25, 2020
@leseb leseb deleted the fix-multus-csi branch August 25, 2020 09:56
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.

SmokeSuite is failing in master with test pod failing to mount a volume
3 participants