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

ospfd: fix internal ldp-sync state flags when feature is disabled #16376

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

c-po
Copy link
Contributor

@c-po c-po commented Jul 13, 2024

When enabling "mpls ldp-sync" under "router ospf" ospfd configures SET_FLAG(ldp_sync_info->flags, LDP_SYNC_FLAG_IF_CONFIG) so internally knowing that the ldp-sync feature is enabled. However the flag is not cleared when turning of the feature using "nompls ldp-sync"!

Closes #16375

Please backup this to stable/10.0 and stable/9.1

Copy link
Contributor

@mjstapp mjstapp left a comment

Choose a reason for hiding this comment

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

thanks, looks good

@mjstapp
Copy link
Contributor

mjstapp commented Jul 15, 2024

@Mergifyio backport dev/10.1 stable/10.0 stable/9.1 stable/9.0

Copy link

mergify bot commented Jul 15, 2024

backport dev/10.1 stable/10.0 stable/9.1 stable/9.0

✅ Backports have been created

@mjstapp
Copy link
Contributor

mjstapp commented Jul 15, 2024

Looks like there are CI test problems, some look suspicious. Please look into the test results.

@c-po c-po force-pushed the ospfd-ldp-sync branch from 89160af to 6bd09ca Compare July 16, 2024 06:39
@c-po
Copy link
Contributor Author

c-po commented Jul 16, 2024

Rebased on current master so Topotests can re-run. I see no issues with the tests. Local run shows: 443 passed, 3 skipped in 9.41s

@mjstapp
Copy link
Contributor

mjstapp commented Jul 16, 2024

Please look at the failures in the "checks" tab - there are errors reported in ldp-sync topotests, it appears.

Rebased on current master so Topotests can re-run. I see no issues with the tests. Local run shows: 443 passed, 3 skipped in 9.41s

Copy link
Member

@riw777 riw777 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

@riw777
Copy link
Member

riw777 commented Jul 16, 2024

failure looks related :-(

rname = 'r2', command = 'show ip ospf mpls ldp-sync json'
reference = 'show_ospf_ldp_sync.ref'

    def router_compare_json_output(rname, command, reference):
        "Compare router JSON output"
    
        logger.info('Comparing router "%s" "%s" output', rname, command)
    
        tgen = get_topogen()
        filename = "{}/{}/{}".format(CWD, rname, reference)
        expected = json.loads(open(filename).read())
    
        # Run test function until we get an result.
        test_func = partial(topotest.router_json_cmp, tgen.gears[rname], command, expected)
        _, diff = topotest.run_and_expect(test_func, None, count=320, wait=0.5)
        assertmsg = '"{}" JSON output mismatches the expected result'.format(rname)
>       assert diff is None, assertmsg
E       AssertionError: "r2" JSON output mismatches the expected result
E       assert Generated JSON diff error report:
E         
E         > $->r2-eth2->ldpIgpSyncEnabled: output has element with value 'True' but in expected it has value 'False'
E         > $->r2-eth2->ldpIgpSyncState: output has element with value 'Sync achieved' but in expected it has value 'Sync not required'

When enabling "mpls ldp-sync" under "router ospf" ospfd configures
SET_FLAG(ldp_sync_info->flags, LDP_SYNC_FLAG_IF_CONFIG) so internally knowing
that the ldp-sync feature is enabled. However the flag is not cleared when
turning of the feature using "nompls ldp-sync"!

FRRouting#16375

Signed-off-by: Christian Breunig <[email protected]>
@c-po c-po force-pushed the ospfd-ldp-sync branch from 6bd09ca to 5a70378 Compare July 17, 2024 08:31
@github-actions github-actions bot added size/S and removed size/XS labels Jul 17, 2024
@c-po
Copy link
Contributor Author

c-po commented Jul 17, 2024

failure looks related :-(

Topotests adjusted

[email protected]:~/frr/tests/topotests$ sudo -E pytest ldp_sync_ospf_topo1
==================================================================== test session starts ====================================================================
platform linux -- Python 3.11.2, pytest-7.2.1, pluggy-1.0.0+repack
rootdir: /home/cpo/frr/tests/topotests, configfile: pytest.ini
collected 14 items

ldp_sync_ospf_topo1/test_ldp_sync_ospf_topo1.py .............s                                                                                        [100%]

----------------------------------------------------- generated xml file: /tmp/topotests/topotests.xml ------------------------------------------------------
========================================================= 13 passed, 1 skipped in 100.13s (0:01:40) =========================================================

@c-po
Copy link
Contributor Author

c-po commented Jul 20, 2024

Anything missing on my side to continue with the PR?

@riw777 riw777 merged commit fba472e into FRRouting:master Jul 23, 2024
11 checks passed
riw777 added a commit that referenced this pull request Jul 23, 2024
ospfd: fix internal ldp-sync state flags when feature is disabled (backport #16376)
riw777 added a commit that referenced this pull request Jul 23, 2024
ospfd: fix internal ldp-sync state flags when feature is disabled (backport #16376)
riw777 added a commit that referenced this pull request Jul 23, 2024
ospfd: fix internal ldp-sync state flags when feature is disabled (backport #16376)
riw777 added a commit that referenced this pull request Jul 24, 2024
ospfd: fix internal ldp-sync state flags when feature is disabled (backport #16376)
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.

ospfd: mpls ldp-sync flag is not cleared if ldp-sync is disabled
3 participants