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

object: add missing cephcluster spec addition in object controller #12273

Merged
merged 1 commit into from
May 30, 2023

Conversation

thotz
Copy link
Contributor

@thotz thotz commented May 23, 2023

Description of your changes:
The ceph cluster spec need to set on object context for executing radosgw-admin commands if multus is enabled.
The radosgw-admin command uses the network spec from ceph cluster spec
in object context but it is not filled properly in the object package.
But with PR 10898, network spec is available in clusterinfo which can
be used directly. Also removed cluserspec from object context.

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: If this is only a documentation change, add the label skip-ci on the PR.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

pkg/operator/ceph/object/controller.go Outdated Show resolved Hide resolved
@thotz thotz force-pushed the multus-cephclusterspec-update branch from 22cf72e to 638a44a Compare May 24, 2023 13:03
@BlaineEXE
Copy link
Member

I think it's appropriate to squash these commits together, and also make sure there is a sentence that says why the ceph cluster is removed -- what is removed and no longer needed in the second commit. Otherwise, I think this looks good 👍 thanks

Copy link
Member

@BlaineEXE BlaineEXE left a comment

Choose a reason for hiding this comment

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

Also, since this is more about the object controller than the RGW itself, let's use object: ... for the commit and PR title rather than rgw: ...

Copy link
Member

@BlaineEXE BlaineEXE left a comment

Choose a reason for hiding this comment

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

Also, since this is more about the object controller than the RGW itself, let's use object: ... for the commit and PR title rather than rgw: ...

@thotz thotz changed the title rgw: add missing cephcluster spec addition in object controller object: add missing cephcluster spec addition in object controller May 25, 2023
@thotz thotz force-pushed the multus-cephclusterspec-update branch from 80fa9f3 to 0f0f7cb Compare May 25, 2023 07:15
@thotz thotz requested a review from BlaineEXE May 25, 2023 07:15
@BlaineEXE
Copy link
Member

@thotz Please take care of the commitlint error as well

@thotz thotz force-pushed the multus-cephclusterspec-update branch 4 times, most recently from c1b9afd to 70b6aff Compare May 29, 2023 09:52
@thotz
Copy link
Contributor Author

thotz commented May 29, 2023

@thotz Please take care of the commitlint error as well

Sorry I tried all the possibilities which I can I don't understand what they meant by


The radosgw-admin command uses the network spec from ceph cluster spec
in object context but it is not filled properly in the object package.

But with PR #10898, network spec is available in clusterinfo which can
be used directly. Also removed cluserspec from object context.

Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>
✖   footer must have leading blank line [footer-leading-blank]

I have given blank line after Signed-off-by: still its complaining the same

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.

regarding, commitlint, try remove # from the description. see if that helps

…dmin

The radosgw-admin command uses the network spec from ceph cluster spec
in object context but it is not filled properly in the object package.
But with PR 10898, network spec is available in clusterinfo which can
be used directly. Also removed cluserspec from object context.

Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>
@thotz thotz force-pushed the multus-cephclusterspec-update branch from 70b6aff to 1f45cfa Compare May 30, 2023 05:36
@thotz
Copy link
Contributor Author

thotz commented May 30, 2023

regarding, commitlint, try remove # from the description. see if that helps

Thanks it worked

@@ -214,7 +213,7 @@ func RunAdminCommandNoMultisite(c *Context, expectJSON bool, args ...string) (st
var err error

// If Multus is enabled we proxy all the command to the mgr sidecar
if c.CephClusterSpec.Network.IsMultus() {
Copy link
Member

Choose a reason for hiding this comment

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

Does this was not working?

Hope you have tested the changes by creating the buckets claims and checked every thing works fine....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some parts of the code, object context was not set with clusterspec, we already had the Network info in clusterinfo. Hence changed this check instead of filling clusterspec to object context

@BlaineEXE BlaineEXE merged commit 870a597 into rook:master May 30, 2023
50 checks passed
BlaineEXE added a commit that referenced this pull request May 30, 2023
object: add missing cephcluster spec addition in object controller (backport #12273)
@thotz thotz deleted the multus-cephclusterspec-update branch May 31, 2023 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants