DRA:add example driver #7

hj-johannes-lee · 2022-08-04T14:55:45Z

DRA: add an example driver

pohly

This needs more work. Can you address this in the next few working days?

pohly · 2022-08-04T15:06:00Z

test/integration/cdi/example-driver/kubeletplugin/kubeletplugin.go

+		klog.Infof("Creating CDI Files")
+
+		containerEditsInDevices := &cdiapi.ContainerEdits{}
+		containerEditsInDevices.Append(&cdiapi.ContainerEdits{ContainerEdits: &specs.ContainerEdits{


Instead of hard-coding some content, decode the handle string into a string/string map and set that as environment variables. That's what has been documented as functionality of the example driver in the README and what the controller implements.

Maybe done.! If you think it's done. Please, resolve it.! I am not sure.

pohly · 2022-08-04T15:06:24Z

test/integration/cdi/example-driver/kubeletplugin/server/noderegistrar.go

@@ -0,0 +1,134 @@
+package server


When adding new files, remember to add the copyright header.

pohly · 2022-08-04T15:08:43Z

test/integration/cdi/example-driver/app/server.go

@@ -44,6 +44,7 @@ import (
 	"k8s.io/component-base/metrics/legacyregistry"
 	"k8s.io/component-base/term"
 	"k8s.io/klog/v2"
+	plugin "k8s.io/kubernetes/test/integration/cdi/example-driver/kubeletplugin"


Please refactor the code:

generic functionality belongs under "example-driver" (like "example-driver/controller") and will be moved into a staging repo later

the driver code itself is under "example-driver/app"

Maybe done.! I think the structure is almost same as Kevin's one. example-driver/kubelet-plugin may also move together with example-driver/controller. I do not know where it's moved to, but I understand your point roughly.

Anyway, the directory is for generic functionalities of kubelet-plugin (registration server, registrar, grpc server).
And, the only kubeletplugin.go file in example-driver/app is the part that actually has the example driver functionalities (prepare/unprepare).

If you think it's done. Please, resolve it.! I am not sure.

pohly · 2022-08-04T15:09:59Z

test/integration/cdi/example-driver/kubeletplugin/kubeletplugin.go

+	return nil
+}
+
+var jsonFilePath = "/etc/cdi/vendor.json"


You cannot hard-code a file name like this.

@bart0sh: is there a naming convention that vendors should follow to avoid overwriting each others files?

I got this when I deployed shared-resourceclaim.yaml.
Interestingly, claimNames are actually unique whereas claimUids are not.
Do you think, we can use this for naming the json files?

I0805 02:00:47.574983 2621373 kubeletplugin.go:86] NodePrepareResource is called: request: &NodePrepareResourceRequest{Namespace:default,ClaimUid:094e7ec3-4713-4404-97b3-08ddd180a338,ClaimName:f1f5c13c-e908-4edf-9fbc-71ccbc643995-example,ResourceHandle:{"user_a":"b"},}
I0805 02:00:47.575035 2621373 kubeletplugin.go:94] Creating CDI Files
I0805 02:00:47.575060 2621373 kubeletplugin.go:135] CDI spec is validated
I0805 02:00:47.575197 2621373 kubeletplugin.go:147] Json file is created at /etc/cdi/vendor.json
I0805 02:00:47.575217 2621373 kubeletplugin.go:152] Return: vendor.com/device=example
I0805 02:00:47.580004 2621373 kubeletplugin.go:86] NodePrepareResource is called: request: &NodePrepareResourceRequest{Namespace:default,ClaimUid:094e7ec3-4713-4404-97b3-08ddd180a338,ClaimName:441e23ec-9cac-47d1-898b-b1e879d68646-example,ResourceHandle:{"user_a":"b"},}
I0805 02:00:47.580055 2621373 kubeletplugin.go:94] Creating CDI Files
I0805 02:00:47.580090 2621373 kubeletplugin.go:135] CDI spec is validated
I0805 02:00:47.580274 2621373 kubeletplugin.go:147] Json file is created at /etc/cdi/vendor.json
I0805 02:00:47.580298 2621373 kubeletplugin.go:152] Return: vendor.com/device=example

Maybe done.! It is based on the guese that ClaimName seems unique for each pod though they share same resource claim. I tested when Prepare and Unprepare functions are called.

If you think it's done. Please, resolve it.! I am not sure.

@bart0sh @pohly Please see this log and let me know what to do. Idk why uid is same, and name is unique.

@hj-johannes-lee uid is the same because 2 pods share the same claim.

Claim name is not passed to this call. It's a bug in my code. Here is the fix

Here is a plugin output with the applied fix:

