Skip to content

Conversation

kgiusti
Copy link
Contributor

@kgiusti kgiusti commented Mar 10, 2025

No description provided.

@kgiusti kgiusti linked an issue Mar 10, 2025 that may be closed by this pull request
Copy link

codecov bot commented Mar 10, 2025

Codecov Report

Attention: Patch coverage is 54.71698% with 120 lines in your changes missing coverage. Please review.

Project coverage is 66.2%. Comparing base (745ca5b) to head (bef72e2).
Report is 26 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1755     +/-   ##
=======================================
- Coverage   67.9%   66.2%   -1.7%     
=======================================
  Files        202     205      +3     
  Lines      47767   48037    +270     
  Branches    5184    5217     +33     
=======================================
- Hits       32457   31840    -617     
- Misses     13198   14221   +1023     
+ Partials    2112    1976    -136     
Flag Coverage Δ
pysystemtests 78.5% <ø> (-0.6%) ⬇️
systemtests 59.3% <54.7%> (-2.3%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
unittests 66.2% <68.0%> (-1.7%) ⬇️
systemtests 66.2% <68.0%> (-1.7%) ⬇️
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kgiusti kgiusti force-pushed the ISSUE-1748 branch 3 times, most recently from aefb2f7 to e93e174 Compare March 19, 2025 17:10
@kgiusti kgiusti marked this pull request as ready for review March 19, 2025 17:20
if (strcmp(core->group_correlator_by_maskbit[mask_bit], correlator) == 0) {
qdr_connection_t *old_conn = core->rnode_conns_by_mask_bit[mask_bit];
qd_log(LOG_ROUTER_CORE, QD_LOG_ERROR,
"CGROUP duplicate group control connection detected: correlator=%s conns=[C%"PRIu64"],[C%"PRIu64"]",
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace CGROUP with Connection Group

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch - fixed.

correlator, member->identity, conn->identity);
DEQ_REMOVE_N(GROUP, core->unallocated_group_members, member);
DEQ_INSERT_HEAD_N(GROUP, conn->connection_group, member);
member->group_parent_mask_bit = conn->mask_bit;
Copy link
Contributor

Choose a reason for hiding this comment

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

group_parent_mask_bit is an excellent /easy to understand name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why thank you! :)

//
if (!!conn->control_links[QD_INCOMING] && !!conn->control_links[QD_OUTGOING]) {
//
// If this connection is pending upgrade we can now to the switchover.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// If this connection is pending upgrade we can now to the switchover.
// If this connection is pending upgrade we can now do the switchover.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

conns = self.management.query(type=CONNECTION_TYPE).get_dicts()
return [c for c in conns if 'inter-router' in c['role']]

def get_inter_router_data_conns(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like some of these functions are generic enough to be added to system_test.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are in system_test.py, right? I remembered you made a similar request on the previous patch so I move all the router-specific accessors into the QdRouter class.

The only two helper functions I left in the certificate rotation testcase are wait_inter_router_conns() and wait_control_links(). wait_inter_router_conns() invokes self.assertTrue() which means it needs to be in a TestCase subclass, and wait_control_link() has some very test-specific constraints which I think would make it error-prone to use generally.

kgiusti added 2 commits March 21, 2025 08:59
…grade

This introduces the concept of a connection group ordinal. It
annotates inter-router connections. Its purpose is to assign a
precedence to connections within and inter-router connection
group. Those inter router connections with the highest ordinal become
the active routing path to a peer router. Connections within the group
that have a lower ordinal remain active but no new flows are sent over
them.

This is used to implement connection upgrade for certificate rotation.
@kgiusti kgiusti merged commit 47628d6 into skupperproject:main Mar 31, 2025
50 of 52 checks passed
@kgiusti kgiusti deleted the ISSUE-1748 branch March 31, 2025 15:14
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.

Seemless upgrade of inter-router connection group
2 participants