Skip to content
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

Introducing TrafficController to manage TrafficGate and Pipeline #55

Merged
merged 4 commits into from
Jun 24, 2021

Conversation

xxx7xxxx
Copy link
Contributor

@xxx7xxxx xxx7xxxx commented Jun 20, 2021

Close #20

  • Move storage to supervisor and rename it to configSyncer, clean it to only handle sync config.
  • Clean Supervisor to make it only handle controllers.
  • Eliminate Global variable of supervisor.
  • Eliminate the ability of the filter to send contexts across pipelines.
  • Introduce TrafficController to manage all TrafficGate/Pipeline.
  • Introduce RawConfigTrafficController to be the bonding layer between raw config and TrafficController.
  • Clean some redundant code of MeshController after introducing TrafficController.
  • Fix hidden race condition of creating child span.

One TODO left (coming soon):

  • change EaseMonitorMetrics to adapt to internal changing of data structure.

@xxx7xxxx
Copy link
Contributor Author

Finished EaseMomitorMetrics, ready to be reviewed.

Copy link
Contributor

@bigangryrobot bigangryrobot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me

@@ -120,7 +120,7 @@ type (
// Status is the status of Proxy.
Status struct {
MainPool *PoolStatus `yaml:"mainPool"`
CandidatePools []*PoolStatus `yaml:"candidatePool,omitempty"`
CandidatePools []*PoolStatus `yaml:"candidatePools,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filter reference needs to be updated accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been updated already. This is a legacy typo.

namespace string
// The scenario here satisfies the first common case:
// When the entry for a given key is only ever written once but read many times.
// Referecen: https://golang.org/pkg/sync/#Map
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

Suggested change
// Referecen: https://golang.org/pkg/sync/#Map
// Reference: https://golang.org/pkg/sync/#Map

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Comment on lines 52 to 53
Space struct {
namespace string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Space represent a namespace? If my understanding is right, suggest change it to Namespace

Suggested change
Space struct {
namespace string
Namespace struct {
name string

I got confused with the name StatusOneSpace at first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Comment on lines +45 to +46
httpServer *supervisor.ObjectEntity
httpPipelines map[string]*supervisor.ObjectEntity
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems httpServer and httpPipelines are redundant as tc already has all the necessary information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the original implementation, it's hard to eliminate them at once. They must be deleted at Close and so on. I will clean all codes of MeshController in the next contribution.

}

// Status is the status of RawConfigTrafficController.
Status = trafficcontroller.StatusOneSpace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to easemonitormetrics.go implementation, StatusOneSpace seems to only be used to let HTTPServer and HTTPPiepline's status records reporting to the same metric name. (SameSpace?) Proposal to add more comments about this status or change it to a more meaningful name.(StatusSameNamespace)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

type (
// RawConfigTrafficController is a system controller to manage
// TrafficGate, Pipeline and their relationship.
RawConfigTrafficController struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that the difference betweenRawConfigTrafficController and TrafficController is only the default-namespace and mulitple-namespace? If so, proposal to remove this RawConfigTrafficController and let TrafficController handle the default namespace as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/megaease/easegress/pull/55/files?short_path=f277e23#diff-f277e23e848049e2b25d0b86693cdcecae6c031e2d70271a4715f2e4d26f5f21

RawConfigTrafficController is a must-have controller to decouple TrafficController with the input source. It will bring more complicity and less extensibility to TrafficController.

Copy link
Contributor

@benja-wu benja-wu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@benja-wu benja-wu merged commit d0d9c8b into easegress-io:main Jun 24, 2021
@xxx7xxxx xxx7xxxx deleted the trafficcontroller branch June 24, 2021 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use traffic controller to manage TrafficGate and Pipeline
4 participants