I0806 02:16:14.676485 3776 nodeserver.go:51] NodePrepareResource is called: request: &NodePrepareResourceRequest{Namespace:default,ClaimUid:61e69ce7-e1f7-4d15-9f6f-7c6192ba7a91,ClaimName:shared-claim,ResourceHandle:{"user_a":"b"},} I0806 02:16:14.676552 3776 nodeserver.go:59] Creating CDI Files I0806 02:16:14.676575 3776 nodeserver.go:89] CDI spec is validated I0806 02:16:14.676979 3776 nodeserver.go:101] Json file is created at /etc/cdi/vendor.json I0806 02:16:14.677001 3776 nodeserver.go:106] Return: vendor.com/device=example I0806 02:16:14.684296 3776 nodeserver.go:51] NodePrepareResource is called: request: &NodePrepareResourceRequest{Namespace:default,ClaimUid:61e69ce7-e1f7-4d15-9f6f-7c6192ba7a91,ClaimName:shared-claim,ResourceHandle:{"user_a":"b"},} I0806 02:16:14.684429 3776 nodeserver.go:59] Creating CDI Files I0806 02:16:14.684465 3776 nodeserver.go:89] CDI spec is validated I0806 02:16:14.684733 3776 nodeserver.go:101] Json file is created at /etc/cdi/vendor.json I0806 02:16:14.684756 3776 nodeserver.go:106] Return: vendor.com/device=example I0806 02:16:18.556491 3776 nodeserver.go:112] NodeUnprepareResource is called: request: &NodeUnprepareResourceRequest{Namespace:default,ClaimUid:61e69ce7-e1f7-4d15-9f6f-7c6192ba7a91,ClaimName:shared-claim,CdiDevice:[vendor.com/device=example],} I0806 02:16:19.561129 3776 nodeserver.go:112] NodeUnprepareResource is called: request: &NodeUnprepareResourceRequest{Namespace:default,ClaimUid:61e69ce7-e1f7-4d15-9f6f-7c6192ba7a91,ClaimName:shared-claim,CdiDevice:[vendor.com/device=example],}

Another idea is to make a hash out of parameters and use that as a suffix. This would complicate your code a little bit though as you should keep track of used file names and only delete them when processing the last NodeUnprepareResource call for the same resource.

Why is NodePrepareResource called twice when it is the same claim? It should only be called once!

@pohly should be fixed in #9

Here is a plugin output fore the shared claim example with changes from #9:

I0809 00:31:42.190905 11002 server.go:106] Found KUBECONFIG environment variable set, using that.. I0809 00:31:42.194965 11002 noderegistrar.go:49] Starting Registration Server at: /var/lib/kubelet/plugins_registry/example-driver.cdi.k8s.io-reg.sock I0809 00:31:42.195060 11002 noderegistrar.go:58] Registration Server started at: /var/lib/kubelet/plugins_registry/example-driver.cdi.k8s.io-reg.sock I0809 00:31:43.710827 11002 registrationserver.go:31] Received GetInfo call: &InfoRequest{} I0809 00:31:43.711683 11002 registrationserver.go:42] Received NotifyRegistrationStatus call: &RegistrationStatus{PluginRegistered:true,Error:,} I0809 00:31:54.967270 11002 nodeserver.go:51] NodePrepareResource is called: request: &NodePrepareResourceRequest{Namespace:default,ClaimUid:0df67e02-93ff-4e14-8f00-c815af4f511f,ClaimName:shared-claim,ResourceHandle:{"user_a":"b"},} I0809 00:31:54.967332 11002 nodeserver.go:59] Creating CDI Files I0809 00:31:54.967363 11002 nodeserver.go:89] CDI spec is validated I0809 00:31:54.967747 11002 nodeserver.go:101] Json file is created at /etc/cdi/vendor.json I0809 00:31:54.967767 11002 nodeserver.go:106] Return: vendor.com/device=example I0809 00:31:58.788329 11002 nodeserver.go:112] NodeUnprepareResource is called: request: &NodeUnprepareResourceRequest{Namespace:default,ClaimUid:0df67e02-93ff-4e14-8f00-c815af4f511f,ClaimName:shared-claim,CdiDevice:[vendor.com/device=example],}

@bart0sh Thanks! :)

pohly · 2022-08-05T06:38:30Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+*/
+
+// Package app does all of the work necessary to configure and run a
+// Kubernetes app process.


Please remove this comment. We would need to rewrite it and if we wanted to, it should be in a stand-alone doc.go file.

AH, i thought I checked all comments, but still there were unnecessary things. Let me check once more.

