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

Convert sig_short_exp acceptance to use Bazel #3561

Merged
merged 4 commits into from
Dec 30, 2019
Merged

Convert sig_short_exp acceptance to use Bazel #3561

merged 4 commits into from
Dec 30, 2019

Conversation

scrye
Copy link
Contributor

@scrye scrye commented Dec 27, 2019

This reduces the worst case running time of the test from
about 8 minutes to about 30 seconds.

Also:

  • sig_short_exp no longer runs a control plane, instead
    using a fake sciond
  • removes the old sig_short_exp acceptance test from
    the main acceptance pipeline

This change is Reviewable

@scrye scrye requested a review from lukedirtwalker December 27, 2019 15:55
@scrye scrye self-assigned this Dec 27, 2019
Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 19 of 19 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @scrye)


acceptance/sig_short_exp_time/test, line 86 at r1 (raw file):


    echo "Shutting down path A"
    docker-compose -f acceptance/sig_short_exp_time/docker-compose.yml stop patha

Is that even required?


acceptance/sig_short_exp_time/testdata/1-ff00_0_110/sig/fake_sciond.json, line 21 at r1 (raw file):

                    "next_hop": "242.254.100.3:50000",
                    "ia": "1-ff00:0:111",
                    "expiry_seconds": 10

Shouldn't this be 5 here? since it's already 5 seconds in?
Ah no looking at the implementation it doesn't seem like it. Ok

Copy link
Contributor Author

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)


acceptance/sig_short_exp_time/test, line 86 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Is that even required?

I'd say it is, otherwise a SIG that doesn't never checks for expiry would pass the test.

This way, the test checks that the SIG switched ahead of time to path B due to path A's imminent expiration.


acceptance/sig_short_exp_time/testdata/1-ff00_0_110/sig/fake_sciond.json, line 21 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Shouldn't this be 5 here? since it's already 5 seconds in?
Ah no looking at the implementation it doesn't seem like it. Ok

I was fearing the semantics would be confusing. Hopefully the new field names are more intuitive.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @scrye)


acceptance/sig_short_exp_time/test, line 74 at r2 (raw file):


    # Set up forward route on network stack 1 and 2 through sig tunnel device
    # If this fails, the test is not stopped.

Maybe explain that the containers use the same network context thus the route is persisted.


acceptance/sig_short_exp_time/testdata/1-ff00_0_110/sig/fake_sciond.json, line 21 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

I was fearing the semantics would be confusing. Hopefully the new field names are more intuitive.

Seems more clear 👍

This reduces the worst case running time of the test from
about 8 minutes to about 30 seconds.

Also:
- sig_short_exp no longer runs a control plane, instead
using a fake sciond
- removes the old sig_short_exp acceptance test from
the main acceptance pipeline
Copy link
Contributor Author

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewable status: 18 of 19 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)


acceptance/sig_short_exp_time/test, line 74 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Maybe explain that the containers use the same network context thus the route is persisted.

Done.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@scrye scrye merged commit dc1c580 into scionproto:master Dec 30, 2019
@scrye scrye deleted the pubpr-make-sig-short-exp-fast branch December 30, 2019 12:20
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.

None yet

2 participants