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: Add interactive and preseed options to configure a dedicated Ceph network in MicroCloud #274

Merged
merged 11 commits into from
Jun 11, 2024
Merged

Ceph: Add interactive and preseed options to configure a dedicated Ceph network in MicroCloud #274

merged 11 commits into from
Jun 11, 2024

Conversation

gabrielmougard
Copy link
Contributor

@gabrielmougard gabrielmougard commented Apr 2, 2024 •
edited
Loading

See https://discourse.ubuntu.com/t/add-options-for-configuring-a-dedicated-ceph-network-in-microcloud/43687
JIRA ticket: https://warthogs.atlassian.net/jira/software/c/projects/LXD/boards/54?assignee=63d9eb6128cddcc70770aff1&selectedIssue=LXD-636

  • Bootstrapping with a dedicated Ceph Network
  • Adding new cluster members to a cluster with an existing dedicated Ceph network
    • If the new cluster members have the right physical network interfaces with IPs that are within the subnet, it should work. Else, error out.

tomponline
tomponline reviewed Apr 2, 2024
View reviewed changes
microcloud/go.mod Outdated Show resolved Hide resolved
@gabrielmougard gabrielmougard force-pushed the feat/ceph-dedicated-nets branch 7 times, most recently from 1dc28b2 to 6bf4b4f Compare April 3, 2024 18:14
@gabrielmougard gabrielmougard force-pushed the feat/ceph-dedicated-nets branch 2 times, most recently from e61f999 to 617159b Compare April 4, 2024 16:20
@gabrielmougard gabrielmougard self-assigned this Apr 4, 2024
sabaini
sabaini reviewed Apr 5, 2024
View reviewed changes
Copy link

@sabaini sabaini left a comment

Choose a reason for hiding this comment

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

Generally lgtm, two comments below.

As a heads-up separating public and cluster nets for Ceph was a relatively frequently requested feature so I wonder if this should be really conflated (will add to the spec as well).

microcloud/cmd/microcloud/main_init.go Outdated Show resolved Hide resolved
microcloud/cmd/microcloud/main_init.go Outdated Show resolved Hide resolved
ru-fu
ru-fu reviewed Apr 12, 2024
View reviewed changes
doc/.wordlist.txt Outdated Show resolved Hide resolved
doc/explanation/microcloud.rst Outdated Show resolved Hide resolved
doc/tutorial/get_started.rst Outdated Show resolved Hide resolved
doc/tutorial/get_started.rst Outdated Show resolved Hide resolved
@gabrielmougard gabrielmougard force-pushed the feat/ceph-dedicated-nets branch 2 times, most recently from 9769473 to a173647 Compare April 12, 2024 15:53
@gabrielmougard
Copy link
Contributor Author

@markylaing @masnax @ru-fu ready for review :)

ru-fu
ru-fu reviewed Apr 15, 2024
View reviewed changes
Copy link
Contributor

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

This looks really good for the documentation already! A few more changes needed though (of course 😉 ).

doc/explanation/microcloud.rst Outdated Show resolved Hide resolved
doc/explanation/microcloud.rst Outdated Show resolved Hide resolved
doc/explanation/microcloud.rst Outdated Show resolved Hide resolved
doc/explanation/microcloud.rst Outdated Show resolved Hide resolved
doc/explanation/microcloud.rst Outdated Show resolved Hide resolved
doc/how-to/ceph_networking.rst Outdated Show resolved Hide resolved
doc/how-to/ceph_networking.rst Outdated Show resolved Hide resolved
doc/how-to/ceph_networking.rst Outdated Show resolved Hide resolved
doc/tutorial/get_started.rst Outdated Show resolved Hide resolved
doc/tutorial/get_started.rst Outdated Show resolved Hide resolved
@gabrielmougard
Copy link
Contributor Author

@ru-fu doc udpated

markylaing
markylaing requested changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@markylaing markylaing left a comment

Choose a reason for hiding this comment

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

Overall it looks really good :)

One thing that's missing though are some tests. Is that something you would be able to incorporate?

microcloud/service/lxd.go Outdated Show resolved Hide resolved
microcloud/service/lxd.go Outdated Show resolved Hide resolved
microcloud/cmd/microcloud/main_init.go Outdated Show resolved Hide resolved
markylaing
markylaing reviewed May 28, 2024
View reviewed changes
Copy link
Contributor

@markylaing markylaing left a comment

Choose a reason for hiding this comment

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

