Skip to content

Fixes #1793: Move the LINK vflow record from the connector object to … #1794

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

Merged
merged 1 commit into from
Jun 16, 2025

Conversation

ganeshmurthy
Copy link
Contributor

…connector_config object. This ensures

that only one LINK record exists per logical link

vflow_set_timestamp_now(connector->vflow_record, VFLOW_ATTRIBUTE_DOWN_TIMESTAMP);
vflow_set_string(connector->vflow_record, VFLOW_ATTRIBUTE_RESULT, condition_name ? condition_name : "unknown");
vflow_set_string(connector->vflow_record, VFLOW_ATTRIBUTE_REASON, condition_description ? condition_description : "");
qd_connector_config_t *ctor_config = connector->ctor_config;
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that the loss of the "old" control connection might be registered here and set the OPER_STATUS to "down" even though the "new" control connection is up?

Copy link
Contributor Author

@ganeshmurthy ganeshmurthy Jun 4, 2025

Choose a reason for hiding this comment

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

@ted-ross I made a small addition to the if statement below to compare the tls ordinals to see if the connector's tls ordinal matches the latest tls ordinal from the connector config before setting the oper status to down. Please let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, I am writing a test for this as well.

Copy link
Member

Choose a reason for hiding this comment

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

This seems like a clever solution. The only question I have is whether this breaks when the link doesn't use TLS (i.e. there's no SslProfile). However, I don't think a connector would ever be rotated if there wasn't an SslProfile, so this is probably fine.

@ganeshmurthy ganeshmurthy force-pushed the ISSUE-1793 branch 3 times, most recently from aae5d4b to 89b90ad Compare June 9, 2025 13:53
@ganeshmurthy ganeshmurthy self-assigned this Jun 9, 2025
@ganeshmurthy ganeshmurthy requested a review from c-kruse June 9, 2025 17:03
Copy link
Contributor

@mgoulish mgoulish left a comment

Choose a reason for hiding this comment

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

OK! Everything looks reasonable and straightforward -- I don't yet understand why this fixes the bug, but maybe I can get you to explain later.

Looks good!

connector->qd_conn = ctx;
qd_conn->connector = connector;
connector->qd_conn = qd_conn;
if (!connector->is_data_connector) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this mean that it is not a data connector? Could we have a comment to explain?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. It means that it's a control connector.

Never mind.

Copy link

@c-kruse c-kruse left a comment

Choose a reason for hiding this comment

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

Verified this fixes the bug!

…tor object to connector_config object. This ensures

that only one LINK record exists per logical link

Signed-off-by: Ganesh Murthy       <[email protected]>
@ganeshmurthy ganeshmurthy merged commit 9c01acd into skupperproject:main Jun 16, 2025
52 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