pohly · 2022-08-05T06:42:22Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+func newExampleDriver(config pluginConfig) (*examplePlugin, error) {
+
+	if err := os.MkdirAll(filepath.Dir(config.draAddress), 0750); err != nil {
+		return nil, err


General remark about error handling:

Always wrap errors with fmt.Errorf("do something: %v", err) unless it is absolutely certain that err itself already contains all information that a user or developer might need. In this case, os guarantees that the file path is included, but it might not be clear what the code was trying to do, so fmt.Errorf("create socket directory: %v", err) would be better.

Don't log errors that get returned to the caller. The caller gets to decide how to report the error to the user, not the callee. This isn't done here, but I've seen it below.

Creating this directory in the constructor feels weird. startPlugin looks like a better place for this.

pohly · 2022-08-05T06:44:30Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+	}), nil
+}
+
+func startPlugin(config *pluginConfig) error {


Why is this a stand-alone function and not a method of examplePlugin? This seems backwards to me.

yeah, true. let me fix it.

pohly · 2022-08-05T06:46:24Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+	}
+
+	// Run a grpc server for the driver, that listens on draAddress
+	plugin.StartNonblockingGrpcServer(config.draAddress, driver)


Shouldn't the GRPC server get started first before telling kubelet about it?

pohly · 2022-08-05T06:47:32Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+}
+
+var cdiVersion = "0.2.0"
+var kind = "vendor.com/device"


Like the JSON file name, the "kind" also needs to be unique. Work with @bart0sh to figure out how to do this, I am not sure.

These look like constants, not variables.

I agree with @pohly here. 'device' is too generic name. vendor.com is ok.
I'd suggest two things regarding CDI file naming:

get CDI directory from the command line with /var/run/cdi as a default

use this naming convention: <cdi directory>/<vendor>-<device>-<claim uid or something unique>.cdi

We discussed this on Slack.

Let's use <cdi directory>/<driver name>-<claim uid>.json, with <cdi directory being configurable and defaulting to /var/run/cdi. Inside the file, let's use k8s.io as vendor and <driver name>/exampledevice as kind.

The kind doesn't need to be unique, does it?

it doesn't, but fully qualified device name must be unique. Otherwise runtime wouldn't be able to get CDI file by device fqdn. This is not the case in the current implementation and needs to be fixed.

Nope. Drop the random suffix to simplify idempotency.

@pohly what about kind? You said before <driver name>/example-device, but @bart0sh now says <vendor>/exmaple-device

@pohly @bart0sh I didn't get any answer about 'vendor'?

Please read what you were talking. Who do I need to listen to if you talk different things?

Patrick: kind = /example-device // drivername = vendor
Ed: kind = /example-device // drivername != vendor

What do I need to do? Did I understand wrong?

we're different people, why should we talk the same thing? @pohly is a main reviewer as I've mentioned before.

I didn't get any answer about 'vendor'?

Here is mine in case you've missed it:

