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

feat: allow k8srm to connect with a kubeconfig #8953

Merged
merged 6 commits into from
Mar 7, 2024

Conversation

stoksc
Copy link
Contributor

@stoksc stoksc commented Mar 5, 2024

Description

Allow users to configure the k8s REST client from a kubeconfig. Promote the master ip/port fields to allow
jobs talking to masters outside the cluster they are in to find the master. Remove _creds_dir and its documentation
and upgrade the local devcluster route byo kubeconfig instead.

Test Plan

Follow the instructions for the dev flow in tools/k8s. Do they work?

Commentary (optional)

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

RM-15

Copy link

netlify bot commented Mar 5, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 3c2590c
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/65e8abc3935bd900089dcf2c

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 38.46154% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 47.37%. Comparing base (6ecd81e) to head (3c2590c).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8953      +/-   ##
==========================================
+ Coverage   47.35%   47.37%   +0.01%     
==========================================
  Files        1162     1162              
  Lines      176133   176129       -4     
  Branches     2237     2236       -1     
==========================================
+ Hits        83402    83435      +33     
+ Misses      92573    92536      -37     
  Partials      158      158              
Flag Coverage Δ
backend 42.73% <38.46%> (+0.08%) ⬆️
harness 63.94% <ø> (-0.01%) ⬇️
web 42.54% <ø> (ø)

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

Files Coverage Δ
master/internal/config/resource_manager_config.go 71.59% <ø> (ø)
...nal/rm/kubernetesrm/kubernetes_resource_manager.go 20.29% <0.00%> (ø)
master/internal/rm/kubernetesrm/pods.go 20.52% <43.47%> (+1.45%) ⬆️

... and 6 files with indirect coverage changes

@stoksc
Copy link
Contributor Author

stoksc commented Mar 5, 2024

I'll update the docs tomorrow, and add tests.

Copy link
Contributor

@carolinaecalderon carolinaecalderon left a comment

Choose a reason for hiding this comment

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

works on my machine, just one change needed in the devcluster.yaml

tools/k8s/devcluster.yaml Outdated Show resolved Hide resolved
tools/k8s/README.md Show resolved Hide resolved
master/internal/rm/multirm/multirm_intg_test.go Outdated Show resolved Hide resolved
@stoksc stoksc force-pushed the stoksc/feat/byo-kubeconfig branch from 39c3603 to e302694 Compare March 5, 2024 20:11
@stoksc stoksc changed the title Stoksc/feat/byo kubeconfig feat: allow k8srm to connect with a kubeconfig Mar 5, 2024
Copy link
Contributor

@carolinaecalderon carolinaecalderon left a comment

Choose a reason for hiding this comment

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

LGTM!

@stoksc stoksc enabled auto-merge (squash) March 6, 2024 17:47
Copy link
Contributor

@amandavialva01 amandavialva01 left a comment

Choose a reason for hiding this comment

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

Awesome work Bradley! LGTM

@stoksc
Copy link
Contributor Author

stoksc commented Mar 6, 2024

running k8s cpu tests then merging. codecov is a bit obnoxious, that I get knocked for adding a config to a bunch of untested files even though the meat of the change is tested.

@stoksc stoksc disabled auto-merge March 7, 2024 19:24
@stoksc stoksc merged commit b06c923 into main Mar 7, 2024
71 of 84 checks passed
@stoksc stoksc deleted the stoksc/feat/byo-kubeconfig branch March 7, 2024 19:26
maxrussell pushed a commit that referenced this pull request Mar 21, 2024
Allow users to configure the k8s REST client using by Determined by supplying us a
kubeconfig. Promote the master ip and port fields to public to allow users to configure
routing masters outside the cluster the jobs are running in (or otherwise explicitly configure
routing). Remove _creds_dir and its documentation in favor of local devclusters using the
byo kubeconfig route instead.
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