Skip to content

Add skywalking satellite support#83

Merged
wu-sheng merged 5 commits intoapache:masterfrom
mrproliu:support-satellite
Nov 29, 2021
Merged

Add skywalking satellite support#83
wu-sheng merged 5 commits intoapache:masterfrom
mrproliu:support-satellite

Conversation

@mrproliu
Copy link
Contributor

  • Support using satellite as a gateway
  • upgrade E2E as infra-e2e

@mrproliu mrproliu added the enhancement New feature or request label Nov 24, 2021
@mrproliu mrproliu added this to the 4.2.0 milestone Nov 24, 2021
helm install "${SKYWALKING_RELEASE_NAME}" ${REPO}/skywalking -n "${SKYWALKING_RELEASE_NAMESPACE}" \
--set satellite.enabled=true \
--set satellite.image.tag=v0.3.0
```
Copy link
Member

Choose a reason for hiding this comment

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

We should add description about user should set service address to satellite instead of oap, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

been added, please take a look.

Copy link
Contributor

@hanahmily hanahmily left a comment

Choose a reason for hiding this comment

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

Some nits of manifests.

@kezhenxu94
Copy link
Member

Others LGTM

@wu-sheng
Copy link
Member

Once recent optimization are done, we should add document about how to set up load balancer in the main doc, and provide it(next release) as activated component in the showcase.
Please submit an issue to track for doc and showcase.

@kezhenxu94
Copy link
Member

Once recent optimization are done, we should add document about how to set up load balancer in the main doc, and provide it(next release) as activated component in the showcase.
Please submit an issue to track for doc and showcase.

Just a reminder, satellite is not mandatory so we won't make all agent services connect to it in showcase, as we have 2 Java services (gateway and songs) we can make one of them send data to satellite and leave all others as is.

@wu-sheng
Copy link
Member

wu-sheng commented Nov 26, 2021

Once recent optimization are done, we should add document about how to set up load balancer in the main doc, and provide it(next release) as activated component in the showcase.
Please submit an issue to track for doc and showcase.

Just a reminder, satellite is not mandatory so we won't make all agent services connect to it in showcase, as we have 2 Java services (gateway and songs) we can make one of them send data to satellite and leave all others as is.

With time, more and more verification, could come from the product env(end users), if all are good, I would like to put it as recommended. In all these days perf testing and load balance testing, connection based load balance has very clear disadvantage, but most proxies are only supporting this way.
Users complained many times, now, we have the solution.
So, once we confirm Satellite is good, it should be some kind of mandatory(recommended as it is better to have). Does this make sense to you?

@kezhenxu94
Copy link
Member

Once recent optimization are done, we should add document about how to set up load balancer in the main doc, and provide it(next release) as activated component in the showcase.

Please submit an issue to track for doc and showcase.

Just a reminder, satellite is not mandatory so we won't make all agent services connect to it in showcase, as we have 2 Java services (gateway and songs) we can make one of them send data to satellite and leave all others as is.

With time, more and more verification, could come from the product, if all are good, I would like to put it as recommended. In all these days perf testing and local balance testing, connection based load balance has very clear disadvantage, but most proxies are only supporting this way.

Users complained many times, now, we have the solution.

So, once we confirm Satellite is good, it should be some kind of mandatory(recommended as it is better to have). Does this make sense to you?

We could recommend, but showcase repo serves as a demonstration that will be referenced by all users, we should have examples without Satellite, unless we say officially SkyWalking cannot work without Satellite, which is obviously not the case.

I know Satellite can work with or without Kubernetes but for traditional deployments I don't believe users will deploy another Satellite process along with their application process, in this case they will ask for non-Satellite solution. That's why I want to keep agents without Satellite in showcase.

@wu-sheng
Copy link
Member

If you are concern about this, I would prefer not deploy satellite in showcase.
The reason I asked about this, is users will face this load balance issue from day one when they go to product env deployment. Even Java agent provide random-based client-side load balancing, AFAIK, it doesn't work perfectly.

If you set the principle of deploying XYZ in showcase as tech level mandatory, then let's leave satellite out of it. It is definitely not.

Half through satellite, and half using direct link, are really not a good showcase at all. It just increases the confusion, I am afraid.

@kezhenxu94
Copy link
Member

kezhenxu94 commented Nov 26, 2021

If you set the principle of deploying XYZ in showcase as tech level mandatory,

Hey that's not what I mean, we have agent services and agentless services and agent is not mandatory tech in SkyWalking but we deploy them still. (I just want to cover more general scenarios that users have). Let's find another way to add Satellite into showcase. Now we have many feature flags that users can choose to deploy a subset of features of SkyWalking(though we enable all features by default), let's add another feature flag satellite that deploys Satellite and makes all agents and ALS connect to it, and enable this feature by default (on demo.skywalking.a.o), but users can have a chance to remove satellite feature and make agent connect to OAP directly. WDYT?

@wu-sheng
Copy link
Member

I don't want showcase becomes another challenge to new users. Features related are complex enough so far.
Let's keep satellite out for now.

@wu-sheng
Copy link
Member

Let's continue discussion on that new created issue if needed.
This is not a block for this pull request.

Copy link
Contributor

@hanahmily hanahmily left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot.

@wu-sheng wu-sheng requested a review from kezhenxu94 November 29, 2021 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants