add kubelet client for Pod resource info #231

ahalimx86 · 2019-01-08T15:41:24Z

This change introduces kubelet client to get allocated device
information of a Pod from newly added Kubelet grpc service.
For more information please see:
kubernetes/kubernetes#70508

With this kubelet service we no longer need to rely on kubelet
checkpoint file to get device information.

Change-Id: I3ba098cfea9a0e798f065a4d9b39d0085e125407
Signed-off-by: Abdul Halim abdul.halim@intel.com

coveralls · 2019-01-08T15:53:54Z

Pull Request Test Coverage Report for Build 729

55 of 77 (71.43%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+1.2%) to 52.841%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
checkpoint/checkpoint.go	3	5	60.0%
k8sclient/k8sclient.go	4	9	44.44%
kubeletclient/kubeletclient.go	48	63	76.19%

Totals
Change from base Build 727:	1.2%
Covered Lines:	651
Relevant Lines:	1232

💛 - Coveralls

rkamudhan · 2019-01-08T15:55:46Z

@dougbtv @s1061123 Targeting for master branch.

dcbw · 2019-01-09T03:50:53Z

This brings in a ton of vendored deps, +2million lines. Are we really sure we need all those deps?

ahalimx86 · 2019-01-09T16:35:06Z

This brings in a ton of vendored deps, +2million lines. Are we really sure we need all those deps?

This PR has following dependencies:

	"k8s.io/kubernetes/pkg/kubelet/apis/podresources"
	"k8s.io/kubernetes/pkg/kubelet/apis/podresources/v1alpha1"
	"k8s.io/kubernetes/pkg/kubelet/util"

So, I've added only these sub-packages in glide.yaml with correct version then run glide up -v. Looks like even though all I needed is those sub-packages only, glide added pretty much everything from Kubernetes! That's why the vendor became gigantic! Any suggestions/alternatives how to overcome this?

s1061123 · 2019-01-11T09:51:50Z

Hi @ahalim-intel ,

How about to use '--skip-test' option in glide? It may reduce dependent packages.

ahalimx86 · 2019-01-11T10:07:09Z

@s1061123

How about to use '--skip-test' option in glide? It may reduce dependent packages.

Actually glide '--skip-test' is counter-intuitive. It does the opposite. From 'glide up --help':

   --skip-test              Resolve dependencies in test files.

I've tried it anyway and it didn't change the vendor/ at all.
I've also tried glide up -v --no-recursive. This reduces vendor/ size dramatically but build fails.

ahalimx86 · 2019-02-04T10:54:45Z

Folks, what is the status of this PR?

dcbw · 2019-02-21T15:26:59Z

/lgtm, as long as we get the backwards compat questions answered.

rkamudhan

@s1061123 @dougbtv

s1061123 · 2019-03-13T00:06:44Z

@ahalim-intel

as Dan's comment, your code removes checkpoint code and you remove checkpoint feature. This change removes previous feature and don't have compatibility. How do you support checkpoint?
your diff is slightly old so could you please rebase it to latest?

ahalimx86 · 2019-03-13T10:00:10Z

@ahalim-intel

as Dan's comment, your code removes checkpoint code and you remove checkpoint feature. This change removes previous feature and don't have compatibility. How do you support checkpoint?

your diff is slightly old so could you please rebase it to latest?

@s1061123
I will be working on this in coming days. I am thinking of introducing an option field in Multus netconf something like resourceInfoProvider: with values checkpoint(default) OR kubelet-api. What do you think?

zshi-redhat · 2019-03-13T22:57:24Z

@ahalim-intel

as Dan's comment, your code removes checkpoint code and you remove checkpoint feature. This change removes previous feature and don't have compatibility. How do you support checkpoint?

your diff is slightly old so could you please rebase it to latest?

Tomo, just want to mention that checkpoint file is a temporary solution for getting device information from kubelet and it will be deprecated in future kube releases. we'd like to switch to kubelet endpoint with kube-1.13 and 1.13+. This feature is mainly used for passing device info to SR-IOV CNI, I dont' know if any other CNI use this feature. I understand that Multus needs maintain backward compatibility, do you know how we can deprecate a feature like checkpoint file in Multus? Like how many kube releases Multus need to maintain backward compat with?

adding new imported package dependencies in vendor which is required for Kubelet Pod Resource api client. Change-Id: If6c74598e12af5f8659df69371e72dd064823f49

ahalimx86 · 2019-03-20T12:18:07Z

@s1061123 @dougbtv This PR is rebased now. Also addressed the backward compatibility. If Kubelet resource API is available Multus will use that OR else fallback in Kubelet checkpoint.

rkamudhan · 2019-03-21T14:39:57Z

@zshi-redhat Could you please test this PR ?

ahalimx86 · 2019-03-21T16:21:43Z

@s1061123

