Skip to content

Commit

Permalink
FIXME Review Continued (#692)
Browse files Browse the repository at this point in the history
  • Loading branch information
kate-osborn authored May 30, 2023
1 parent 888f03b commit 592ad5e
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 19 deletions.
1 change: 0 additions & 1 deletion internal/manager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ func Start(cfg config.Config) error {

// Note: for any new object type or a change to the existing one,
// make sure to also update prepareFirstEventBatchPreparerArgs()
// FIXME(pleshakov): Make the comment above redundant.
controllerRegCfgs := []struct {
objectType client.Object
options []controller.Option
Expand Down
8 changes: 4 additions & 4 deletions internal/status/gateway.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@ import (
)

// prepareGatewayStatus prepares the status for a Gateway resource.
// FIXME(pleshakov): Be compliant with in the Gateway API.
// Currently, we only support simple valid/invalid status per each listener.
// Extend support to cover more cases.
func prepareGatewayStatus(
gatewayStatus state.GatewayStatus,
podIP string,
Expand All @@ -21,6 +18,7 @@ func prepareGatewayStatus(
listenerStatuses := make([]v1beta1.ListenerStatus, 0, len(gatewayStatus.ListenerStatuses))

// FIXME(pleshakov) Maintain the order from the Gateway resource
// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/689
names := make([]string, 0, len(gatewayStatus.ListenerStatuses))
for name := range gatewayStatus.ListenerStatuses {
names = append(names, name)
Expand All @@ -34,7 +32,9 @@ func prepareGatewayStatus(
Name: v1beta1.SectionName(name),
SupportedKinds: []v1beta1.RouteGroupKind{
{
Kind: "HTTPRoute", // FIXME(pleshakov) Set it based on the listener
// FIXME(pleshakov) Set it based on the listener
// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/690
Kind: "HTTPRoute",
},
},
AttachedRoutes: s.AttachedRoutes,
Expand Down
3 changes: 0 additions & 3 deletions internal/status/httproute.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@ import (
)

// prepareHTTPRouteStatus prepares the status for an HTTPRoute resource.
// FIXME(pleshakov): Be compliant with in the Gateway API.
// Currently, we only support simple attached/not attached status per each parentRef.
// Extend support to cover more cases.
func prepareHTTPRouteStatus(
status state.HTTPRouteStatus,
gatewayCtlrName string,
Expand Down
15 changes: 4 additions & 11 deletions internal/status/updater.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,37 +44,32 @@ type UpdaterConfig struct {
//
// (1) It doesn't understand the leader election. Only the leader must report the statuses of the resources. Otherwise,
// multiple replicas will step on each other when trying to report statuses for the same resources.
// FIXME(pleshakov): address limitation (1)
//
// (2) It is not smart. It will update the status of a resource (make an API call) even if it hasn't changed.
// FIXME(pleshakov) address limitation (2)
//
// (3) It is synchronous, which means the status reporter can slow down the event loop.
// Consider the following cases:
// (a) Sometimes the Gateway will need to update statuses of all resources it handles, which could be ~1000. Making 1000
// status API calls sequentially will take time.
// (b) k8s API can become slow or even timeout. This will increase every update status API call.
// Making updaterImpl asynchronous will prevent it from adding variable delays to the event loop.
// FIXME(pleshakov) address limitation (3)
//
// (4) It doesn't retry on failures. This means there is a chance that some resources will not have up-to-do statuses.
// Statuses are important part of the Gateway API, so we need to ensure that the Gateway always keep the resources
// statuses up-to-date.
// FIXME(pleshakov): address limitation (4)
//
// (5) It doesn't clear the statuses of a resources that are no longer handled by the Gateway. For example, if
// an HTTPRoute resource no longer has the parentRef to the Gateway resources, the Gateway must update the status
// of the resource to remove the status about the removed parentRef.
// FIXME(pleshakov): address limitation (5)
//
// (6) If another controllers changes the status of the Gateway/HTTPRoute resource so that the information set by our
// Gateway is removed, our Gateway will not restore the status until the EventLoop invokes the StatusUpdater as a
// result of processing some other new change to a resource(s).
// FIXME(pleshakov): Figure out if this is something that needs to be addressed.
// FIXME(pleshakov): Make updater production ready
// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/691

// (7) To support new resources, updaterImpl needs to be modified. Consider making updaterImpl extendable, so that it
// To support new resources, updaterImpl needs to be modified. Consider making updaterImpl extendable, so that it
// goes along the Open-closed principle.
// FIXME(pleshakov): address limitation (7)
type updaterImpl struct {
cfg UpdaterConfig
}
Expand All @@ -88,7 +83,7 @@ func NewUpdater(cfg UpdaterConfig) Updater {

func (upd *updaterImpl) Update(ctx context.Context, statuses state.Statuses) {
// FIXME(pleshakov) Merge the new Conditions in the status with the existing Conditions
// FIXME(pleshakov) Skip the status update (API call) if the status hasn't changed.
// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/558

if upd.cfg.UpdateGatewayClassStatus && statuses.GatewayClassStatus != nil {
upd.update(
Expand Down Expand Up @@ -135,8 +130,6 @@ func (upd *updaterImpl) update(
statusSetter func(client.Object),
) {
// The function handles errors by reporting them in the logs.
// FIXME(pleshakov): figure out appropriate log level for these errors. Perhaps 3?

// We need to get the latest version of the resource.
// Otherwise, the Update status API call can fail.
// Note: the default client uses a cache for reads, so we're not making an unnecessary API call here.
Expand Down

0 comments on commit 592ad5e

Please sign in to comment.