refactor(ports): refactor port service state #18147

jack-w-shaw · 2024-09-26T21:05:07Z

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

Code style: imports ordered, good names, simple structure, etc
Comments saying why design decisions were made
Go unit tests, with comments saying what you're testing

QA steps

Unit tests pass

gfouillet

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).

gfouillet · 2024-09-27T07:12:11Z

domain/port/service/package_test.go

 )

 //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)


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 !

Remove this.

State should not know about service and service should not know about state.

gfouillet · 2024-09-27T07:15:50Z

domain/port/service/service.go

-	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)


todo: For me it is not spelled correcty. Should be GetUnitOpenedPortUUIDs

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

gfouillet · 2024-09-27T08:02:58Z

domain/port/service/service.go

+	wildcardOpen, _ := openPorts[WildcardEndpoint]
+	wildcardClose, _ := closePorts[WildcardEndpoint]
+
+	wildcardOpenedSet := map[network.PortRange]bool{}


question: Is it possible to have both wildcardOpen != nil and wildcardClose != nil

Technically yes. However, in reality, it is probably never going to happen

mmmh. It is why it is never tested, i suppose :D

I'll make sure to add one now

domain/port/service/service.go

gfouillet · 2024-09-27T08:32:01Z

domain/port/service/service.go

-		wildcardClose, _ := closePorts[WildcardEndpoint]
-
-		wildcardOpened, err := s.st.GetOpenedEndpointPorts(ctx, unitUUID, WildcardEndpoint)
+		endpoints, err := s.ensureEndpoints(ctx, unitUUID, openPorts, closePorts)


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.

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.

domain/port/service/service.go

SimonRichardson

I'm still understanding this, but here is a first pass.

SimonRichardson · 2024-09-27T09:15:13Z

domain/port/service/service.go

-	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


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.

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

SimonRichardson · 2024-09-27T09:20:06Z

domain/port/service/service.go

+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)
+}


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.

SimonRichardson · 2024-09-27T09:29:28Z

domain/port/service/service.go

+		}
+		for i, portRange := range endpointOpenPorts {
+			if _, ok := wildcardOpenedSet[portRange]; ok {
+				openPorts[endpoint] = append(openPorts[endpoint][:i], openPorts[endpoint][i+1:]...)


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.

SimonRichardson

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

manadart · 2024-09-30T10:27:46Z

You seem to have agreed with some of the review comments, and resolved the conversations, but not made the associated changes.

jack-w-shaw · 2024-09-30T10:49:17Z

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

jack-w-shaw · 2024-09-30T10:49:42Z

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.

Agree. It is necessarily complex unfortunately. I have gone through an made comments more detailed.

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).

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

jack-w-shaw · 2024-09-30T11:03:47Z

@gfouillet @SimonRichardson

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

jack-w-shaw · 2024-10-04T11:59:50Z

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

https://github.com/juju/juju/pull/18185/files#r1787772899

SimonRichardson

Another pass.

SimonRichardson · 2024-10-17T11:55:39Z

domain/port/service/package_test.go

 )

 //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)


Remove this.

State should not know about service and service should not know about state.

SimonRichardson · 2024-10-17T11:56:57Z

domain/port/service/service.go

+// 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,


Suggested change

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,

SimonRichardson · 2024-10-17T11:58:11Z

domain/port/service/service.go

+	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


Exit out early.

Suggested change

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)

}

SimonRichardson · 2024-10-17T11:59:50Z

domain/port/service/service.go

+// 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.


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.

SimonRichardson · 2024-10-17T12:00:03Z

domain/port/service/service.go

+// 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


Suggested change

// 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

SimonRichardson · 2024-10-17T12:08:46Z

domain/port/service/service.go

+	var uuidsToRemove []port.UUID
+	for endpoint, portRanges := range toRemove {
+		for _, portRange := range portRanges {
+			if uuid, ok := current[endpoint][portRange]; ok {


Same here, I'm unsure if we need to iterate the portRanges everytime?

SimonRichardson · 2024-10-17T12:09:58Z

domain/port/service/service_test.go

 			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)
 }


I want to see tests around your helper methods. I want to lock in behavior. This includes the wildcard reconcile and the filter methods.

SimonRichardson · 2024-10-17T12:10:31Z

domain/port/state/state.go

+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


This should be a view.

SimonRichardson · 2024-10-17T12:12:32Z

domain/port/types.go

 )

