Add support for cri-resource-manager project #232

obedmr · 2019-10-21T22:29:09Z

This commit adds support for installing and configuring the
cri-resource-manager project as an systemd-based service.

It also adds the automation to configure kubelet service to consume
it as a remote container runtime. Finally, it's providing the automation
for cleaning up the kubelet service configuration to its original state
without cri-resource-manager.

Binary installation will be temporally consumed from a personal fork that
is currently hosting cri-resource-manager binaries in the meantime that
cri-resource-manager generates its packaging strategy.

Signed-off-by: Obed N Munoz obed.n.munoz@intel.com

clr-k8s-examples/10-cri-resource-manager/README.md

grahamwhaley

Hi @obedmr . Left a bunch of comments - half just stylistic markdown stuff, but some queries about a couple of bits of code, and then asking if we can do this without tying to a user account/repo... please :-)

grahamwhaley · 2019-10-29T18:37:50Z

clr-k8s-examples/10-cri-resource-manager/README.md

@@ -0,0 +1,55 @@
+CRI Resource Manager
+====================


Oh, apparently you are using markdown setext heading markers (===== and ------- for instance), and not atx heading markers (#, ## etc.) - which is what I think we use for all other docs in this repo. I didn't even know you could use setext in markdown :-)

grahamwhaley · 2019-10-29T18:41:47Z

clr-k8s-examples/10-cri-resource-manager/install.sh

+CRI_RESMGR_POLICY_OPTIONS=${CRI_RESMGR_POLICY_OPTIONS:-"-dump='reset,full:.*' -dump-file=/tmp/cri.dump"}
+CRI_RESMGR_DEBUG_OPTIONS=${CRI_RESMGR_DEBUG_OPTIONS:-""}
+
+curl https://raw.githubusercontent.com/obedmr/cri-resource-manager/master/godownloader.sh | bash


Ah, I'm going to struggle to ack a script that curls down a binary from a user account and installs it...
Can we try and make this something from either the official CRI-RM repo (or release, if they are maybe released on docker hub for instance), or a 'pull and build from official sources' script?

and, curl | bash is generally just seen as a big security hole.... ;-)

well, CRI-Resource-Manager project's plans for releasing binaries is still on definition, we need something to continue, we may use this and later update when official container images or binaries are released.

I'd like to hold off until CRI-RM gets their binary release method sorted out. I see you have the in-flight intel/cri-resource-manager#55 which creates a deployment style method, which is great. That will require a formal process of pushing a pre-built deployment container image somwhere for CRI-RM.
It would be good to hear if there is some general timeline for CRI-RM binary releases planned - can we ask the CRI-RM team?

grahamwhaley · 2019-10-29T18:43:09Z

clr-k8s-examples/10-cri-resource-manager/README.md

+| `RUNNER`                    | Default Container Runtime                 | `containerd`                                     |
+| `CRI_RESMGR_POLICY`         | CRI Resource Manager Policy type          | `null`                                           |
+| `CRI_RESMGR_POLICY_OPTIONS` | CRI Resource Manager extra policy options | `-dump='reset,full:.*' -dump-file=/tmp/cri.dump` |
+| `CRI_RESMGR_DEBUG_OPTIONS`  | CRI Resource Manager debugging options    |                                                  |


No default value for this last row? - if none, then can we make it say <none> or N/A or similar pls :-)

grahamwhaley · 2019-10-29T18:44:31Z

clr-k8s-examples/10-cri-resource-manager/README.md

+| `CRI_RESMGR_DEBUG_OPTIONS`  | CRI Resource Manager debugging options    |                                                  |
+
+```
+RUNNER=containerd ./install.sh


Looks like you have an example here - can you add an 'example:' text to make that clear etc.
also, maybe make it a ```bash block, and add a $ to the front of the command line to make that clear.

And.... do we say anywhere where to run these scripts? Are they on the master node, or every node, or just workers? It's not clear to me how this is meant to be invoked. Does that need clarification?

Added a comment for installing/configuring in all nodes where it's wanted to have the cri-resmgr service

grahamwhaley · 2019-10-29T18:46:02Z

clr-k8s-examples/10-cri-resource-manager/README.md

+```
+
+- Install verification
+  - Verify that the cri-resource-manager service is actually running.


probably don't need this line as a bullet

agree, removed as bullet.

clr-k8s-examples/10-cri-resource-manager/README.md

grahamwhaley · 2019-10-29T18:48:16Z

clr-k8s-examples/10-cri-resource-manager/README.md

+```
+
+- Setup verification
+  - Kubelet service should be restarted and now using `cri-resource-manager` as its container runtime


is there any sort of kubectl command etc. you can use to verify it is in place? extracting some JSON or something etc.?

Tried with the following ones, but they're still showing the botton final container runtime (i.e. containerd, crio or dockershim)

kubectl cluster-info dump kubectl get nodes -o wide

grahamwhaley · 2019-10-29T18:49:40Z

clr-k8s-examples/10-cri-resource-manager/README.md

