[multikueue] Add Path kubeConfig location type #1640

trasc · 2024-01-25T10:37:55Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add Path location type for MultiKueue cluster KubeConfigs

Which issue(s) this PR fixes:

Relates to #693

Special notes for your reviewer:

Does this PR introduce a user-facing change?

MultiKueue: Add Path location type for cluster KubeConfigs.

k8s-ci-robot · 2024-01-25T10:37:57Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

netlify · 2024-01-25T10:38:01Z

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	`fffb0c1`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/65b765b5983bdd00087c68a4
😎 Deploy Preview	https://deploy-preview-1640--kubernetes-sigs-kueue.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

trasc · 2024-01-25T10:38:26Z

/hold

for #1631

trasc · 2024-01-29T08:46:23Z

/unhold

pkg/controller/admissionchecks/multikueue/multikueuecluster.go

mimowo

LGTM overall, one question regarding the not-so-clear change: https://github.com/kubernetes-sigs/kueue/pull/1640/files#r1469263382

mimowo

/lgtm
/assign @alculquicondor @tenzen-y

k8s-ci-robot · 2024-01-29T15:18:37Z

LGTM label has been added.

Git tree hash: 9d0c6171359e5d7cfa8c64acf154933660f89896

tenzen-y · 2024-01-30T12:48:03Z

apis/kueue/v1alpha1/multikueue_types.go

+	// Location is the path on the disk of kueue-controller-manager.
+	PathLocationType LocationType = "Path"


This Path location type is assumed to be the main story that we want to use credentials that existed on the Node. It means that the main use case is mounting credentials by hostPath volume, right?

It makes sense. I think that we should mention here that we would suggest using the SecretLocationType as much as possible, and the PathLocationType is assumed to be used for mounting credentials from nodes via hostPath volumeMount.

However, since this is a non-blocking suggestion, I think that we can work on another PR. I leave on @trasc whether or not we work on it at this PR.

SecretLocationType is the default value, we could present come pros/cons when we add the documentation.

tenzen-y · 2024-01-30T12:54:16Z

pkg/controller/admissionchecks/multikueue/testdata/worker1KubeConfig

Uhm, I don't like having such a test.
Should we generate this file in bin or tmp dir, then can we use the generated testdata?

It will make the testing strategy more complicated, (create the temp file, replace the path in the needed clusters in tc.clusters, cleanup) .

Even if probably not used in kueue until now, the testdata approach is more or less standard in go,

Uhm, I understand that this is the standard approach, but as much as possible, I want to avoid having such testdata.

However, I don't want to block the merge of this PR due to this discussion. So, we probably can come back here when we have more testdata in the near future.

tenzen-y

/lgtm
/approve

/hold for #1640 (comment)

tenzen-y · 2024-01-31T16:40:23Z

apis/kueue/v1alpha1/multikueue_types.go

+	// Location is the path on the disk of kueue-controller-manager.
+	PathLocationType LocationType = "Path"


It makes sense. I think that we should mention here that we would suggest using the SecretLocationType as much as possible, and the PathLocationType is assumed to be used for mounting credentials from nodes via hostPath volumeMount.

However, since this is a non-blocking suggestion, I think that we can work on another PR. I leave on @trasc whether or not we work on it at this PR.

tenzen-y · 2024-01-31T16:44:07Z

pkg/controller/admissionchecks/multikueue/testdata/worker1KubeConfig

Uhm, I understand that this is the standard approach, but as much as possible, I want to avoid having such testdata.

However, I don't want to block the merge of this PR due to this discussion. So, we probably can come back here when we have more testdata in the near future.

k8s-ci-robot · 2024-01-31T16:44:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y, trasc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [tenzen-y]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

trasc · 2024-02-01T06:39:30Z

/unhold

tenzen-y · 2024-02-12T20:53:06Z

/kind api-change

alculquicondor · 2024-02-13T19:40:43Z

/release-note-edit

MultiKueue: Add Path location type for cluster KubeConfigs.

* [multikueue] Add Path kubeconfig location type * Fix after rebase

k8s-ci-robot requested review from denkensk and mimowo January 25, 2024 10:38

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 25, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 25, 2024

trasc marked this pull request as ready for review January 25, 2024 10:38

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 25, 2024

trasc added 2 commits January 29, 2024 10:29

[multikueue] Add Path kubeconfig location type

fc99487

Fix after rebase

fffb0c1

trasc force-pushed the multikueue-kubeconfig-file branch from 7a9d840 to fffb0c1 Compare January 29, 2024 08:45

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 29, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2024

mimowo reviewed Jan 29, 2024

View reviewed changes

pkg/controller/admissionchecks/multikueue/multikueuecluster.go Show resolved Hide resolved

mimowo reviewed Jan 29, 2024

View reviewed changes

k8s-ci-robot assigned alculquicondor, tenzen-y and mimowo Jan 29, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2024

tenzen-y reviewed Jan 30, 2024

View reviewed changes

tenzen-y reviewed Jan 31, 2024

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 31, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2024

k8s-ci-robot merged commit d62d526 into kubernetes-sigs:main Feb 1, 2024
14 checks passed

k8s-ci-robot added this to the v0.6 milestone Feb 1, 2024

trasc deleted the multikueue-kubeconfig-file branch February 1, 2024 07:07

k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Feb 12, 2024

kannon92 pushed a commit to openshift-kannon92/kubernetes-sigs-kueue that referenced this pull request Nov 19, 2024

[multikueue] Add Path kubeConfig location type (kubernetes-sigs#1640)

d5cd73c

* [multikueue] Add Path kubeconfig location type * Fix after rebase

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[multikueue] Add Path kubeConfig location type #1640

[multikueue] Add Path kubeConfig location type #1640

trasc commented Jan 25, 2024 •

edited by k8s-ci-robot

Loading

k8s-ci-robot commented Jan 25, 2024

netlify bot commented Jan 25, 2024 •

edited

Loading

trasc commented Jan 25, 2024

trasc commented Jan 29, 2024

mimowo left a comment

mimowo left a comment

k8s-ci-robot commented Jan 29, 2024

tenzen-y Jan 30, 2024

trasc Jan 30, 2024

tenzen-y Jan 31, 2024

trasc Feb 1, 2024

tenzen-y Jan 30, 2024

trasc Jan 30, 2024

tenzen-y Jan 31, 2024

tenzen-y left a comment •

edited

Loading

tenzen-y Jan 31, 2024

tenzen-y Jan 31, 2024

k8s-ci-robot commented Jan 31, 2024

trasc commented Feb 1, 2024

tenzen-y commented Feb 12, 2024

alculquicondor commented Feb 13, 2024

		// Location is the path on the disk of kueue-controller-manager.
		PathLocationType LocationType = "Path"

[multikueue] Add Path kubeConfig location type #1640

[multikueue] Add Path kubeConfig location type #1640

Conversation

trasc commented Jan 25, 2024 • edited by k8s-ci-robot Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Jan 25, 2024

netlify bot commented Jan 25, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue ready!

trasc commented Jan 25, 2024

trasc commented Jan 29, 2024

mimowo left a comment

Choose a reason for hiding this comment

mimowo left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 31, 2024

trasc commented Feb 1, 2024

tenzen-y commented Feb 12, 2024

alculquicondor commented Feb 13, 2024

trasc commented Jan 25, 2024 •

edited by k8s-ci-robot

Loading

netlify bot commented Jan 25, 2024 •

edited

Loading

tenzen-y left a comment •

edited

Loading