[bart0sh](https://github.com/bart0sh) [4 hours ago](https://github.com/pohly/kubernetes/pull/7#discussion_r941174593) vendor is "k8s.io", driverName is "example-driver.cdi." + vendor, kind is vendor+"/exampledevice" in my understanding

pohly · 2022-08-05T06:58:24Z

test/integration/cdi/example-driver/deploy/example/pod-inline.yaml

@@ -1,32 +1,41 @@
+# One external resource claim, one pod, two containers.
+# One container uses resource, one does not.


Please remove changes to the example YAMLs from this PR.

Ah, sorry, I was testing with Ed's one,,

pohly · 2022-08-05T06:59:27Z

test/integration/cdi/example-driver/kubeletplugin/noderegistrar.go

+
+// Package app does all of the work necessary to configure and run a
+// Kubernetes app process.
+package server


Copy-and-paste error. This is not the "server" package...

Add doc.go file where you explain what this package is about.

oops, sorry it's not the matter of copy and paste..
it used to be in 'server' directory under 'kubernetes-plugin' and decided to remove the folder since it seems to be incorrect name. So,, I just forgot to rename it..

pohly · 2022-08-05T07:01:19Z

test/integration/cdi/example-driver/kubeletplugin/noderegistrar.go

+	return false, nil
+}
+
+func StartRegistrar(driverName, draAddress, pluginRegistrationPath string) error {


Documentation?

pohly · 2022-08-05T07:03:21Z

test/integration/cdi/example-driver/kubeletplugin/nonblockinggrpcserver.go

+	s := newNonBlockingGRPCServer()
+
+	s.Start(draAddress, driver)
+	s.Wait()


How can this be non-blocking when it waits for the server to terminate?

Well, that's also from.. csi hostpath..
https://github.com/kubernetes-csi/csi-driver-host-path/blob/master/pkg/hostpath/hostpath.go#L126

Let me check it if I understand..

Yes, Run there is blocking. But here you use it in a functions saying "non-blocking".

Ah, yeah.. it's blocking..

Is it really resolved?

@bart0sh It does not block any more?

ok, resolved then. Thanks.

pohly · 2022-08-05T07:06:35Z

test/integration/cdi/example-driver/kubeletplugin/nonblockinggrpcserver.go

+
+func (s *nonBlockingGRPCServer) Start(endpoint string, ns drapbv1.NodeServer) {
+
+	s.wg.Add(1)


Something seems to haven gotten lost as you copied this code around: where is the corresponding s.wg.Done()?

Did I?
https://github.com/kubernetes-csi/csi-driver-host-path/blob/master/pkg/hostpath/server.go#L43
Maybe,, the example I got from was not really good option...

But, I studed about waitinggroup a bit, and I understand your point.!

I don't know where it went of the rails, but s.wg.Add(1) followed by s.wg.Done() makes no sense. It becomes a no-op. The entire wait group serves no purpose anymore in the current revision of the code.

Perhaps it is not needed (always spawn goroutine, never wait for it), but then the entire code could be simpler.

hj-johannes-lee · 2022-08-05T21:26:41Z

@pohly
Updates:

unresolved things above are left not changed. Those are about drivername, jsonfilepath, etc that are hard coded. Let me change after making it sure that "claimuid" is unique. It seems not.
I set "valid" form of "ResourceHandle" that user can write in yaml files as "env: foo bar". Plugin checks very simply just by seeing whether the key is "user_env" or not. If we are gonna make it more complex (with mounts, hooks, or whatever specific for the example plugin) then I will change it as you think as good. But, as you said, for now it's just handling "env" for now. So, I made it like that.
Pod yaml files should not work with the plugin for now, because it checks 'if the parameters are valid form or not'. Do I update those yamls also?

bart0sh · 2022-08-05T22:41:25Z

@hj-johannes-lee re 2 and 3: can you implement it the way it's described in the README?

hj-johannes-lee · 2022-08-08T13:46:01Z

@bart0sh AH, it's what I meant last time: two types of parameters are user_ and admin_.I didn't remember that the another one has prefix 'admin_'.

But, anyway, as I told, for now, I can see only one type, user_, from the request. Is there any reason why admin_ is not possible to get from the request.GetResourceHandle()?
If you want me to make as it is, I need to get admin_ resourcehandle also from the request?

pohly · 2022-08-08T14:09:02Z

I can see only one type, user_, from the request. ... Is there any reason why admin_ is not possible to get from the request.GetResourceHandle()?

That it is not in any of the current examples does not mean that it isn't possible (cannot draw conclusions from an example).

But there is a reason why it is not in the examples already, and that reason is that the reference to the ConfigMap isn't possible at the moment without a namespace in the parameter reference: kubernetes#111023 (comment)

pohly · 2022-08-09T09:14:19Z

@hj-johannes-lee: can you rebase on top of the latest branch?

test/integration/cdi/example-driver/app/kubeletplugin.go

bart0sh · 2022-08-09T09:43:33Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+		return nil, fmt.Errorf("failed to marshal: %v", err)
+	}
+
+	if err := os.WriteFile(ex.getJsonFilePath(req.ClaimUid), doc, os.FileMode(0644)); err != nil {


It was suggested by CDI devs to use Tempfile to avoid name conflicts. Please, use that.

Thanks! But the document says it's deprecated. So, maybe another recommendation in the document 'os.CreateTemp'.!

yep, use os.CreateTemp or any API that creates temporary file.

Why was this marked as resolved? It's still using os.WriteFile.

Regarding the proposal to use some tempfile function: that makes it harder to ensure that the function is idempotent. Before writing a new temp file, it would have to read all CDI files to check whether a previous invocation already wrote it.

I prefer to have a deterministic name that is guaranteed (by convention) to be unique. If no such convention exists, then CDI should define one.

makes sense to me.

@pohly Because,, you are writing hundreds of comments, I cannot see what is what if I do not just resolve after solving in my local env.

Do you mean, we need to still use writefile?
getJsonFilePath will return the name you suggested earlier.

Because,, you are writing hundreds of comments

Just pick up one conversation, fix it. Push the code and only after that resolve. You don't need to resolve them all at once. Alternatively we can agree that reviewers will resolve them when they think they are resolved.

Ok, from now on, I will never click 'Resolve'. Either of you resolve please!

bart0sh · 2022-08-09T09:44:56Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+		klog.Infof("Json file " + ex.getJsonFilePath(req.ClaimUid) + " is removed")
+	} else if errors.Is(err, os.ErrNotExist) {
+		// jsonFile does not exist
+		klog.Infof("Json file " + ex.getJsonFilePath(req.ClaimUid) + " is already deleted")


That should be a warning I believe. The fact that it's deleted signals that something went wrong.

I see. It's a bit difficult to know what to put. There are two many options, and, to be honest, documents do not help that much no matter how many times I read... Thanks!

I'd suggest not to overcomplicate things here. It's an example code. I'd do something like this:

if err := WriteFile(path); err != nil { return nil, fmt.Errorf("error creating CDI file %s: %v", path, err) }

err will contain more info about the reason.

Not resolved either?

@pohly You will see a bit later if I already clicked "resolve". If I keep it open, this page is ..... too long

But if it is marked resolved without me checking it first, then I won't know that I still need to check it because GitHub hides the discussion thread.

bart0sh · 2022-08-09T09:46:07Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+		// jsonFile does not exist
+		klog.Infof("Json file " + ex.getJsonFilePath(req.ClaimUid) + " is already deleted")
+	} else {
+		return nil, fmt.Errorf("unexpected error: %v", err)


give more details here, please. 'Unexpected error' doesn't explain much.

I do actually not know what can go there..
File is neither 'existing', nor 'not-existing'.. So, I thought the error itself would be 'unexpected error'.

Do you have an idea?

you're trying to write a CDI file and getting unexpected error. How about fmt.Errorf("Unexpected error writing CDI file %s: %v", path, err)?

Aha!! Thanks! But, this part is not 'writing' but 'checking fileinfo', and I think path is not necessary since os.Stat() returns '*PathError'!

Error messages should start with a lowercase letter. Log messages should start with uppercase.

bart0sh · 2022-08-09T09:46:54Z

test/integration/cdi/example-driver/app/server.go

@@ -77,6 +77,7 @@ func NewCommand() *cobra.Command {

 	fs = sharedFlagSets.FlagSet("CDI")
 	driverName := fs.String("drivername", "example-driver.cdi.k8s.io", "Resource driver name.")
+	cdiDir := fs.String("cdi-dir", "/var/run/cdi", "CDI directory where the driver sockets and CDI files will be")


"/var/run/cdi" should be a constant.

Umm, sorry??
What you said before:

get CDI directory from the command line with /var/run/cdi as a default

What @pohly said right below your comment:

with <cdi directory being configurable and defaulting to /var/run/cdi

In go language, is there a way to make it constant after getting the value from the user...?

I'm talking about "/var/run/cdi", not about cdiDir.

please use this constant: https://github.com/container-orchestrated-devices/container-device-interface/blob/master/pkg/cdi/spec-dirs.go#L30

Ah, I got it, now. Thanks!! :)

not resolved, but marked as resolved again!

bart0sh · 2022-08-09T09:49:42Z

test/integration/cdi/example-driver/app/server.go

+	kubeletPluginFlagSets := cliflag.NamedFlagSets{}
+	fs = kubeletPluginFlagSets.FlagSet("kubelet plugin")
+
+	kubeletDir := fs.String("kubelet-dir", "/var/lib/kubelet", "Path to the Kubelet directory.")


please avoid using hardcoded paths. Either import it from the k8s code or at least define your own constant.

Sorry, I think I do not understand what is 'hard code'. Isn't it configurable by users, and the default is /var/lib/kubelet?

I cannot find how to make it configurable.
When I asked before @pohly with the example os.Getenv, he said about 'drivername'.

driverName := fs.String("drivername", "example-driver.cdi.k8s.io", "Resource driver name.")

Isn't it then also.. hard-coded as 'example-driver.cdi.k8s.io'?

define a constant, e.g. const defaultKubeletDir = "/var/lib/kubelet" and use it in the fs.String call. Or, better, import it from k8s if it exists there.

@bart0sh AHA!!!!!! Thanks!

bart0sh · 2022-08-09T09:51:04Z

test/integration/cdi/example-driver/app/server.go

+	fs = kubeletPluginFlagSets.FlagSet("kubelet plugin")
+
+	kubeletDir := fs.String("kubelet-dir", "/var/lib/kubelet", "Path to the Kubelet directory.")
+	draAddress := fs.String("dra-address", *kubeletDir+"/plugins/"+*driverName+"/dra.sock", "Path to the DRA driver socket kubelet will use to issue CDI operations.")


use path.Join

Ah, yeah, I forgot to do for this part though I changed other parts. Thanks!

bart0sh · 2022-08-09T09:51:41Z

test/integration/cdi/example-driver/app/server.go

+
+	kubeletDir := fs.String("kubelet-dir", "/var/lib/kubelet", "Path to the Kubelet directory.")
+	draAddress := fs.String("dra-address", *kubeletDir+"/plugins/"+*driverName+"/dra.sock", "Path to the DRA driver socket kubelet will use to issue CDI operations.")
+	pluginRegistrationPath := fs.String("plugin-registration-path", *kubeletDir+"/plugins_registry", "Path to Kubernetes plugin registration socket.")


the same thing: use path.Join

bart0sh · 2022-08-09T10:19:19Z

@hj-johannes-lee please rebase your branch on top of the latest target branch. Thanks.

bart0sh · 2022-08-09T11:03:42Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+
+func (ex *examplePlugin) NodePrepareResource(ctx context.Context, req *drapbv1.NodePrepareResourceRequest) (*drapbv1.NodePrepareResourceResponse, error) {
+
+	const (


please move constants to the top of the file

bart0sh · 2022-08-09T11:30:25Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+
+	//return qualified name of the cdi device
+	dev := spec.GetDevice(deviceName).GetQualifiedName()
+	klog.Infof("Return: " + dev)


this is misleading. you're not returning dev, you're returning NodePrepareResourceResponse.

... and don't log debug information at klog.Infof = klog.V(0).Infof. That's way too verbose.

Also, use contextual logging as in my code.

the same here. this is not resolved.

bart0sh · 2022-08-09T11:32:44Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+		file.Close()
+		return nil, fmt.Errorf("failed to write json file: %v", err)
+	}
+	if err := file.Close(); err != nil {


use defer file.Close() golang idiom.

Please stop resolving conversation before submitting a code that fixes it! Thanks.

bart0sh · 2022-08-09T12:42:07Z

So, please resolve them if you think they are resolved.

I don't see "Resolve" button. @pohly can do that I think.

bart0sh · 2022-08-09T12:51:15Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+	}
+
+	// create ContainerEdits in device spec and ContainerEdits
+	containerEditsInDeviceSpec := &cdiapi.ContainerEdits{ContainerEdits: &specs.ContainerEdits{}}


do we need this variable? Can we just initialise the whole spec below, where we call cdiapi.NewSpec?

bart0sh · 2022-08-09T12:52:03Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+	containerEditsInDeviceSpec.Append(&cdiapi.ContainerEdits{ContainerEdits: &specs.ContainerEdits{
+		Env: envs,
+	}})
+	containerEdits := &cdiapi.ContainerEdits{ContainerEdits: &specs.ContainerEdits{}}


This one can be also dropped, I believe.

hj-johannes-lee · 2022-08-09T12:52:10Z

@pohly I cannot know what I need to see any more because all are unresolved. Please resolve them, if you think they are resolved.

bart0sh · 2022-08-09T12:53:22Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+	// create bytes from spec, which would be written into the json file
+	jsonBytes, err := json.Marshal(*spec)
+	if err != nil {
+		return nil, fmt.Errorf("failed to marshal: %v", err)


failed to marshal what? please, be more specific.

bart0sh · 2022-08-09T12:54:21Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+	// create json files
+	filePath := ex.getJsonFilePath(req.ClaimUid)
+	if err := os.WriteFile(filePath, jsonBytes, os.FileMode(0644)); err != nil {
+		return nil, fmt.Errorf("failed to write file %v", err)


I'd suggest to use 'CDI file' instead of just file in error messages. This is more descriptive in my pov.

bart0sh · 2022-08-09T12:54:39Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+	if err := os.WriteFile(filePath, jsonBytes, os.FileMode(0644)); err != nil {
+		return nil, fmt.Errorf("failed to write file %v", err)
+	}
+	klog.Infof("Json file is created at " + filePath)


CDI file is created

bart0sh · 2022-08-09T13:03:36Z

test/integration/cdi/example-driver/kubeletplugin/nonblockinggrpcserver.go

+	s := newNonBlockingGRPCServer()
+
+	s.Start(draAddress, driver)
+	s.Wait()


ok, resolved then. Thanks.

pohly · 2022-08-09T13:06:58Z

@pohly I cannot know what I need to see any more because all are unresolved. Please resolve them, if you think they are resolved.

I cannot resolve either. What you can do is give the person you are talking with a chance to acknowledge that your response answers the question (via comment or a thumbs up reaction), then resolve it.

bart0sh · 2022-08-09T13:07:56Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+
+	// create spec
+	spec, err := cdiapi.NewSpec(
+		&specs.Spec{


I'd make this a separate variable as its content is quite big, which makes NewSpec call less readable, e.g.

spec := spec.Spec{ .... ... } cdiSpec, err := cdiapi.NewSpec(&spec) ...

bart0sh · 2022-08-09T13:11:08Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+
+	dev := spec.GetDevice(deviceName).GetQualifiedName()
+	res := &drapbv1.NodePrepareResourceResponse{CdiDevice: []string{dev}}
+	klog.V(5).Infof("Returned: %s", res.String())


I'd use something like this to match the starting message for this call:

klog.Infof("NodePrepareResource is successfuly finished, response: %+v", resp)

hj-johannes-lee · 2022-08-09T13:24:48Z

@pohly, @bart0sh As I told, I won't resolve anything. If I miss something because it's super messy from 'everything-is-unresolved' condition, then please just make a comment again to make me solve. I have no energy to see from the beginning to the end every time.

bart0sh · 2022-08-09T13:29:48Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+}
+
+// newExamplePlugin returns an initialized examplePlugin instance.
+func newExamplePlugin(config examplePlugin) (*examplePlugin, error) {


so, you're passing examplePlugin struct as a parameter to create examplePlugin struct? This looks quite strange to be honest. Can you just pass 4 parameters instead?

There's actually a good argument for passing a struct: it's easy to mix up the order of parameters when a function takes four parameters of the same type (strings in this case). Optional parameters are easy with a struct and impossible with args.

No need to change this again, I just wanted to point it out.

bart0sh · 2022-08-09T13:31:39Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+
+	klog.Infof("NodePrepareResource is called: request: %+v", req)
+
+	klog.Infof("Creating CDI Files")


Do we create multiple CDI files?

I, from the begining, asked multiple times

why you say the plugin generates CDI fileS though we create only JSON file.

Is there any other file I need to generate?

I got no answer, and kept it like that?

There is only one file.

Yes, changed.

bart0sh · 2022-08-09T13:35:39Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+	}
+	cdiSpec, err := cdiapi.NewSpec(
+		&spec,
+		ex.getJsonFilePath(req.ClaimUid), 1)


nit: I'd put all parameters in one line. This way error checking is closer to the call and more readable.

bart0sh · 2022-08-09T17:01:05Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+
+func (ex *examplePlugin) NodePrepareResource(ctx context.Context, req *drapbv1.NodePrepareResourceRequest) (*drapbv1.NodePrepareResourceResponse, error) {
+
+	kind := ex.driverName + "/" + deviceName


please move this variable lower, where it's used or get rid of it.

I don't think we should use driver name as a vendor. We don't validate it, so any string can be passed as a driver name to the plugin with a command line parameter. If we put it as a vendor into the CDI file it might cause validation errors.

bart0sh · 2022-08-09T17:03:33Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+			ContainerEdits: specs.ContainerEdits{
+				Env: envs,
+			},
+		}},


As we don't have any devices here can we skip this and set env vars only in the global container edits section below?

bart0sh · 2022-08-09T17:29:03Z

test/integration/cdi/example-driver/app/kubeletplugin.go

+	}
+	klog.V(5).InfoS("CDI spec is validated")
+
+	// create bytes from spec, which would be written into the json file


There is a cdiSpec.Write() API that does exactly what we need here. I've asked CDI maintainer to tag it. After that we can use it here.

pohly · 2022-08-09T17:49:33Z

@hj-johannes-lee: I know that you have worked hard on this, but we are not making progress quickly enough with the current approach (you write some code, @bart0sh and I comment, you revise it again, etc.).

Let's try something else: I'll take your latest revision (90dd92e) and then I'll go through everything and make all changes that I consider necessary or useful. When I am done, I can post that as a separate commit on top of yours and then we can discuss why I made changes in those case where it isn't obvious before squashing.

pohly requested changes Aug 4, 2022

View reviewed changes

hj-johannes-lee force-pushed the dra-example-driver branch from 22c6c74 to 30891b6 Compare August 5, 2022 00:36

pohly requested changes Aug 5, 2022

View reviewed changes

hj-johannes-lee force-pushed the dra-example-driver branch from 30891b6 to 8d2e35b Compare August 5, 2022 21:15

hj-johannes-lee force-pushed the dra-example-driver branch 2 times, most recently from 58c50f5 to 2989811 Compare August 9, 2022 01:03

pohly force-pushed the dynamic-resource-allocation branch from 1f00794 to ca95a8a Compare August 9, 2022 06:21

bart0sh reviewed Aug 9, 2022

View reviewed changes

test/integration/cdi/example-driver/app/kubeletplugin.go Outdated Show resolved Hide resolved

bart0sh reviewed Aug 9, 2022

View reviewed changes

test/integration/cdi/example-driver/app/kubeletplugin.go Outdated Show resolved Hide resolved

bart0sh reviewed Aug 9, 2022

View reviewed changes

test/integration/cdi/example-driver/app/kubeletplugin.go Outdated Show resolved Hide resolved

bart0sh reviewed Aug 9, 2022

View reviewed changes

test/integration/cdi/example-driver/app/kubeletplugin.go Outdated Show resolved Hide resolved

bart0sh reviewed Aug 9, 2022

View reviewed changes

test/integration/cdi/example-driver/app/kubeletplugin.go Outdated Show resolved Hide resolved

bart0sh reviewed Aug 9, 2022

View reviewed changes

test/integration/cdi/example-driver/app/kubeletplugin.go Outdated Show resolved Hide resolved

bart0sh reviewed Aug 9, 2022

View reviewed changes

hj-johannes-lee force-pushed the dra-example-driver branch from 2989811 to c6272fe Compare August 9, 2022 11:12

bart0sh reviewed Aug 9, 2022

View reviewed changes

hj-johannes-lee force-pushed the dra-example-driver branch from 0bbdc59 to bbe3a7a Compare August 9, 2022 12:41

hj-johannes-lee force-pushed the dra-example-driver branch from bbe3a7a to 9b98702 Compare August 9, 2022 12:43

bart0sh reviewed Aug 9, 2022

View reviewed changes

hj-johannes-lee force-pushed the dra-example-driver branch from 9b98702 to 6bce091 Compare August 9, 2022 12:55

bart0sh suggested changes Aug 9, 2022

View reviewed changes

bart0sh reviewed Aug 9, 2022

View reviewed changes

hj-johannes-lee force-pushed the dra-example-driver branch 2 times, most recently from 3210390 to 42511d5 Compare August 9, 2022 13:27

bart0sh suggested changes Aug 9, 2022

View reviewed changes

hj-johannes-lee force-pushed the dra-example-driver branch 3 times, most recently from 89422ef to 90dd92e Compare August 9, 2022 13:47

bart0sh reviewed Aug 9, 2022

View reviewed changes

pohly mentioned this pull request Aug 10, 2022

kubelet plugin #12

Closed

pohly force-pushed the dynamic-resource-allocation branch 3 times, most recently from d4c9729 to d9b1636 Compare August 12, 2022 16:32

hj-johannes-lee closed this Aug 15, 2022

		@@ -1,32 +1,41 @@
		# One external resource claim, one pod, two containers.
		# One container uses resource, one does not.


		func (s *nonBlockingGRPCServer) Start(endpoint string, ns drapbv1.NodeServer) {

		s.wg.Add(1)


		func (ex examplePlugin) NodePrepareResource(ctx context.Context, req drapbv1.NodePrepareResourceRequest) (*drapbv1.NodePrepareResourceResponse, error) {

		const (


		klog.Infof("NodePrepareResource is called: request: %+v", req)

		klog.Infof("Creating CDI Files")


		func (ex examplePlugin) NodePrepareResource(ctx context.Context, req drapbv1.NodePrepareResourceRequest) (*drapbv1.NodePrepareResourceResponse, error) {

		kind := ex.driverName + "/" + deviceName

DRA:add example driver #7

DRA:add example driver #7

Conversation

hj-johannes-lee commented Aug 4, 2022 • edited Loading

pohly 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

hj-johannes-lee Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hj-johannes-lee Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

hj-johannes-lee Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bart0sh Aug 5, 2022 • edited Loading

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

hj-johannes-lee Aug 5, 2022 • edited Loading

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

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

hj-johannes-lee Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

hj-johannes-lee Aug 9, 2022 • edited Loading

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

hj-johannes-lee Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hj-johannes-lee Aug 5, 2022 • edited Loading

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

hj-johannes-lee Aug 9, 2022 • edited Loading

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

hj-johannes-lee commented Aug 5, 2022 • edited Loading

bart0sh commented Aug 5, 2022

hj-johannes-lee commented Aug 8, 2022 • edited Loading

pohly commented Aug 8, 2022

pohly commented Aug 9, 2022

Choose a reason for hiding this comment

hj-johannes-lee Aug 9, 2022 • edited Loading

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

bart0sh Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hj-johannes-lee commented Aug 4, 2022 •

edited

Loading

hj-johannes-lee Aug 5, 2022 •

edited

Loading

hj-johannes-lee Aug 4, 2022 •

edited

Loading

hj-johannes-lee Aug 5, 2022 •

edited

Loading

bart0sh Aug 5, 2022 •

edited

Loading

hj-johannes-lee Aug 5, 2022 •

edited

Loading

hj-johannes-lee Aug 9, 2022 •

edited

Loading

hj-johannes-lee Aug 9, 2022 •

edited

Loading

hj-johannes-lee Aug 5, 2022 •

edited

Loading

hj-johannes-lee Aug 5, 2022 •

edited

Loading

hj-johannes-lee Aug 9, 2022 •

edited

Loading

hj-johannes-lee commented Aug 5, 2022 •

edited

Loading

hj-johannes-lee commented Aug 8, 2022 •

edited

Loading

hj-johannes-lee Aug 9, 2022 •

edited

Loading

bart0sh Aug 9, 2022 •

edited

Loading

hj-johannes-lee Aug 9, 2022 •

edited

Loading

hj-johannes-lee Aug 9, 2022 •

edited

Loading