+
+More kubernetes native approach (experimental)
+----------------------------------------------
+In case that you're interested in a more Kubernetes native way of deploying the CRI Resource manager, take a look on: https://github.com/obedmr/cri-resource-manager/blob/k8s-native/cmd/cri-resmgr/deployment.yaml


can/will this end up in the main CRI-RM repo - maybe you can add a wiki page or a PR or Issue there? preferable to referencing a user acct...

yes, changing it to my current PR

clr-k8s-examples/10-cri-resource-manager/clean.sh

obedmr · 2019-10-29T20:24:28Z

@grahamwhaley thanks for the feedback, I updated my PR. :)

grahamwhaley

I'm going to NACK this for now, mostly on the grounds of the informal binary releases being referenced for CRI-RM - and, given it looks like there is WIP to get a more formal binary release and install method for CRI-RM in the works.
But, I'd also like to hear input from the clr-k8s-examples owners on the additon of CRI-RM to the stack, and how they'd like to see it etc. /cc @jascott1

grahamwhaley · 2019-10-31T11:45:45Z

clr-k8s-examples/10-cri-resource-manager/install.sh

+CRI_RESMGR_POLICY_OPTIONS=${CRI_RESMGR_POLICY_OPTIONS:-"-dump='reset,full:.*' -dump-file=/tmp/cri.dump"}
+CRI_RESMGR_DEBUG_OPTIONS=${CRI_RESMGR_DEBUG_OPTIONS:-""}
+
+curl https://raw.githubusercontent.com/obedmr/cri-resource-manager/master/godownloader.sh | bash


I'd like to hold off until CRI-RM gets their binary release method sorted out. I see you have the in-flight intel/cri-resource-manager#55 which creates a deployment style method, which is great. That will require a formal process of pushing a pre-built deployment container image somwhere for CRI-RM.
It would be good to hear if there is some general timeline for CRI-RM binary releases planned - can we ask the CRI-RM team?

This commit adds support for installing and configuring the [cri-resource-manager](https://github.com/intel/cri-resource-manager) project as an systemd-based service. It also adds the automation to configure `kubelet` service to consume it as a remote container runtime. Finally, it's providing the automation for cleaning up the `kubelet` service configuration to its original state without `cri-resource-manager`. Binary installation will be temporally consumed from a personal fork that is currently hosting `cri-resource-manager` binaries in the meantime that `cri-resource-manager` generates its packaging strategy. Signed-off-by: Obed N Munoz <obed.n.munoz@intel.com>

obedmr requested a review from dklyle October 21, 2019 22:29

obedmr added the scaling label Oct 21, 2019

obedmr force-pushed the cri-resource-manager branch 6 times, most recently from 623b4b4 to a466723 Compare October 23, 2019 22:32

jascott1 reviewed Oct 24, 2019

View reviewed changes

clr-k8s-examples/10-cri-resource-manager/README.md Outdated Show resolved Hide resolved

clr-k8s-examples/10-cri-resource-manager/README.md Outdated Show resolved Hide resolved

obedmr force-pushed the cri-resource-manager branch from a466723 to 757df25 Compare October 24, 2019 20:15

obedmr requested a review from grahamwhaley October 29, 2019 17:27

grahamwhaley reviewed Oct 29, 2019

View reviewed changes

obedmr force-pushed the cri-resource-manager branch from 757df25 to afa35be Compare October 29, 2019 20:21

obedmr requested a review from grahamwhaley October 29, 2019 20:34

obedmr force-pushed the cri-resource-manager branch from afa35be to b2b0991 Compare October 29, 2019 21:23

grahamwhaley suggested changes Oct 31, 2019

View reviewed changes

obedmr force-pushed the cri-resource-manager branch from b2b0991 to 063b6ea Compare November 26, 2019 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for cri-resource-manager project #232

Add support for cri-resource-manager project #232

obedmr commented Oct 21, 2019

grahamwhaley left a comment

grahamwhaley Oct 29, 2019

obedmr Oct 29, 2019

grahamwhaley Oct 29, 2019

grahamwhaley Oct 29, 2019

obedmr Oct 29, 2019

grahamwhaley Oct 31, 2019

grahamwhaley Oct 29, 2019

obedmr Oct 29, 2019

grahamwhaley Oct 29, 2019

grahamwhaley Oct 29, 2019

obedmr Oct 29, 2019

grahamwhaley Oct 29, 2019

obedmr Oct 29, 2019

grahamwhaley Oct 29, 2019

obedmr Oct 29, 2019

grahamwhaley Oct 29, 2019

obedmr Oct 29, 2019

obedmr commented Oct 29, 2019

grahamwhaley left a comment

grahamwhaley Oct 31, 2019

Add support for cri-resource-manager project #232

Are you sure you want to change the base?

Add support for cri-resource-manager project #232

Conversation

obedmr commented Oct 21, 2019

grahamwhaley 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

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

obedmr commented Oct 29, 2019

grahamwhaley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment