-
Notifications
You must be signed in to change notification settings - Fork 503
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
refactor(ports): refactor port service state #18147
base: main
Are you sure you want to change the base?
Conversation
ce0d590
to
add4dc1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some suggestion. My general feeling is that it could be more comprehensive by chosing better name.
Moreover, I think we should have a special focus on the wildcard reconciliation, which is complicated. However I don't have a strong conviction that it may be improved outside my few comments.
My feeling is that it is made utterly complicated by mixing both open and close ports in UpdateUnitPort (amho, we should have 3 function, on which open ports, one which close ports, and one which "update ports", meaning, force all ports to follow whatever configuration passed (it will actually open and close ports, but we won't need such differenciation about what we are closing and what we are opening, because it would be "closed unless updated to be open", with only one map of portrange).
) | ||
|
||
//go:generate go run go.uber.org/mock/mockgen -typed -package service -destination package_mock_test.go github.com/juju/juju/domain/port/service State | ||
|
||
var _ State = (*portstate.State)(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: I was wondering how to assert that one without introducing a dependency between service and domain. Putting it as a test seems a good idea !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this.
State should not know about service and service should not know about state.
domain/port/service/service.go
Outdated
GetOpenedEndpointPorts(ctx domain.AtomicContext, unitUUID, endpoint string) ([]network.PortRange, error) | ||
// GetUnitOpenedPortsUUID returns the opened ports for the given unit with the | ||
// UUID of the port range. The opened ports are grouped by endpoint. | ||
GetUnitOpenedPortsUUID(ctx domain.AtomicContext, unitUUID string) (map[string][]port.PortRangeUUID, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: For me it is not spelled correcty. Should be GetUnitOpenedPortUUIDs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some extra clarity could be useful here. PortRangeUUID sounds like the uuid of a port range, not the port range and uuid.
So I've gone for GetUnitOpenedPortsWithUUIDs
domain/port/service/service.go
Outdated
wildcardOpen, _ := openPorts[WildcardEndpoint] | ||
wildcardClose, _ := closePorts[WildcardEndpoint] | ||
|
||
wildcardOpenedSet := map[network.PortRange]bool{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Is it possible to have both wildcardOpen != nil
and wildcardClose != nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically yes. However, in reality, it is probably never going to happen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmmh. It is why it is never tested, i suppose :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make sure to add one now
domain/port/service/service.go
Outdated
wildcardClose, _ := closePorts[WildcardEndpoint] | ||
|
||
wildcardOpened, err := s.st.GetOpenedEndpointPorts(ctx, unitUUID, WildcardEndpoint) | ||
endpoints, err := s.ensureEndpoints(ctx, unitUUID, openPorts, closePorts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I don't think ensureEndpoints is a good abstraction, because I need to check the implementation or the doc to understand what is going on.
An helper function func collectEndpoint(portRanges ... network.GroupedPortRanges) []string
and a method func (s *Service) addMissingEndpoint(ctx domain.AtomicContext, endpointNames []string)
would be more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right this name is a little misleading, but I don't think your suggestion quite solves the problem.
I've gone for something slightly different to your suggestion, as we need to get the uuids of all endpoints, both existing and new here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still understanding this, but here is a first pass.
domain/port/service/service.go
Outdated
UpdateUnitPorts(ctx domain.AtomicContext, unitUUID string, openPorts, closePorts network.GroupedPortRanges) error | ||
// AddOpenedPorts adds the given port ranges to the database. Port ranges must | ||
// be grouped by endpoint UUID. | ||
AddOpenedPorts(ctx domain.AtomicContext, portRangesByEndpointUUID network.GroupedPortRanges) error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't pass core types to state. The types should be serialised to domain level values. These values can live in domain/port/types.go
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignoring for now after chatting with @SimonRichardson. These types should be moved out of core into domain/ports/types.go
, but doesn't need to be changed now.
domain/port/service/service.go
Outdated
func (s *Service) verifyNoColocatedPortRangeConflicts( | ||
ctx domain.AtomicContext, unitUUID string, portRanges []network.PortRange, | ||
) error { | ||
colocatedOpened, err := s.st.GetColocatedOpenedPorts(ctx, unitUUID) | ||
if err != nil { | ||
return errors.Errorf("failed to get opened ports co-located with unit %s: %w", unitUUID, err) | ||
} | ||
return verifyNoPortRangeConflicts(portRanges, colocatedOpened) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the fact that a service method knows about an atomic context. I'm ok with functions knowing about these things, but not methods.
Also, I'm unsure why the error can't be returned at the state layer.
domain/port/service/service.go
Outdated
} | ||
for i, portRange := range endpointOpenPorts { | ||
if _, ok := wildcardOpenedSet[portRange]; ok { | ||
openPorts[endpoint] = append(openPorts[endpoint][:i], openPorts[endpoint][i+1:]...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm more of a fan of creating a new type of things you want to keep, rather than modify out what you don't want to keep, esp. because you don't own that original type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be honest, there was a lot more clarity when it was in state.
The initial implementation of the ports domain included far too much business logic in the state layer. This was made particularly bad given the usage of RunAtomic. Hoist all of this business logic out of the state layer and into the service layer. Do this by re-writing the domain
You seem to have agreed with some of the review comments, and resolved the conversations, but not made the associated changes. |
A new fixup commit is on it's way. Admittedly, resolving them may have been premature |
Agree. It is necessarily complex unfortunately. I have gone through an made comments more detailed.
I'm not convinced this will make things any simpler. Unfortunately, as a result of wildcard reconciliation, a simple operation which just opens XOR closes some port ranges, may end up opening and closing some port ranges. As such, these proposed methods would be very similar, so the simplest implementation would probably see all methods calling out to a common private generic handler method early on |
add4dc1
to
9d8a17b
Compare
I have just appended a new commit responding to your comments. However, we may need to new approach entirely. So this change is being put on hold until then |
9d8a17b
to
3de8817
Compare
Another change: Before this PR, if you open and close the same port range on an endpoint, in Mongo these operations cancel out. However, it does not in DQLite. Remember to evaluate this case when re-evaluating the domain with RunAtomic & all that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another pass.
) | ||
|
||
//go:generate go run go.uber.org/mock/mockgen -typed -package service -destination package_mock_test.go github.com/juju/juju/domain/port/service State | ||
|
||
var _ State = (*portstate.State)(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this.
State should not know about service and service should not know about state.
// set of ports are present for the given unit. Returns all endpoints for the unit | ||
// after ensuring after adding any. | ||
func (s *Service) addMissingEndpoints( | ||
ctx domain.AtomicContext, unitUUID unit.UUID, currentEndpoints []port.Endpoint, openPorts, closePorts network.GroupedPortRanges, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctx domain.AtomicContext, unitUUID unit.UUID, currentEndpoints []port.Endpoint, openPorts, closePorts network.GroupedPortRanges, | |
ctx domain.AtomicContext, | |
unitUUID unit.UUID, | |
currentEndpoints []port.Endpoint, | |
openPorts, closePorts network.GroupedPortRanges, |
if len(newEndpointNames) > 0 { | ||
var err error | ||
newEndpoints, err = s.st.AddEndpoints(ctx, unitUUID, newEndpointNames) | ||
if err != nil { | ||
return nil, errors.Errorf("failed to add endpoints for unit %s: %w", unitUUID, err) | ||
} | ||
} | ||
return newEndpoints, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exit out early.
if len(newEndpointNames) > 0 { | |
var err error | |
newEndpoints, err = s.st.AddEndpoints(ctx, unitUUID, newEndpointNames) | |
if err != nil { | |
return nil, errors.Errorf("failed to add endpoints for unit %s: %w", unitUUID, err) | |
} | |
} | |
return newEndpoints, nil | |
if len(newEndpointNames) == 0 { | |
return newEndpoints, nil | |
} | |
var err error | |
newEndpoints, err = s.st.AddEndpoints(ctx, unitUUID, newEndpointNames) | |
if err != nil { | |
return nil, errors.Errorf("failed to add endpoints for unit %s: %w", unitUUID, err) | |
} |
// endpoint with the open and close port ranges for all other endpoints, returning | ||
// a new collection of openPorts and closePorts. | ||
// | ||
// There is a special wildcard endpoint "" that represents all endpoints. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Between service and state, we should have re-modeled the wildcard endpoint so that means something other than the absence of a value. Then in the future, we could start trickling that out to the API.
// Any operations applied to the wildcard endpoint will logically applied to all | ||
// endpoints. | ||
// | ||
// That is, if we open a port range on the wildcard endpoint, we will open it as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// That is, if we open a port range on the wildcard endpoint, we will open it as | |
// If we open a port range on the wildcard endpoint, we will open it as |
var uuidsToRemove []port.UUID | ||
for endpoint, portRanges := range toRemove { | ||
for _, portRange := range portRanges { | ||
if uuid, ok := current[endpoint][portRange]; ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I'm unsure if we need to iterate the portRanges everytime?
network.MustParsePortRange("100-200/tcp"), | ||
}, | ||
}).Return(nil) | ||
|
||
srv := NewService(s.st) | ||
err := srv.UpdateUnitPorts(context.Background(), unitUUID, openPorts, closePorts) | ||
err := srv.UpdateUnitPorts(context.Background(), s.unitUUID1, openPorts, network.GroupedPortRanges{}) | ||
c.Assert(err, jc.ErrorIsNil) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to see tests around your helper methods. I want to lock in behavior. This includes the wildcard reconcile and the filter methods.
SELECT | ||
port_range.uuid AS &endpointPortRangeUUID.uuid, | ||
protocol.protocol AS &endpointPortRangeUUID.protocol, | ||
port_range.from_port AS &endpointPortRangeUUID.from_port, | ||
port_range.to_port AS &endpointPortRangeUUID.to_port, | ||
unit_endpoint.endpoint AS &endpointPortRangeUUID.endpoint | ||
FROM port_range | ||
JOIN protocol ON port_range.protocol_id = protocol.id | ||
JOIN unit_endpoint ON port_range.unit_endpoint_uuid = unit_endpoint.uuid | ||
WHERE unit_endpoint.unit_uuid = $unitUUID.unit_uuid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a view.
) | ||
|
||
type UUID string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comment.
|
||
import "github.com/juju/juju/internal/errors" | ||
|
||
const ErrUnitEndpointConflict = errors.ConstError("unit endpoint conflict") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. This should be in domain/port/errors
Also, add a comment.
The initial implementation of the ports domain included far too much business logic in the state layer. This was made particularly bad given the usage of RunAtomic.
Hoist all of this business logic out of the state layer and into the service layer. Do this by re-writing the domain
This PR essentially represents an entire re-write of the domain. It might be easiest for you to just clone the branch and review it from scratch
Checklist
QA steps
Unit tests pass