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

LDMS Rail fixes #1430

Merged
merged 6 commits into from
Aug 20, 2024
Merged

LDMS Rail fixes #1430

merged 6 commits into from
Aug 20, 2024

Conversation

narategithub
Copy link
Collaborator

This pull request addresses the following issues:

  • Add the forgotten set_by_name() xprt operation.
  • The passive (accepted) legacy xprt was intentionally exposed to the application. It should be wrapped in rail object.
  • Fix set_delete notification code path to support rail.

The application should only interacts with the rail object. However, the
legacy connection from the remote peer slipped through and got exposed
to the application. This patch wraps the legacy connection xprt inside a
rail object.
@narategithub
Copy link
Collaborator Author

@nick-enoent I tested this but could you please include this in your test?

@nick-enoent
Copy link
Contributor

@nick-enoent I tested this but could you please include this in your test?

Sure thing, I'll give it a go today.

@nick-enoent
Copy link
Contributor

@narategithub I haven't found any issue with the rails fixes you implemented.

ldms/src/core/ldms_rail.c Show resolved Hide resolved
ldms/src/core/ldms_rail.c Outdated Show resolved Hide resolved
* Tell the peer that have an RBD for this set that it is being
* deleted. When they all reply, we can delete the set.
*/
void __rail_set_delete(ldms_t _r, struct ldms_set *s,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great if the function name format differs from the rails operation interfaces, e.g., __rail_listen().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

any suggestion? the '__' was just meant to mark private, in a sense that application should not call it (it is not exported in ldms.h ...).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with you that __rail_on_set_delete helps at least to me to know that it's that an operation.

@narategithub narategithub force-pushed the rail-fix branch 2 times, most recently from 8200bcc to 5bfc435 Compare August 19, 2024 14:51
} else {
/*
* This condition is only for `x` with legacy LDMS remote peer.
* I this case, we shall wrap `x` with a new rail before
Copy link
Collaborator

Choose a reason for hiding this comment

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

A typo: 'I' should read 'In'.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops. thanks.

ldms/src/core/ldms_rail.c Outdated Show resolved Hide resolved
* Tell the peer that have an RBD for this set that it is being
* deleted. When they all reply, we can delete the set.
*/
void __rail_set_delete(ldms_t _r, struct ldms_set *s,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with you that __rail_on_set_delete helps at least to me to know that it's that an operation.

@tom95858 tom95858 merged commit 0ea80ca into ovis-hpc:OVIS-4 Aug 20, 2024
14 checks passed
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