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

fix: Helm master-service clusterip mngt. openshift #8546

Merged
merged 4 commits into from
Jan 10, 2024

Conversation

daniele-cellai
Copy link
Contributor

@daniele-cellai daniele-cellai commented Dec 6, 2023

Description

Changes regarding: Helm chart - master-service.yaml template file

On OPENSHIFT clusters, if the parameter .Values.openshiftRoute.enabled is set to true, to request the creation of the Route HAProxy, the related master service must have type ClusterIP; this because using the the Haproxy route the IP traffic goes though the Load Balancer (LB) already present in the Haproxy itself, hence nor the LoadBalancer neither the NodePort are needed.

Previously the chart created a master service that depends on the param. useNodePortForMaster, so it creartes NodePort or LoadBalancer (as in this case) that is not needed.

Template master-service.yaml change practically works this way: if an route openshift is requested the service type will be ClusterIP independently on the useNodePortForMaster value.

Customer Intesa Sanpaolo Bank
https://hpe-aiatscale.atlassian.net/browse/HELP-381

Test Plan

Tested on both, OpenShift enviroment and GCP cluster.

Commentary (optional)

N/A

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

HELP-381

@cla-bot cla-bot bot added the cla-signed label Dec 6, 2023
Copy link

netlify bot commented Dec 6, 2023

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 8137891
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/659ec75532a60b000800a021
😎 Deploy Preview https://deploy-preview-8546--determined-ui.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

cla-bot bot commented Dec 15, 2023

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @daniele-cellai on file. In order for us to review and merge your code, please start the CLA process at https://determined.ai/cla.

After we approve your CLA, we will update the contributors list (private) and comment @cla-bot[bot] check to rerun the check.

@cla-bot cla-bot bot removed the cla-signed label Dec 15, 2023
Copy link

cla-bot bot commented Dec 15, 2023

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @daniele-cellai on file. In order for us to review and merge your code, please start the CLA process at https://determined.ai/cla.

After we approve your CLA, we will update the contributors list (private) and comment @cla-bot[bot] check to rerun the check.

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4354c8e) 51.47% compared to head (8137891) 51.48%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8546   +/-   ##
=======================================
  Coverage   51.47%   51.48%           
=======================================
  Files         884      884           
  Lines      156061   156061           
  Branches     2084     2084           
=======================================
+ Hits        80339    80342    +3     
+ Misses      74234    74232    -2     
+ Partials     1488     1487    -1     
Flag Coverage Δ
backend 36.14% <ø> (+<0.01%) ⬆️
harness 64.31% <ø> (ø)
web 53.76% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

@daniele-cellai daniele-cellai merged commit c99bfc3 into main Jan 10, 2024
69 of 83 checks passed
@daniele-cellai daniele-cellai deleted the fix-helm-master-service-tpl branch January 10, 2024 16:52
@dannysauer dannysauer modified the milestone: 0.27.1 Feb 6, 2024
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.

3 participants