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

service is always running on 6789 (cannot be changed) #54

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

mummyhen
Copy link
Contributor

@mummyhen mummyhen commented May 31, 2024

Summary

When updating the port of the load balancer the container port also gets updated but it is not possible to have mageai run on a different port than 6789.

Tests

Deployed the helm chart and tested the connection from the Load Balancer

cc:

@mummyhen
Copy link
Contributor Author

mummyhen commented Jun 3, 2024

@wangxiaoyou1993, could you please support?

@younsl
Copy link

younsl commented Jun 21, 2024

Currently, mageai chart v0.2.4 does not support custom containerPort value.

If you needed, Must manually refactor helm templates as shown below.

Background

In my opinion, values.yaml does not support flexible reception of containerPort from the Helm template where the mageai-webserver pod is deployed, and the current problem is that it is all input as a service.port value.

service:
type: LoadBalancer
port: 6789
# Annotations to add to the service
annotations: {}

Environment

  • Tested in EKS v1.28
  • mage-ai chart v0.2.4

Workaround

  1. add containerPort value in values.yaml newly.
# mageai/values.yaml
service:
  type: NodePort
  port: 80
+ containerPort: 6789
  # Annotations to add to the service
  annotations: {}
  1. Refactor containerPort templates/webservice.yaml
# mageai/templates/webservice.yaml
      containers:
        - name: {{ .Chart.Name }}
          securityContext:
            {{- toYaml .Values.securityContext | nindent 12 }}
          image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"
          imagePullPolicy: {{ .Values.image.pullPolicy }}
          ports:
            - name: http
-             containerPort: {{ .Values.service.port }}
+             containerPort: {{ .Values.service.containerPort | default 6789 }}
              protocol: TCP
  1. Deploy modified mage-ai chart on your kubernetes cluster
# running `mageai-webserver` pod's spec
    ports:
    - containerPort: 6789
      name: http
      protocol: TCP
    readinessProbe:
      failureThreshold: 3
      httpGet:
        path: /api/status
        port: http
        scheme: HTTP
  1. Verify connectivity to mageai service

@mummyhen
Copy link
Contributor Author

Currently, mageai chart v0.2.4 does not support custom containerPort value.

If you needed, Must manually refactor helm templates as shown below.

Background

In my opinion, values.yaml does not support flexible reception of containerPort from the Helm template where the mageai-webserver pod is deployed, and the current problem is that it is all input as a service.port value.

service:
type: LoadBalancer
port: 6789
# Annotations to add to the service
annotations: {}

Environment

  • Tested in EKS v1.28
  • mage-ai chart v0.2.4

Workaround

  1. add containerPort value in values.yaml newly.
# mageai/values.yaml
service:
  type: NodePort
  port: 80
+ containerPort: 6789
  # Annotations to add to the service
  annotations: {}
  1. Refactor containerPort templates/webservice.yaml
# mageai/templates/webservice.yaml
      containers:
        - name: {{ .Chart.Name }}
          securityContext:
            {{- toYaml .Values.securityContext | nindent 12 }}
          image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"
          imagePullPolicy: {{ .Values.image.pullPolicy }}
          ports:
            - name: http
-             containerPort: {{ .Values.service.port }}
+             containerPort: {{ .Values.service.containerPort | default 6789 }}
              protocol: TCP
  1. Deploy modified mage-ai chart on your kubernetes cluster
# running `mageai-webserver` pod's spec
    ports:
    - containerPort: 6789
      name: http
      protocol: TCP
    readinessProbe:
      failureThreshold: 3
      httpGet:
        path: /api/status
        port: http
        scheme: HTTP
  1. Verify connectivity to mageai service

Thank you for your input :)
The modification in the PR worked for me to deploy the service as a LoadBalancer with 443 port. The only issue was that the helm chart port variable was trying to modify the port on the container as well as the one on the LoadBalancer. Do you see an issue with the modification?

@younsl
Copy link

younsl commented Jun 25, 2024

@mummyhen Yes, You're right. mageai-webserver pod will not run unless using containerPort 6789.

In my opinion, But in terms of clarity in the chart, I believe it would be better to declare the containerPort of mageai-webserver pod in the values.yaml, even though we can't use ports other than 6789 in reality.

It is also related to flexibility. By declaring the port in the values.yaml file, it will be easy to change the port later if needed. If the configuration is hard-coded in the code or other locations, many parts may need to be modified when changes are required.


@wangxiaoyou1993 Could you please review this PR?

@wangxiaoyou1993 wangxiaoyou1993 merged commit 7b3ff06 into mage-ai:master Jun 25, 2024
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.

None yet

3 participants