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

external: enable the use of only v2 mon port #13856

Merged
merged 1 commit into from Mar 4, 2024

Conversation

parth-gr
Copy link
Member

@parth-gr parth-gr commented Mar 4, 2024

currently the script requires to have both v2 and v1 port to enable v2 port, but that is not the necessary condition, so removing the chek, and enabling it only v2 is present to successfully configure with v2 only

part-of: #13827

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • 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.

currently  the script requires to have both v2 and v1 port
to enable v2 port, but that is not the necessary condition,
so removing the chek, and enabling it only v2 is present to
successfully configure with v2 only

part-of: rook#13827

Signed-off-by: parth-gr <partharora1010@gmail.com>
Copy link
Contributor

@subhamkrai subhamkrai left a comment

Choose a reason for hiding this comment

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

I'm sure v2 port doesnt' require v1 port but is there any doc or something to confirm this?

@bauerjs1
Copy link

bauerjs1 commented Mar 4, 2024

@subhamkrai I am not sure if this is what you're looking for but this section suggests that it should be possible. With encryption or compression enabled, it is actually a requirement for external clusters to work with v2 only.

@parth-gr
Copy link
Member Author

parth-gr commented Mar 4, 2024

@subhamkrai but i think thats a nice point, and i can update that with a new flag if someone wants to use the encryption and can also document that

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.

Looks good to remove this requirement, no need for the v1 port to be enabled.

@travisn travisn merged commit e88f28e into rook:master Mar 4, 2024
51 checks passed
travisn added a commit that referenced this pull request Mar 4, 2024
external: enable the use of only v2 mon port (backport #13856)
@bauerjs1
Copy link

The script isn't failing anymore, however, I am still getting an error/warning:

'v2' address type not present, and 'v2-port-enable' flag is provided

After a lot of digging, it seems to me like this check does not make sense, because it assumes that a MON with only a single entry in the addrvec (from ceph quorum_status) has only v1 port enabled. However, in reality it can also have only v2 enabled.

@parth-gr
Copy link
Member Author

The script isn't failing anymore, however, I am still getting an error/warning:

'v2' address type not present, and 'v2-port-enable' flag is provided

After a lot of digging, it seems to me like this check does not make sense, because it assumes that a MON with only a single entry in the addrvec (from ceph quorum_status) has only v1 port enabled. However, in reality it can also have only v2 enabled.

But it make sense if we have both the entries

@bauerjs1
Copy link

bauerjs1 commented Mar 22, 2024

True! I correct myself: The mentioned check doesn't make sense in case there is only a v2 port. However, that is one of the possible cases that need to be handeled here.

The assumption behind this conditional (if I understand the code correctly) is "if there is only one port, then it's v1" but that is not always true.

@travisn
Copy link
Member

travisn commented Mar 26, 2024

Agreed, the v2 port could be the only one specified. @parth-gr Could you fix that check so that it doesn't require two endpoints in the list? The v2 port might be the only port.

@parth-gr
Copy link
Member Author

Sure it make sense added a PR #13982

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