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

Revert part of #47-Improve global-sidecar request forwarding capabilities #155

Merged
merged 1 commit into from
Jun 14, 2022

Conversation

cywang1905
Copy link
Contributor

@cywang1905 cywang1905 commented Jun 13, 2022

In #47 , We have given the global-sidecar component the ability to underwrite disaster recovery. The pod port and service port of global-sidecar correspond to each other. But the addition of this capability pollutes the lazyload previous logic. We decided to make a clearer distinction between the two.

  • Normal lazyload traffic goes to the global-sidecar port defined by component.globalSidecar.port in slimeboot, int value, default "80"
  • Abnormal disaster recovery traffic goes to the corresponding port in the global-sidecar based on the port of the original request

Other added capabilities of #47 are retained, such as global-sidecar health check ports, logging levels, number of instances, resource quotas, etc. can be configured.


#47 中,我们赋予了global-sidecar组件灾备兜底的能力。global-sidecar的pod端口和service端口一一对应。但这种能力的添加污染了懒加载原有逻辑。我们决定对二者做更明确的区分。

  • 正常的懒加载兜底流量走到slimeboot中component.globalSidecar.port定义的global-sidecar端口,int类型,默认80
  • 异常的灾备流量则根据原始请求的端口走到global-sidecar中相应的端口

#47 中其他的新增能力予以保留,比如global-sidecar健康检查端口、日志级别、实例数、资源配额等可配置。

@cywang1905 cywang1905 force-pushed the master branch 3 times, most recently from b3ed797 to 8acbbda Compare June 13, 2022 11:31
@@ -26,12 +26,20 @@ metadata:
slime.io/serviceFenced: "false"
spec:
ports:
# for lazyload
Copy link
Contributor

Choose a reason for hiding this comment

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

To reduce duplicate code, what do you think about writing it in the following way?

var gsPort = default(80, gs.port)
var hasSidecarPort bool
svcPorts := make([]int, 0, len(wormholePort))
for _, p := range wormholePort {
  svcPorts = append(svcPorts, p)
  if p == gsPort {
    hasSidecarPort = true
  }
}

if !hasSidecarPort {
  svcPorts = append(svcPorts, gsPort)
}

for _, p := range svcPorts {
   // ...

   // gsPort ...

  /// ...
}

@cywang1905 cywang1905 force-pushed the master branch 2 times, most recently from 7822e1b to fe17558 Compare June 14, 2022 07:00
{{ $hasGsPort := false }}
{{- range $f.wormholePort }}
{{- if eq (int .) $gsPort }}
{{ $hasGsPort = true }}
Copy link
Contributor

Choose a reason for hiding this comment

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

then break?

Copy link
Contributor

Choose a reason for hiding this comment

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

  {{ $hasGsPort := false }}
  {{ $gsSvcPorts := list }}
  {{- range .Values.wormholePort }}
  {{- if eq (int .) (int $.Values.gsPort) }}
  {{ $hasGsPort = true }}
  {{- end -}}
  {{ $gsSvcPorts = append $gsSvcPorts . }}
  {{- end -}}

  {{- if not $hasGsPort }}
  {{ $gsSvcPorts = append $gsSvcPorts .Values.gsPort }}
  {{- end -}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

much simpler, changed

@@ -2,6 +2,7 @@
{{ if .Values.component.globalSidecar }}
{{ if .Values.component.globalSidecar.enable }}
{{ $gs := .Values.component.globalSidecar }}
{{ $gsPort := $gs.port | default 80 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

does this globalSidecar.port do sth with lazyload.gsPort?

@YonkaFang YonkaFang merged commit 151396a into slime-io:master Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants