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

Add service list and service add #260

Closed
wants to merge 8 commits into from
Closed

Conversation

masnax
Copy link
Contributor

@masnax masnax commented Feb 1, 2024

Depends on #259 and #287

Adds 2 new commands:

  • microcloud service list will list the cluster members for every installed service, or report if it is not initialized. This will effectively be the same as calling all of the following in succession:
lxc cluster list
microcloud cluster list
microceph cluster list
microovn cluster list

The information shown will be the name, address, dqlite role, and current status of each member.

  • microcloud service add will try to setup MicroOVN and MicroCeph on all existing MicroCloud cluster members, optionally setting up storage and networks for LXD. This is useful if MicroOVN or MicroCeph was at one point not installed on the systems and skipped during microcloud init. LXD and MicroCloud itself are required to already be set up.
    Thanks to Reuse existing MicroCeph and MicroOVN clusters #259 we can also try to re-use a service that partially covers existing MicroCloud cluster members. So if a MicroCloud is set up without MicroCeph, and then the user manually configures MicroCeph to partially cover the cluster, the user can then use microcloud service add to further configure MicroCeph to work with MicroCloud, and set up storage pools for LXD.

microcloud/service/microceph.go Fixed Show fixed Hide fixed
microcloud/service/microcloud.go Fixed Show fixed Hide fixed
microcloud/service/microovn.go Fixed Show fixed Hide fixed
client/proxy.go Dismissed Show dismissed Hide dismissed
@masnax
Copy link
Contributor Author

masnax commented May 30, 2024

@roosterfish @gabrielmougard @markylaing

This PR fails the tests because the microcluster dependency is mismatched between MicroCloud and MicroCeph/MicroOVN. This won't get resolved until the schema issues are fixed on the microcluster side, and the dependency is updated in MicroCeph/MicroOVN.

I thought that since it might take a while, and since the implementation here won't change in the mean time, it would be great if this PR could get some reviews while we wait for it to be unblocked, thanks.

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 good overall, I just have a couple of questions :)

err = lxdClient.UpdateProfile(profile.Name, profile.ProfilePut, "")
if err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If the cluster was set up initially with only local storage I'm guessing that when adding ceph afterwards this will overwrite the root disk device in the default profile to be Ceph instead?

I think this might be a bit confusing to users so I'm not sure about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I think we should either ask a question here, or only update the default profile if there is not already a corresponding entry.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - I guess asking a question makes sense since it's highly likely there will already be root disk and nic devices in the default profile.

api/services.go Outdated Show resolved Hide resolved
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

Looking very good, I have only a few smaller comments.

I saw #287 is also linked as a dependency but these changes already seem to be included in this PR. Am I right?

client/proxy.go Outdated Show resolved Hide resolved
service/microovn.go Outdated Show resolved Hide resolved
@masnax
Copy link
Contributor Author

masnax commented Jun 6, 2024

Looking very good, I have only a few smaller comments.

I saw #287 is also linked as a dependency but these changes already seem to be included in this PR. Am I right?

Yeah I've just rebased this PR off of that one.

@roosterfish
Copy link
Contributor

Yeah I've just rebased this PR off of that one.

Ok. Does this PR require a rebase of some sort? The pipeline doesn't look happy rn.

@masnax
Copy link
Contributor Author

masnax commented Jun 6, 2024

Yeah I've just rebased this PR off of that one.

Ok. Does this PR require a rebase of some sort? The pipeline doesn't look happy rn.

#260 (comment)

The tests won't be happy until MicroCeph & MicroOVN move to the same microcluster version as MicroCloud.

@roosterfish
Copy link
Contributor

The tests won't be happy until MicroCeph & MicroOVN move to the same microcluster version as MicroCloud.

My mistake, I have missed the comment. canonical/microcluster#132 is ready :)

To add a service, we need to talk to existing cluster members, so we can
use TLS authentication instead of the mDNS auth secret.

Signed-off-by: Max Asnaashari <[email protected]>
Rather than overwriting the default profile when adding devices, this
appends the devices and config to the default profile. When adding a new
service to microcloud that was skipped during initial setup, we may have
to add a new device to the default profile but we shouldn't remove
anything we added during first init.

