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

chore: determined_master_host and friends helm support, better defaults #10092

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

stoksc
Copy link
Contributor

@stoksc stoksc commented Oct 21, 2024

Ticket

DFR-530 / DFR-529

Description

When determined_master_ip is unsettable via Helm and defaults to the service IP, life with proxies is hard. This change renames determined_master_ip to determined_master_host everywhere with some backwards compatibility, defaults determined_master_host to <det_namespace>.<det_service_name>.svc.cluster.local, and makes all of this overridable in Helm.

Test Plan

Tested manually.

Checklist

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

@stoksc stoksc requested review from a team as code owners October 21, 2024 20:35
@stoksc stoksc requested a review from salonig23 October 21, 2024 20:35
@cla-bot cla-bot bot added the cla-signed label Oct 21, 2024
@determined-ci determined-ci added the documentation Improvements or additions to documentation label Oct 21, 2024
@determined-ci determined-ci requested a review from a team October 21, 2024 20:35
Copy link

netlify bot commented Oct 21, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit fc441a2
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/671be5bc2f61a700080ffcb1

@stoksc stoksc removed the request for review from salonig23 October 21, 2024 20:36
@stoksc stoksc force-pushed the k8s-proxy-usability-fixes branch from 4d68c85 to 1e5388c Compare October 21, 2024 21:07
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 72.72727% with 18 lines in your changes missing coverage. Please review.

Project coverage is 54.47%. Comparing base (22ad457) to head (fc441a2).
Report is 28 commits behind head on main.

Files with missing lines Patch % Lines
master/cmd/determined-master/root.go 75.55% 11 Missing ⚠️
master/internal/config/resource_manager_config.go 55.55% 4 Missing ⚠️
master/internal/rm/kubernetesrm/jobs.go 71.42% 2 Missing ⚠️
...nal/rm/kubernetesrm/kubernetes_resource_manager.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10092      +/-   ##
==========================================
- Coverage   54.49%   54.47%   -0.02%     
==========================================
  Files        1267     1267              
  Lines      159437   159456      +19     
  Branches     3637     3636       -1     
==========================================
- Hits        86885    86865      -20     
- Misses      72419    72458      +39     
  Partials      133      133              
Flag Coverage Δ
backend 45.67% <72.72%> (-0.06%) ⬇️
harness 72.55% <ø> (ø)
web 54.02% <ø> (ø)

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

Files with missing lines Coverage Δ
master/internal/rm/kubernetesrm/job.go 77.31% <100.00%> (-0.49%) ⬇️
master/internal/rm/kubernetesrm/spec.go 84.64% <100.00%> (ø)
...nal/rm/kubernetesrm/kubernetes_resource_manager.go 27.67% <0.00%> (ø)
master/internal/rm/kubernetesrm/jobs.go 70.62% <71.42%> (ø)
master/internal/config/resource_manager_config.go 64.46% <55.55%> (-1.42%) ⬇️
master/cmd/determined-master/root.go 57.50% <75.55%> (+5.22%) ⬆️

... and 8 files with indirect coverage changes

@stoksc stoksc changed the title chore: expose determined_master_host and friends in helm and default it more reasonably chore: determined_master_host and friends defaulted better and available in helm Oct 21, 2024
@stoksc stoksc changed the title chore: determined_master_host and friends defaulted better and available in helm chore: determined_master_host and friends helm support, better defaults Oct 21, 2024
@stoksc stoksc requested a review from maxrussell October 23, 2024 14:52
DetMasterIP string `json:"determined_master_ip,omitempty"`
DetMasterPort int32 `json:"determined_master_port,omitempty"`
// DeprecatedDetMasterHost shouldn't be used. Use the method DetMasterHost instead.
DeprecatedDetMasterHost string `json:"determined_master_host,omitempty"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxrussell if you know a better trick for this let me know

@stoksc stoksc force-pushed the k8s-proxy-usability-fixes branch from 1e5388c to 9b18d19 Compare October 25, 2024 01:57
Copy link
Contributor

@maxrussell maxrussell left a comment

Choose a reason for hiding this comment

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

LGTM. Could add that suggested test case, but I think it's okay without.

Comment on lines +349 to +352
delete(rm, "determined_master_ip")
} else {
log.Warn("ignoring duplicated configuration `determined_master_ip`")
delete(rm, "determined_master_ip")
Copy link
Contributor

Choose a reason for hiding this comment

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

Hah, don't see a lot of delete in Go.

},
},
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a test that ensures determined_master_host is preferred when they're both set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

super easy, i'll add it

@stoksc stoksc merged commit b3f928b into main Oct 25, 2024
82 of 95 checks passed
@stoksc stoksc deleted the k8s-proxy-usability-fixes branch October 25, 2024 19:46
stoksc added a commit that referenced this pull request Oct 25, 2024
stoksc added a commit that referenced this pull request Oct 25, 2024
thiagodallacqua-hpe pushed a commit that referenced this pull request Oct 28, 2024
…ts (#10092)

When `determined_master_ip` is unsettable via Helm and defaults to the service IP, life
with proxies is hard. This change renames `determined_master_ip` to
`determined_master_host` everywhere with some backwards compatibility, defaults
`determined_master_host` to `<det_namespace>.<det_service_name>.svc.cluster.local`
instead of the service IP, and makes all of this overridable in Helm.
thiagodallacqua-hpe pushed a commit that referenced this pull request Oct 28, 2024
…ts (#10092)

When `determined_master_ip` is unsettable via Helm and defaults to the service IP, life
with proxies is hard. This change renames `determined_master_ip` to
`determined_master_host` everywhere with some backwards compatibility, defaults
`determined_master_host` to `<det_namespace>.<det_service_name>.svc.cluster.local`
instead of the service IP, and makes all of this overridable in Helm.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants