feature(no_network) Add an option to disable any network activity on the NIC #128
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…the NIC * Add NoNetwork option that prevent any Gratuitous ARP burst and the addition/removal of the IP on the NIC * Add the update command on the link client that let's user customize VIP options This is a first step toward #97 a next step would be to implements hooks on the FAILING, STANDBY and ACTIVATED state
EtienneM
reviewed
Jan 6, 2022
| Flags: []cli.Flag{ | ||
| cli.BoolFlag{ | ||
| Name: "no-network", | ||
| Usage: "Disable any operation on the network interface", |
There was a problem hiding this comment.
I find this option so counterintuitive that I would add in the usage an example of why one would want to call it. WDYT?
| cli.BoolFlag{ | ||
| Name: "no-network", | ||
| Usage: "Disable any operation on the network interface", | ||
| }, | ||
| }, | ||
| Action: func(c *cli.Context) error { | ||
| if c.NArg()%2 == 0 { |
There was a problem hiding this comment.
Is this condition still working with the flag no-network?
Comment on lines
+185
to
+194
| var noNetwork bool | ||
| if c.Bool("no-network") { | ||
| noNetwork = true | ||
| updateParams.NoNetwork = &noNetwork | ||
| } | ||
|
|
||
| if c.Bool("network") { | ||
| noNetwork = false | ||
| updateParams.NoNetwork = &noNetwork | ||
| } |
There was a problem hiding this comment.
nitpick: I don't think you need to declare it outside the conditions:
Suggested change
| var noNetwork bool | |
| if c.Bool("no-network") { | |
| noNetwork = true | |
| updateParams.NoNetwork = &noNetwork | |
| } | |
| if c.Bool("network") { | |
| noNetwork = false | |
| updateParams.NoNetwork = &noNetwork | |
| } | |
| if c.Bool("no-network") { | |
| noNetwork := true | |
| updateParams.NoNetwork = &noNetwork | |
| } | |
| if c.Bool("network") { | |
| noNetwork := false | |
| updateParams.NoNetwork = &noNetwork | |
| } |
| @@ -27,12 +27,14 @@ type Manager interface { | |||
| Status() string | |||
| IP() models.IP | |||
| SetHealthchecks(context.Context, config.Config, []models.Healthcheck) | |||
| UpdateIP(ctx context.Context, ip models.IP) | |||
There was a problem hiding this comment.
nitpick: like for the other methods, don't name the arguments if it doesn't add any value:
Suggested change
| UpdateIP(ctx context.Context, ip models.IP) | |
| UpdateIP(context.Context, models.IP) |
| log.Info("NoNetwork option enabled: Removing IP from network interface") | ||
| err := m.networkInterface.RemoveIP(ip.IP) | ||
| if err != nil { | ||
| log.WithError(err).Error("Fail to remove IP from network interface") |
There was a problem hiding this comment.
Can you explain in a comment why we don't return in case of error here? This is unclear for me
| log.Info("NoNetwork option disabled and we are MASTER: Adding the IP to our network interface") | ||
| err := m.networkInterface.EnsureIP(ip.IP) | ||
| if err != nil { | ||
| log.WithError(err).Error("fail to add IP to our network interface") |
There was a problem hiding this comment.
Can you explain in a comment why we don't return in case of error here? This is unclear for me
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a first step toward #97 a next step would be to implements hooks on the FAILING, STANDBY and ACTIVATED state