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

Add sig_failover_acceptance test. #2272

Merged
merged 1 commit into from
Jan 4, 2019
Merged

Add sig_failover_acceptance test. #2272

merged 1 commit into from
Jan 4, 2019

Conversation

sustrik
Copy link
Contributor

@sustrik sustrik commented Dec 21, 2018

Fixes #2186


This change is Reviewable

Copy link
Contributor

@worxli worxli left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @sustrik)


acceptance/sig_failover_acceptance/test, line 17 at r1 (raw file):

    ./tools/dc stop scion_br1-ff00_0_110-2
    sleep 10
    # Make sure that the ping stills get through.

typo stills get -> still gets

Copy link
Contributor

@worxli worxli 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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @sustrik)


acceptance/sig_failover_acceptance/test, line 16 at r1 (raw file):

    # Disable one of the two paths betweek the ASes by stopping the corresponding BR.
    ./tools/dc stop scion_br1-ff00_0_110-2
    sleep 10

Are the sleeps really needed?

Copy link
Contributor Author

@sustrik sustrik 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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @worxli)


acceptance/sig_failover_acceptance/test, line 16 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

Are the sleeps really needed?

It works even without the sleeping at the moment. However, my understanding is that once we get rid of reliable sockets the pings may just disappear before the new path is set up. @scrye: can you confirm?


acceptance/sig_failover_acceptance/test, line 17 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

typo stills get -> still gets

Done.

Copy link
Contributor

@worxli worxli 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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @sustrik and @worxli)


acceptance/sig_failover_acceptance/test, line 17 at r1 (raw file):

ake sure that the ping still get through

gets

Copy link
Contributor

@worxli worxli 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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @sustrik and @worxli)


acceptance/sig_failover_acceptance/test, line 17 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…
ake sure that the ping still get through

gets

or pings for that matter

Copy link
Contributor

@worxli worxli 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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @sustrik)


acceptance/sig_failover_acceptance/test, line 16 at r1 (raw file):

Previously, sustrik (Martin Sustrik) wrote…

It works even without the sleeping at the moment. However, my understanding is that once we get rid of reliable sockets the pings may just disappear before the new path is set up. @scrye: can you confirm?

But isn't the idea that the failover should be quick enough for no ping to get lost?

Copy link
Contributor Author

@sustrik sustrik 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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @worxli)


acceptance/sig_failover_acceptance/test, line 16 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

But isn't the idea that the failover should be quick enough for no ping to get lost?

The timout period in sig is 1 second. I've lowered to sleep period to 3 seconds.


acceptance/sig_failover_acceptance/test, line 17 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

or pings for that matter

Fixed.

Copy link
Contributor

@worxli worxli 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: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @sustrik)


acceptance/sig_failover_acceptance/test, line 16 at r1 (raw file):

sig_ping_acceptance

sig_ping_acceptance by default takes 5 attempts. I don't think the sleep is necessary unless you specify -attempts 1.

Copy link
Contributor Author

@sustrik sustrik 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: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @worxli)


acceptance/sig_failover_acceptance/test, line 16 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…
sig_ping_acceptance

sig_ping_acceptance by default takes 5 attempts. I don't think the sleep is necessary unless you specify -attempts 1.

Done.

Copy link
Contributor

@worxli worxli left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@worxli worxli 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 @sustrik)


acceptance/sig_failover_acceptance/test, line 2 at r2 (raw file):

#!/bin/bash

can you please add a line with a description of what this tests?

Copy link
Contributor

@worxli worxli 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, 2 unresolved discussions (waiting on @sustrik)


acceptance/sig_failover_acceptance/test, line 14 at r2 (raw file):

    # Make sure that pings go through in a vanilla setup.
    ./bin/sig_ping_acceptance -d -log.console info
    # Disable one of the two paths betweek the ASes by stopping the corresponding BR.

between

Copy link
Contributor Author

@sustrik sustrik 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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @worxli)


acceptance/sig_failover_acceptance/test, line 2 at r2 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

can you please add a line with a description of what this tests?

Done.


acceptance/sig_failover_acceptance/test, line 14 at r2 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

between

Done.

Copy link
Contributor

@worxli worxli left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@worxli worxli left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@sustrik sustrik merged commit 5db19d0 into scionproto:master Jan 4, 2019
@sustrik sustrik deleted the sig-failover branch January 8, 2019 11:12
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.

2 participants