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

mds: support mds with multus #11611

Merged
merged 1 commit into from
Feb 8, 2023
Merged

mds: support mds with multus #11611

merged 1 commit into from
Feb 8, 2023

Conversation

subhamkrai
Copy link
Contributor

@subhamkrai subhamkrai commented Feb 3, 2023

to support mds with multus, we don't need
to add --public-adr, if multus is selected as
the network in cephCluster CR.

Signed-off-by: subhamkrai srai@redhat.com

Description of your changes:

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.

@subhamkrai
Copy link
Contributor Author

testing result

mdsMap

cat mdsmap 
epoch 7
root 0
max_mds 1
session_timeout 60
session_autoclose 300
        mds0    (1)192.168.20.12:6801   (up:active)

network attachment

apiVersion: k8s.cni.cncf.io/v1
kind: NetworkAttachmentDefinition
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"k8s.cni.cncf.io/v1","kind":"NetworkAttachmentDefinition","metadata":{"annotations":{},"labels":null,"name":"public-net","namespace":"rook-ceph"},"spec":{"config":"{ \"cniVersion\": \"0.3.0\", \"type\": \"macvlan\", \"master\": \"eth0\", \"mode\": \"bridge\", \"ipam\": { \"type\": \"whereabouts\", \"range\": \"192.168.20.0/24\" } }"}}
  creationTimestamp: "2023-02-03T03:43:20Z"
  generation: 1
  name: public-net
  namespace: rook-ceph
  resourceVersion: "1246"
  uid: 5d84b2cb-0baa-4683-9362-c46a3e23e265
spec:
  config: '{ "cniVersion": "0.3.0", "type": "macvlan", "master": "eth0", "mode": "bridge",
    "ipam": { "type": "whereabouts", "range": "192.168.20.0/24" } }'

we can see in mdsmap IP range is the same as the network attachment.

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

LGTM and the multus test passed, so the CI failures look like the known issues.

@parth-gr
Copy link
Member

parth-gr commented Feb 6, 2023

to support mds with multus, we need don't need
to add --public-adr is multus is selected as
a network in cephCluster CR network.

Can we update commit message to

to support mds with multus, we don't need
to add `--public-adr`, if multus is selected as
the network in cephCluster CR.

@@ -132,7 +132,7 @@ func (c *Cluster) makeMdsDaemonContainer(mdsConfig *mdsConfig) v1.Container {
"--foreground",
)

if !c.clusterSpec.Network.IsHost() {
if !c.clusterSpec.Network.IsHost() && !c.clusterSpec.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.

maybe a comment to it why the --pubic-addr is not required

Copy link
Contributor Author

@subhamkrai subhamkrai Feb 6, 2023

Choose a reason for hiding this comment

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

Do you want to a specific message? since I think the condition is readable but I'm happy to add some message

Copy link
Member

@parth-gr parth-gr Feb 7, 2023

Choose a reason for hiding this comment

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

Yaa condition looks fine,
I mean to tell the developer why we need that

to support mds with multus, we don't need
to add `--public-adr`, if multus is selected as
the network in cephCluster CR.

Signed-off-by: subhamkrai <srai@redhat.com>
@subhamkrai
Copy link
Contributor Author

to support mds with multus, we need don't need
to add --public-adr is multus is selected as
a network in cephCluster CR network.

Can we update commit message to

to support mds with multus, we don't need
to add `--public-adr`, if multus is selected as
the network in cephCluster CR.

updated

@BlaineEXE
Copy link
Member

Many failing tests last run. Re-running.

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.

requesting changes until CI passes

@subhamkrai
Copy link
Contributor Author

requesting changes until CI passes

@BlaineEXE this are know ci errors, we have some issue in ci from past week or so. Discussion going on #11600

@BlaineEXE BlaineEXE merged commit fb2dcb5 into rook:master Feb 8, 2023
@subhamkrai subhamkrai deleted the mds-multus branch February 8, 2023 16:12
travisn added a commit that referenced this pull request Feb 8, 2023
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

4 participants