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

MCS: use cluster ID for ns lookup on exported service. #12064

Merged
merged 2 commits into from
Apr 11, 2023

Conversation

sp98
Copy link
Contributor

@sp98 sp98 commented Apr 11, 2023

Description of your changes:
In MCS, to access the Service in a specific cluster, prefix the query with cluster-id so that we only query for the exported service on the local cluster.

clusterid is a unique identity/name for a cluster and it should not overlap with any of the remote clusters in the clusterset as per the MCS API.

ClusterID should be provided by the user because there are multiple ways in which the clusterid can be configured as part of Submariner installation and assuming one could break the solution when deployed with a custom clusterid

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

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.

ClusterID uniquely identifies a cluster. It is used as a prefix to
nslookup exported services.
For example: <clusterid>.<svc>.<ns>.svc.clusterset.local

Signed-off-by: sp98 <sapillai@redhat.com>
use ClusterID of the k8s cluster while doing nslookup of the
exported mon or osd service IP

Signed-off-by: sp98 <sapillai@redhat.com>
@sp98 sp98 requested a review from travisn April 11, 2023 12:26
@sp98
Copy link
Contributor Author

sp98 commented Apr 11, 2023

testing:

kubectl get cephblockpools.ceph.rook.io mirrored-pool -n rook-ceph -o jsonpath='{.status.mirroringStatus.summary}'
{"daemon_health":"OK","health":"OK","image_health":"OK","states":{}}%

@@ -2207,6 +2207,10 @@ type MultiClusterServiceSpec struct {
// like Globalnet Submariner.
// +optional
Enabled bool `json:"enabled,omitempty"`

// ClusterID uniquely identifies a cluster. It is used as a prefix to nslookup exported
// services. For example: <clusterid>.<svc>.<ns>.svc.clusterset.local
Copy link
Member

Choose a reason for hiding this comment

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

How does the user know what to set the clusterID to? Would it be found in their kube context? And if the k8s dns lookup can work with it, isn't there a way rook can look up the cluster ID?

Copy link
Contributor Author

@sp98 sp98 Apr 11, 2023

Choose a reason for hiding this comment

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

Based on the discussion with Submariner team, the cluster ID can be from the kubeconfig but can also be configured by the user with something different. So we can't rely on kubeconfig every time.

The clusterID can be found in kubectl get submariners.submariner.io -n submariner-operator submariner -ojsonpath={.spec.clusterID}

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so the user must be aware of the clusterID and apply it the same both for submariner and in rook.

@sp98 sp98 requested a review from travisn April 11, 2023 14:50
@@ -75,7 +75,7 @@ func (c *Cluster) createService(mon *monConfig) (*v1.Service, error) {

func (c *Cluster) exportService(service *v1.Service, monDaemon string) (string, error) {
logger.Infof("exporting service %q", service.Name)
exportedIP, err := k8sutil.ExportService(c.ClusterInfo.Context, c.context, service)
exportedIP, err := k8sutil.ExportService(c.ClusterInfo.Context, c.context, service, c.spec.Network.MultiClusterService.ClusterID)
Copy link
Member

Choose a reason for hiding this comment

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

Where this is added for upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't get that question.

@@ -2207,6 +2207,10 @@ type MultiClusterServiceSpec struct {
// like Globalnet Submariner.
// +optional
Enabled bool `json:"enabled,omitempty"`

// ClusterID uniquely identifies a cluster. It is used as a prefix to nslookup exported
// services. For example: <clusterid>.<svc>.<ns>.svc.clusterset.local
Copy link
Member

Choose a reason for hiding this comment

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

Ok, so the user must be aware of the clusterID and apply it the same both for submariner and in rook.

@travisn travisn merged commit 85088e7 into rook:master Apr 11, 2023
45 of 50 checks passed
travisn added a commit that referenced this pull request Apr 11, 2023
MCS: use cluster ID for ns lookup on exported service. (backport #12064)
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.

MultiClusterService: Mon quorum fails on the peer cluster when mcs is enabled.
3 participants