Update port conflict checker to distinguish between conflicts and exact matches. #667
Conversation
|
"LGTM" (rookie). Does a similar change have to happen in network/port.go? |
|
No, network/port.go does not need to handle this - juju is moving from handling individual ports to operating on port ranges. Thanks for the review! |
| @@ -222,7 +225,7 @@ func (p *Ports) ClosePorts(portRange PortRange) error { | |||
| if existingPortsDef == portRange { | |||
| found = true | |||
| continue | |||
| } else if existingPortsDef.UnitName == portRange.UnitName && existingPortsDef.ConflictsWith(portRange) { | |||
| } else if existingPortsDef.UnitName == portRange.UnitName && existingPortsDef.CheckConflicts(portRange) == ErrPortsConflict { | |||
There was a problem hiding this comment.
This line is getting a bit too long for my taste, so how about:
} else if existingPortsDef.UnitName == portRange.UnitName &&
existingPortsDef.CheckConflicts(portRange) == ErrPortsConflict {
...
}
There was a problem hiding this comment.
you don't need the else
just open a new if condition here.
|
review done, sorry not lgtm yet. |
| } | ||
|
|
||
| return false | ||
| if a.ToPort >= b.FromPort && b.ToPort >= a.FromPort { |
There was a problem hiding this comment.
Just trying to understand what we're checking here:
a := PortRange{FromPort: 1000, ToPort: 2000}
b := PortRange{FromPort: 1999, ToPort: 1001}In the case above this if block will return ErrPortsConflict, but:
- We're not checking ToPort <= FromPort (and possibly swapping them if so)
- b as defined is not a valid range, but if b was {FromPort:1001, ToPort: 1999} (a valid range) ErrPortsConflict won't be returned, but it should as there is indeed conflict / overlap.
So I think there are about 10 cases to check:
- if a.FromPort > a.ToPort => "%d-%d is not a valid port range", a.FromPort, a.ToPort
- if b.FromPort > b.ToPort => "%d-%d is not a valid port range", b.FromPort, b.ToPort
- if a.FromPort == b.FromPort && a.ToPort == b.ToPort => ErrPortsMatch
- if a.FromPort < b.FromPort && a.ToPort > b.ToPort => ErrPortsConflict (a completely includes b)
- if b.FromPort < a.FromPort && b.ToPort > a.ToPort => ErrPortsConflict (b completely includes a)
- if a.FromPort < b.FromPort && a.ToPort < b.ToPort => ErrPortsConflict? (a overlaps partially with b)
- if b.FromPort < a.FromPort && b.ToPort < a.ToPort => ErrPortsConflict? (b overlaps partially with a)
- (only 2 cases left: a before b, b before a - in both cases no overlap or conflict) => nil
|
A few thoughts and suggestions. |
|
@dimitern, @davecheney - PTAL |
| "invalid port range", | ||
| state.PortRange{"wordpress/0", 100, 80, "TCP"}, | ||
| MustPortRange("wordpress/0", 80, 80, "TCP"), | ||
| "100-80/tcp is not a valid port range: invalid port range 100-80", |
There was a problem hiding this comment.
I'd rather see "invalid port range: 100-80/tcp" only here as an error, which probably means if Validate() returns and error, just return it as-is in CheckConflicts().
|
@dimitern, @davecheney - PTAL |
| if !ports.canOpenPorts(portRange) { | ||
| return nil, fmt.Errorf("cannot open ports %v on machine %v due to conflict", portRange, machineId) | ||
| if err := ports.canOpenPorts(portRange); err != nil { | ||
| return nil, fmt.Errorf("cannot open ports %v on machine %v: %v", portRange, machineId, err) |
There was a problem hiding this comment.
Why not use errors.Annotatef(err, "cannot open ports %v on machine %q", portRange, machineId) ?
|
@dimitern - thanks for the review! PTAL |
| expected: "mismatched port ranges 100-200/tcp and 100-300/tcp", | ||
| }, | ||
| } | ||
| expected: "mismatched port ranges 100-200/tcp and 100-300/tcp: port ranges 100-200/tcp \\(wordpress/0\\) and 100-300/tcp \\(wordpress/0\\) conflict", |
There was a problem hiding this comment.
This error message seems a bit too repetitive, I'd trim it like this "mismatched port ranges: 100-200/tcp (wordpress/0) and 100-300/tcp (wordpress/0) conflict", but this is just a suggestion, I can live with it.
|
Apart from possibly rephrasing a minor eye-poker error message, now it looks much better, thanks for bearing with me! LGTM |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Update port conflict checker to distinguish between conflicts and exact matches. This is a fix for #1359837 (https://bugs.launchpad.net/juju-core/+bug/1359837). Opening a port that is already open should not result in an error.
This is a fix for #1359837 (https://bugs.launchpad.net/juju-core/+bug/1359837).
Opening a port that is already open should not result in an error.