I'm bit confused with the travis test output there.

Output from travisci:

# github.com/intel/multus-cni/kubeletclient
kubeletclient/kubeletclient_test.go:106:3: client declared but not used
vet: typecheck failures
FAIL	github.com/intel/multus-cni/kubeletclient [build failed]

If you see the closure variable client on line 106, it is being used within other context block. It's not 'not used'. What do you suggest?

And running test.sh locally with same file(kubeletclient/kubeletclient_test.go):

=== RUN   TestKubeletclient                                                             
Running Suite: Kubeletclient Suite                                                      
==================================                                                      
Random Seed: 1553184260                                                                 
Will run 5 of 5 specs                                                                   

•••••
Ran 5 of 5 Specs in 0.018 seconds
SUCCESS! -- 5 Passed | 0 Failed | 0 Pending | 0 Skipped --- PASS: TestKubeletclient (0.02s)
PASS                                                                                       
coverage: 77.5% of statements                                                              
ok      github.com/intel/multus-cni/kubeletclient       0.023s  coverage: 77.5% of statements

ahalimx86 · 2019-03-21T16:29:03Z

Ok, nevermind it. I've move the variable from there and updated the PR.

zshi-redhat · 2019-03-22T04:08:39Z

@zshi-redhat Could you please test this PR ?

I ran the following tests with this PR applied to multus master branch, all passed:

Enable KubeletPodResources feature gate with kube-1.13 env, Multus can detect that kubelet podResource endpoint exists, query and pass the correct device ID that allocated by SR-IOV Device Plugin to SR-IOV CNI.
Disable KubeletPodResources (default config) with kube-1.13 env, Multus can fall back to checkpoint file solution to get Device ID information and pass it to SR-IOV CNI.

zshi-redhat · 2019-03-22T02:09:36Z

kubeletclient/kubeletclient.go