Signed-off-by: Max Asnaashari <[email protected]>
Adds `microcloud service list` and `microcloud service add` commands.

`list` lists all cluster members for every installed and initialized
service.

`add` is used to add services that were skipped during initialization.
MicroCloud's direct cluster members will be compared against MicroCeph
and MicroOVN if installed, across all other systems in the cluster. If a
cluster exists, the user will be prompted to reuse it. By the end, all
cluster members in MicroCloud should also be present in MicroOVN and
MicroCeph if chosen. Additionally, if LXD storage pools and networks
were not set up initially, they can be set up now.

Signed-off-by: Max Asnaashari <[email protected]>
@masnax masnax force-pushed the add-service branch 2 times, most recently from cbb67a9 to f17b914 Compare June 25, 2024 21:03
@masnax
Copy link
Contributor Author

masnax commented Jun 25, 2024

@ru-fu I've added the associated documentation to the last commit of this PR, let me know if you have any feedback :)

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.

The docs look good as they are, but I think splitting the instructions into two will make them even clearer - what do you think?

@@ -0,0 +1,41 @@
.. _howto-add-service:

How to add a new service
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
How to add a new service
How to add a service

Sorry, pet peeve. ;)
How would you add an old service? 😜


If MicroCloud detects a service is installed but not set up, it will ask to configure the service.

#. Select whether you want to set up distributed storage (if adding MicroCeph to the MicroCloud).
Copy link
Contributor

Choose a reason for hiding this comment

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

This took me a bit to understand. I think it would be clearer to split the instructions into two parts, one for MicroCeph and one for MicroOVN. The drawback is that we don't have one full list of steps for running the tool, but I think it will still be easy to follow. And users who want to add only one service can skip the part for the other one without wondering if they are missing anything.

Suggested change
#. Select whether you want to set up distributed storage (if adding MicroCeph to the MicroCloud).
To add MicroCeph:
1. Select ``yes`` to set up distributed storage.

Comment on lines +14 to +20
.. note::
To set up distributed storage, you need at least three additional disks on at least three different machines.
The disks must not contain any partitions.

If you choose ``yes``, configure the distributed storage:

1. Select the disks that you want to use for distributed storage.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.. note::
To set up distributed storage, you need at least three additional disks on at least three different machines.
The disks must not contain any partitions.
If you choose ``yes``, configure the distributed storage:
1. Select the disks that you want to use for distributed storage.
.. note::
To set up distributed storage, you need at least three additional disks on at least three different machines.
The disks must not contain any partitions.
#. Select the disks that you want to use for distributed storage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to fix the indentation for the next steps accordingly.


#. You can choose to optionally set up a CephFS distributed file system.

#. Select either an IPv4 or IPv6 CIDR subnet for the Ceph internal traffic. You can leave it empty to use the default value, which is the MicroCloud internal network (see :ref:`howto-ceph-networking` for how to configure it).
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why this is not a substep to the first one? The question is asked only if you add MicroCeph, correct?

Comment on lines +30 to +34
#. Select whether you want to set up distributed networking (if adding MicroOVN to the MicroCloud).

If you choose ``yes``, configure the distributed networking:

1. Select the network interfaces that you want to use (see :ref:`microcloud-networking-uplink`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#. Select whether you want to set up distributed networking (if adding MicroOVN to the MicroCloud).
If you choose ``yes``, configure the distributed networking:
1. Select the network interfaces that you want to use (see :ref:`microcloud-networking-uplink`).
To add MicroOVN:
1. Select ``yes`` to set up distributed networking.
#. Select the network interfaces that you want to use (see :ref:`microcloud-networking-uplink`).

Copy link
Contributor

Choose a reason for hiding this comment

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

Plus indentation for the following steps.

Comment on lines +39 to +41
#. MicroCloud now starts to bootstrap the cluster for only the new services.
Monitor the output to see whether all steps complete successfully.
See :ref:`bootstrapping-process` for more information.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#. MicroCloud now starts to bootstrap the cluster for only the new services.
Monitor the output to see whether all steps complete successfully.
See :ref:`bootstrapping-process` for more information.
MicroCloud now starts to bootstrap the cluster for only the new services.
Monitor the output to see whether all steps complete successfully.
See :ref:`bootstrapping-process` for more information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants