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: change external rgw detection, not relying on cluster #6888

Merged
merged 1 commit into from
Jan 8, 2021
Merged

ceph: change external rgw detection, not relying on cluster #6888

merged 1 commit into from
Jan 8, 2021

Conversation

Zempashi
Copy link
Contributor

@Zempashi Zempashi commented Dec 28, 2020 •
edited

Following #6217, allow rgw to created for external cluster

Signed-off-by: Julien Girardin jugirardin@free.fr

Took me some time to test #6226, but this was not still working, operator claiming:

ceph-object-controller: failed to reconcile failed to create object store deployments: failed to reconcile external endpoint: failed to create or update object store "arch-cloud" endpoint: failed to create endpoint "rook-ceph-rgw-arch-cloud". Endpoints "rook-ceph-rgw-arch-cloud" is invalid: subsets[0]: Required value: must specify `addresses` or `notReadyAddresses`

Change external rgw detection, not relying on cluster. Now there is multiple posibilities:

  • internal cluster and internal rgw pods: "normal case"
  • external cluster and internal rgw pods: <= now working with the PR
  • external cluster and external rgw pods: the external case
  • internal cluster and external rgw pods: new case that could exist, don't know if needed to be preventing or not

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

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 Dec 28, 2020
leseb
leseb requested changes Jan 5, 2021
View reviewed changes
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.

Not sure if this block is relevant anymore https://github.com/rook/rook/blob/master/pkg/operator/ceph/object/rgw.go#L312-L316, you might as well remove it.

pkg/apis/ceph.rook.io/v1/object.go Outdated
@@ -20,6 +20,11 @@ func (s *ObjectStoreSpec) IsMultisite() bool {
return s.Zone.Name != ""
}

func (s *ObjectStoreSpec) IsExternal() bool {
return len(s.Gateway.ExternalRgwEndpoints) != 0

Copy link
Member

Choose a reason for hiding this comment

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

remove empty line

@leseb
Copy link
Member

leseb commented Jan 5, 2021 •
edited

Also, internal cluster and external rgw pods: new case that could exist, don't know if needed to be preventing or not, is acceptable, so let it be.

@Zempashi Zempashi force-pushed the ceph-internal-rgw-on-ext-cluster branch from 53e8291 to 48b67ad Compare January 6, 2021 09:46
@Zempashi Zempashi requested a review from leseb January 6, 2021 09:46
@leseb
Copy link
Member

leseb commented Jan 6, 2021

@Zempashi please fix the unit tests and we will be good to go.

@Zempashi Zempashi force-pushed the ceph-internal-rgw-on-ext-cluster branch from 48b67ad to 7a3ef7c Compare January 6, 2021 21:10
leseb
leseb requested changes Jan 7, 2021
View reviewed changes
pkg/operator/ceph/object/spec_test.go Outdated
@@ -194,10 +194,10 @@ func TestValidateSpec(t *testing.T) {
err = r.validateStore(s)
assert.NoError(t, err)

// external with no endpoints, failure
// external with no endpoints, success
Copy link
Member

Choose a reason for hiding this comment

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

L192 to L200 makes no sense now, so I'd just remove both tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed...

@Zempashi Zempashi force-pushed the ceph-internal-rgw-on-ext-cluster branch from 7a3ef7c to 5564553 Compare January 7, 2021 21:37
leseb
leseb requested changes Jan 8, 2021
View reviewed changes
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.

One last thing, update your commit message with the content of the PR description. Thanks.

@Zempashi
Copy link
Contributor Author

Zempashi commented Jan 8, 2021

Including the test and previous error ?

@Zempashi Zempashi force-pushed the ceph-internal-rgw-on-ext-cluster branch from 5564553 to 3ad2530 Compare January 8, 2021 08:54
@leseb
Copy link
Member

leseb commented Jan 8, 2021

Including the test and previous error ?

The commit message must be self-sufficient, imagine being offline and trying to understand a commit, having a description like:

  • what is the issue?
  • how did you solve it?

is a must-have. Just a bit of rework and we should be good.
Thanks.

@Zempashi
Copy link
Contributor Author

Zempashi commented Jan 8, 2021 •
edited

What do you think of the proposal ? It references the previous issue (I saw in commit og that it is allowed), the error log encountered, the change and the implication. It's really a copy-paste from the issue. I could rework this, but don't know in which way to improve.
Thanks for assisting me through the PR ;)

EDIT: may be making more clear on "what is the issue?" and "how did you solve it?" ?

@Zempashi Zempashi force-pushed the ceph-internal-rgw-on-ext-cluster branch from 3ad2530 to f9516d2 Compare January 8, 2021 09:32
@leseb
Copy link
Member

leseb commented Jan 8, 2021

What do you think of the proposal ? It references the previous issue (I saw in commit og that it is allowed), the error log encountered, the change and the implication. It's really a copy-paste from the issue. I could rework this, but don't know in which way to improve.
Thanks for assisting me through the PR ;)

EDIT: may be making more clear on "what is the issue?" and "how did you solve it?" ?

Yes this will be nice, let me break down your commit a bit:

After #6217, this was not still working, operator claiming:

What is "this"? What was not working?

ceph-object-controller: failed to reconcile failed to create object
store deployments: failed to reconcile external endpoint:
failed to create or update object store "arch-cloud" endpoint:
failed to create endpoint "rook-ceph-rgw-arch-cloud". Endpoints
"rook-ceph-rgw-arch-cloud" is invalid: subsets[0]:
Required value: must specify `addresses` or `notReadyAddresses`

Change external rgw detection, not relying on cluster.

This is good but could be clearer, how did you solve the detection? What's the new detection mechanism?

Now there is multiple posibilities:

  • internal cluster and internal rgw pods: "normal case"
  • external cluster and internal rgw pods: <= now working with the PR
  • external cluster and external rgw pods: the external case
  • internal cluster and external rgw pods: <= new case that could exist

Thanks for your patience

@Zempashi Zempashi force-pushed the ceph-internal-rgw-on-ext-cluster branch from f9516d2 to d06a502 Compare January 8, 2021 09:48
@Zempashi
Copy link
Contributor Author

Zempashi commented Jan 8, 2021

Another try, and I'm the one to thanks for your patience

@leseb
Copy link
Member

leseb commented Jan 8, 2021

Another try, and I'm the one to thanks for your patience

Just one typo in your commit message s/onyl/only/.

@Zempashi
ceph: change external rgw detection, not relying on cluster
d63d9b7 
After rook#6217, internal rgw spawning on external cluster was still broken,
operator claiming:
```
ceph-object-controller: failed to reconcile failed to create object
store deployments: failed to reconcile external endpoint:
failed to create or update object store "arch-cloud" endpoint:
failed to create endpoint "rook-ceph-rgw-arch-cloud". Endpoints
"rook-ceph-rgw-arch-cloud" is invalid: subsets[0]:
Required value: must specify `addresses` or `notReadyAddresses`
```

To spawn internal rgw for external cluster, changes detection of
what mean 'internal' for rgw and only use the externalRgwEndpoints
list for that independently of the status of the cluster
Now there is multiple posibilities:

 * internal cluster and internal rgw pods: "normal case"
 * external cluster and internal rgw pods: <= now working with the PR
 * external cluster and external rgw pods: the external case
 * internal cluster and external rgw pods: <= new case that could exist

Signed-off-by: Julien Girardin <jugirardin@free.fr>
@Zempashi Zempashi force-pushed the ceph-internal-rgw-on-ext-cluster branch from d06a502 to d63d9b7 Compare January 8, 2021 10:28
@Zempashi
Copy link
Contributor Author

Zempashi commented Jan 8, 2021

Thanks for spotting the typo

@leseb leseb merged commit 7929b1f into rook:master Jan 8, 2021
@mergify mergify bot mentioned this pull request Jan 8, 2021
Merged
mergify bot added a commit that referenced this pull request Jan 8, 2021
@mergify
Merge pull request #6929 from rook/mergify/bp/release-1.5/pr-6888
f9aede0 
ceph: change external rgw detection, not relying on cluster (bp #6888)
@Zempashi Zempashi deleted the ceph-internal-rgw-on-ext-cluster branch January 8, 2021 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leseb leseb leseb approved these changes

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.

ceph cluster is external but externalRgwEndpoints list is empty
2 participants
@Zempashi @leseb