+	// Check for kubelet resource API socket file
+	kubeletSocket = filepath.Join(defaultPodResourcesPath, defaultKubeletSocketFile)
+	if _, err := os.Stat(kubeletSocket); err != nil {
+		logging.Errorf("hasKubeletAPIEndpoint(): error looking up kubelet resource api socket file: %q", err)


Might be good to have a warning message instead of error.

Might be good to have a warning message instead of error.

Agree! I will change this to Printf() as the logging has no Warningf()

+1, Printf() shall be fine.

zshi-redhat

It looks like the CI failure is due to that kubeletSocket is overwritten in hasKubeletAPIEndpoint to the value of defaultPodResourcesPath + defaultKubeletSocketFile, perhaps we can just set socketDir and socketName to the same value of defaultPodResourcesPath and defaultKubeletSocketFile.

zshi-redhat · 2019-03-22T05:28:05Z

kubeletclient/kubeletclient.go

+
+func getKubeletClient() (types.ResourceClient, error) {
+	newClient := &kubeletClient{}
+	if kubeletSocket == "" {


This will always be false as kubeletSocket is set in hasKubeletAPIEndpoint.

This will always be false as kubeletSocket is set in hasKubeletAPIEndpoint.

Good catch! I've corrected the variable in hasKubeletAPIEndpoint which should be only local.

zshi-redhat · 2019-03-22T05:30:15Z

kubeletclient/kubeletclient.go

+
+func hasKubeletAPIEndpoint() bool {
+	// Check for kubelet resource API socket file
+	kubeletSocket = filepath.Join(defaultPodResourcesPath, defaultKubeletSocketFile)


why not use the same as util.LocalEndpoint(defaultPodResourcesPath, podresources.Socket)?

why not use the same as util.LocalEndpoint(defaultPodResourcesPath, podresources.Socket)?

util.LocalEndpoint(defaultPodResourcesPath, podresources.Socket) returns path with unix:// URL scheme prefix. All we need here simple path string.

gotcha, thanks!

zshi-redhat · 2019-03-22T05:31:21Z

kubeletclient/kubeletclient.go

+)
+
+var (
+	kubeletSocket string


why this is a global var?

Need this global here for unit-tests to override it.

This change introduces kubelet client to get allocated device information of a Pod from newly added Kubelet grpc service. For more information please see: [kubernetes/kubernetes#70508](kubernetes/kubernetes#70508) Change-Id: I11e58ccdd52662601f445fa24c7d55c225441efc Signed-off-by: Abdul Halim <abdul.halim@intel.com>

ahalimx86 · 2019-03-22T13:38:09Z

@s1061123 @zshi-redhat @dougbtv

I've addressed pretty much all the comments and feedback. Also fixed the travis-ci tests. All checks now pass. It's ready to be merged!

Thanks everyone for help reviewing/improving this PR... much appreciated!

zshi-redhat · 2019-03-22T13:48:10Z

/lgtm

ahalimx86 requested a review from rkamudhan January 8, 2019 15:41

ahalimx86 force-pushed the dev/kubelet_client branch from 1ba9062 to cc5c0f5 Compare January 8, 2019 16:04

ahalimx86 force-pushed the dev/kubelet_client branch 2 times, most recently from 7304cfa to 32a3a7c Compare January 9, 2019 16:09

ahalimx86 force-pushed the dev/kubelet_client branch from 32a3a7c to b18c5c4 Compare February 7, 2019 15:52

rkamudhan requested review from s1061123 and dougbtv February 21, 2019 15:26

rkamudhan reviewed Feb 21, 2019

View reviewed changes

ahalimx86 force-pushed the dev/kubelet_client branch from b18c5c4 to 38c0aae Compare March 20, 2019 11:50

update vendor dependencies for kubelet client

81afd29

adding new imported package dependencies in vendor which is required for Kubelet Pod Resource api client. Change-Id: If6c74598e12af5f8659df69371e72dd064823f49

ahalimx86 force-pushed the dev/kubelet_client branch from 38c0aae to 957de54 Compare March 20, 2019 12:14

ahalimx86 force-pushed the dev/kubelet_client branch from 957de54 to 032237a Compare March 20, 2019 12:21

ahalimx86 force-pushed the dev/kubelet_client branch from 032237a to 78db614 Compare March 21, 2019 16:28

s1061123 force-pushed the master branch 2 times, most recently from 1b0b39d to c319f6b Compare March 22, 2019 03:14

zshi-redhat reviewed Mar 22, 2019

View reviewed changes

ahalimx86 force-pushed the dev/kubelet_client branch from 78db614 to f92f2da Compare March 22, 2019 11:27

ahalimx86 force-pushed the dev/kubelet_client branch from f92f2da to 97c195b Compare March 22, 2019 12:34

dougbtv merged commit d3c92b4 into k8snetworkplumbingwg:master Mar 22, 2019

ahalimx86 deleted the dev/kubelet_client branch March 22, 2019 15:06

dougbtv added the release-v3 PRs to make it in release branch label Mar 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add kubelet client for Pod resource info #231

add kubelet client for Pod resource info #231

ahalimx86 commented Jan 8, 2019

coveralls commented Jan 8, 2019 •

edited

rkamudhan commented Jan 8, 2019

dcbw commented Jan 9, 2019

ahalimx86 commented Jan 9, 2019 •

edited

s1061123 commented Jan 11, 2019

ahalimx86 commented Jan 11, 2019

ahalimx86 commented Feb 4, 2019

dcbw commented Feb 21, 2019

rkamudhan left a comment

s1061123 commented Mar 13, 2019

ahalimx86 commented Mar 13, 2019

zshi-redhat commented Mar 13, 2019

ahalimx86 commented Mar 20, 2019

rkamudhan commented Mar 21, 2019

ahalimx86 commented Mar 21, 2019

ahalimx86 commented Mar 21, 2019

zshi-redhat commented Mar 22, 2019 •

edited

zshi-redhat Mar 22, 2019

ahalimx86 Mar 22, 2019

zshi-redhat Mar 22, 2019

zshi-redhat left a comment

zshi-redhat Mar 22, 2019

ahalimx86 Mar 22, 2019

zshi-redhat Mar 22, 2019

ahalimx86 Mar 22, 2019

zshi-redhat Mar 22, 2019

zshi-redhat Mar 22, 2019

ahalimx86 Mar 22, 2019

ahalimx86 commented Mar 22, 2019

zshi-redhat commented Mar 22, 2019

add kubelet client for Pod resource info #231

add kubelet client for Pod resource info #231

Conversation

ahalimx86 commented Jan 8, 2019

coveralls commented Jan 8, 2019 • edited

Pull Request Test Coverage Report for Build 729

💛 - Coveralls

rkamudhan commented Jan 8, 2019

dcbw commented Jan 9, 2019

ahalimx86 commented Jan 9, 2019 • edited

s1061123 commented Jan 11, 2019

ahalimx86 commented Jan 11, 2019

ahalimx86 commented Feb 4, 2019

dcbw commented Feb 21, 2019

rkamudhan left a comment

Choose a reason for hiding this comment

s1061123 commented Mar 13, 2019

ahalimx86 commented Mar 13, 2019

zshi-redhat commented Mar 13, 2019

ahalimx86 commented Mar 20, 2019

rkamudhan commented Mar 21, 2019

ahalimx86 commented Mar 21, 2019

ahalimx86 commented Mar 21, 2019

zshi-redhat commented Mar 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zshi-redhat left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahalimx86 commented Mar 22, 2019

zshi-redhat commented Mar 22, 2019

coveralls commented Jan 8, 2019 •

edited

ahalimx86 commented Jan 9, 2019 •

edited

zshi-redhat commented Mar 22, 2019 •

edited