Looks great 😃. Lots of docs and tests. I just have one comment and one nit.

service/lxd.go Outdated Show resolved Hide resolved
test/includes/microcloud.sh Outdated Show resolved Hide resolved
@gabrielmougard gabrielmougard mentioned this pull request Jun 3, 2024
Closed
2 tasks
@masnax
Copy link
Contributor

masnax commented Jun 6, 2024

@gabrielmougard

I seem to be consistently seeing this error at the same spot: https://github.com/canonical/microcloud/actions/runs/9347254250/job/25868665575?pr=274#step:13:9857

Error: failed to fetch cluster config: Get "http://control.socket/1.0/services/microceph/1.0/configs?target=10.186.56.49": EOF, Key:

@gabrielmougard
Copy link
Contributor Author

@gabrielmougard

I seem to be consistently seeing this error at the same spot: https://github.com/canonical/microcloud/actions/runs/9347254250/job/25868665575?pr=274#step:13:9857

Error: failed to fetch cluster config: Get "http://control.socket/1.0/services/microceph/1.0/configs?target=10.186.56.49": EOF, Key:

working on it.

@gabrielmougard gabrielmougard force-pushed the feat/ceph-dedicated-nets branch 2 times, most recently from 3d5269c to 04b91a6 Compare June 7, 2024 18:36
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 7, 2024
View reviewed changes
service/microceph.go Dismissed Show dismissed Hide dismissed
@gabrielmougard
Copy link
Contributor Author

@masnax tests should be passing

markylaing
markylaing requested changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@markylaing markylaing left a comment

Choose a reason for hiding this comment

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

A few nits but other than that it looks good to go :)

service/lxd.go Outdated Show resolved Hide resolved
service/lxd.go Outdated Show resolved Hide resolved
service/lxd.go Outdated Show resolved Hide resolved
cmd/microcloud/main_init.go Show resolved Hide resolved
doc/.wordlist.txt Outdated Show resolved Hide resolved
@gabrielmougard
microcloud/service/lxd: GetCephInterfaces function
6a3ac62 
Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
microcloud/service/lxd: Add ValidateCephInterfaces
cbc8227 
This defines the validation rules for a Ceph subnet string given a set of physical network interfaces.

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
microcloud/service/microceph: Add the ClusterConfig function
f2064c9 
When a Ceph cluster is deployed, we need to be able to query its global configuration.
For example, we'll need this client logic to know if a deployed Ceph cluster has an existing
`cluster_network` parameter. This will be required for adding a new cluster member using the
dedicated Ceph subnet.

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
microcloud/cmd/microcloud: Add MicroCephInternalNetworkSubnet to In…
4051030 
…itSystem

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
microcloud/cmd/microcloud: fix comments format
7ba4acf 
Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
microcloud/cmd/microcloud: Ceph dedicated network interactive setup
c4ad62f 
Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
microcloud/cmd/microcloud: Preseed logic to setup a dedicated Ceph ne…
21144a7 
…twork

The added logic handle the bootstrapping and adding logic, when a
dedicated Ceph network has been required.

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
microcloud/cmd/microcloud: Apply Ceph dedicated network config to Mic…
40c4fd7 
…roCeph service

If a dedicated Ceph network has been required,
we choose this subnet for the `cluster` traffic.

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
doc: Add documentation on how to set up a dedicated Ceph cluster network
f60d9f3 
Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
microcloud/test: Add integration tests for Ceph networking
e25100b 
Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
Copy link
Contributor Author

@markylaing updated

markylaing
markylaing approved these changes Jun 11, 2024
View reviewed changes
Copy link
Contributor

@markylaing markylaing left a comment

Choose a reason for hiding this comment

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

LGTM thanks :)

masnax
masnax approved these changes Jun 11, 2024
View reviewed changes
Copy link
Contributor

@masnax masnax left a comment

Choose a reason for hiding this comment

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

We can finally put this one to rest :)

@masnax masnax merged commit 06766fe into canonical:main Jun 11, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ru-fu ru-fu ru-fu left review comments

@masnax masnax masnax approved these changes

@markylaing markylaing markylaing approved these changes

@roosterfish roosterfish Awaiting requested review from roosterfish

@sabaini sabaini Awaiting requested review from sabaini

@simondeziel simondeziel Awaiting requested review from simondeziel

@tomponline tomponline Awaiting requested review from tomponline

Assignees

@gabrielmougard gabrielmougard

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gabrielmougard @ru-fu @tomponline @simondeziel @masnax @sabaini @markylaing