+type UUID string


Missing comment.

SimonRichardson · 2024-10-17T12:14:07Z

domain/port/state/errors.go

+
+import "github.com/juju/juju/internal/errors"
+
+const ErrUnitEndpointConflict = errors.ConstError("unit endpoint conflict")


No. This should be in domain/port/errors

Also, add a comment.

jack-w-shaw requested review from manadart, SimonRichardson, nvinuesa and gfouillet September 26, 2024 21:05

jack-w-shaw force-pushed the refatcor_port_service_state branch 2 times, most recently from ce0d590 to add4dc1 Compare September 26, 2024 21:18

hpidcock added the 4.0 label Sep 26, 2024

jack-w-shaw removed request for nvinuesa and gfouillet September 27, 2024 09:18

gfouillet requested changes Sep 27, 2024

View reviewed changes

SimonRichardson reviewed Sep 27, 2024

View reviewed changes

SimonRichardson requested changes Sep 27, 2024

View reviewed changes

jack-w-shaw force-pushed the refatcor_port_service_state branch from add4dc1 to 9d8a17b Compare September 30, 2024 11:03

jack-w-shaw added the do not merge Even if a PR has been approved, do not merge the PR! label Sep 30, 2024

chore: respond to comments

3de8817

jack-w-shaw force-pushed the refatcor_port_service_state branch from 9d8a17b to 3de8817 Compare September 30, 2024 11:36

jack-w-shaw mentioned this pull request Oct 2, 2024

feat(ports): implement required watchers #18166

Merged

3 tasks

jack-w-shaw mentioned this pull request Oct 4, 2024

feat(ports): wire up ports domain to uniter api #18185

Merged

5 tasks

hpidcock added the has merge conflicts label Oct 7, 2024

SimonRichardson reviewed Oct 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ports): refactor port service state #18147

refactor(ports): refactor port service state #18147

jack-w-shaw commented Sep 26, 2024 •

edited

Loading

gfouillet left a comment

gfouillet Sep 27, 2024

SimonRichardson Oct 17, 2024

gfouillet Sep 27, 2024

jack-w-shaw Sep 27, 2024

gfouillet Sep 27, 2024

jack-w-shaw Sep 27, 2024

gfouillet Sep 27, 2024

jack-w-shaw Sep 27, 2024

gfouillet Sep 27, 2024

jack-w-shaw Sep 27, 2024

SimonRichardson left a comment

SimonRichardson Sep 27, 2024

jack-w-shaw Sep 27, 2024 •

edited

Loading

SimonRichardson Sep 27, 2024

SimonRichardson Sep 27, 2024

SimonRichardson left a comment

manadart commented Sep 30, 2024

jack-w-shaw commented Sep 30, 2024

jack-w-shaw commented Sep 30, 2024

jack-w-shaw commented Sep 30, 2024

jack-w-shaw commented Oct 4, 2024 •

edited

Loading

SimonRichardson left a comment

SimonRichardson Oct 17, 2024

SimonRichardson Oct 17, 2024

SimonRichardson Oct 17, 2024

SimonRichardson Oct 17, 2024

SimonRichardson Oct 17, 2024

SimonRichardson Oct 17, 2024

SimonRichardson Oct 17, 2024

SimonRichardson Oct 17, 2024

SimonRichardson Oct 17, 2024

SimonRichardson Oct 17, 2024

	// 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


		import "github.com/juju/juju/internal/errors"

		const ErrUnitEndpointConflict = errors.ConstError("unit endpoint conflict")

refactor(ports): refactor port service state #18147

Are you sure you want to change the base?

refactor(ports): refactor port service state #18147

Conversation

jack-w-shaw commented Sep 26, 2024 • edited Loading

Checklist

QA steps

gfouillet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonRichardson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-w-shaw Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonRichardson left a comment

Choose a reason for hiding this comment

manadart commented Sep 30, 2024

jack-w-shaw commented Sep 30, 2024

jack-w-shaw commented Sep 30, 2024

jack-w-shaw commented Sep 30, 2024

jack-w-shaw commented Oct 4, 2024 • edited Loading

SimonRichardson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-w-shaw commented Sep 26, 2024 •

edited

Loading

jack-w-shaw Sep 27, 2024 •

edited

Loading

jack-w-shaw commented Oct 4, 2024 •

edited

Loading