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

prefect-server helm chart enhancement to support multi-port service #318

Merged
merged 5 commits into from
May 24, 2024

Conversation

baisystems
Copy link
Contributor

@baisystems baisystems commented Apr 24, 2024

prefect-server helm chart enhancement to support multi-port service.

  • service template creates service name and ports based on the ports array from values.yaml file
  • update deployment template to use the server port extracted from the ports array
  • add a service port to ingress template and values
  • updated helm chart validation schema

closes #314

@baisystems baisystems requested a review from a team as a code owner April 24, 2024 15:03
@jamiezieziula
Copy link
Contributor

Hi @baisystems, thanks for the PR! Is there a reason we can't accomplish this functionality by allowing for additionalPorts to be passed as an optional array[dictionary] and leaving the current handling of the required service ports as is?

@baisystems
Copy link
Contributor Author

Hi @jamiezieziula, there is no additionalPorts currently. If I understand you correctly, you are proposing to add a new extraPorts field under service in values.yaml file and use it like this in service template:

ports:
    - name: server-svc-port
      port: {{ .Values.service.port }}
      ...
    {{- range .Values.service.extraPorts }}
    - name: {{ .name }}
      port: {{ .port }}
      ...
    {{- end }}

If this is the case, my answer is Yes, we can accomplish this functionality with extraPorts. I do see the merits of your proposal: minimal changes to current code base as I don't need to change deployment.yaml and I don't need to add a new function tartget-port.yaml.

Your proposal also agree well with your current helm chart pattern extra ..., for example, extraContainers for server pod and extraPaths for server ingress.

I actually like your idea and can tailor my pr to implement the extraPorts field as I illustrated above.

The second feature of this PR is its introduction of a new field servicePort to ingress. This will make the ingress service port configurable to accommodate the case where the ingress is serving a port other than server-svc-port. What do you think about this change?

@jamiezieziula
Copy link
Contributor

@baisystems thanks for the feedback! I look forward to reviewing your changes :)

I like the idea of the customizability of the ingress service port!

@baisystems
Copy link
Contributor Author

@jamiezieziula I pushed a new commit which implemented the extraPorts field as discussed. Please review.

@@ -26,6 +26,7 @@ spec:
{{- if eq .Values.service.type "NodePort" }}
externalTrafficPolicy: {{ .Values.service.externalTrafficPolicy | quote }}
{{- end }}
{{- $serviceType := .Values.service.type }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is neccessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to define this variable to hold the value of serviceType. The variable is used inside the loop of {{- range .Values.service.extraPorts }} to set nodePort. While inside the loop, it can't access the .Values.service.type due to context being changed. Is there better way to handle this situation?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah i see - yes i think this makes sense!

Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to access the value with $.Values.service.type (notice the $. instead of just .).

There's an example at the bottom of https://helm.sh/docs/chart_template_guide/variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mitchnielsen Thank you for the tip. It works. $ - this variable will always point to the root context. Great to learn this.

Copy link
Contributor

@jamiezieziula jamiezieziula left a comment

Choose a reason for hiding this comment

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

A few minor comments - thanks!

@baisystems
Copy link
Contributor Author

@jamiezieziula I pushed a new commit which default the extraPorts value to []. Any other issue for this pr?

@jamiezieziula
Copy link
Contributor

@baisystems - can you install precommit and run it? linters are failing.

@baisystems
Copy link
Contributor Author

@jamiezieziula I pushed a new commit. All the lint errors are fixed and the build is now passed. Could you please take another look?

@jamiezieziula
Copy link
Contributor

Hi @baisystems - thanks! All linting looks healthy now. Sorry to do this, but one last thing - see @mitchnielsen's comment above.

@baisystems
Copy link
Contributor Author

@jamiezieziula I tested @mitchnielsen's suggestion, helm install succeed with it. I will push an update commit soon.

@baisystems
Copy link
Contributor Author

@jamiezieziula I pushed a new commit, the 5th. Please review again.

@jamiezieziula jamiezieziula merged commit a03ef7a into PrefectHQ:main May 24, 2024
16 checks passed
@jamiezieziula jamiezieziula added the enhancement An improvement of an existing feature label May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancing Prefect Server Helm Chart for Sidecar Containers
3 participants