Add Node API in config/v1 #1107
Conversation
|
/hold |
571a5c3 to
e8c2872
Compare
|
/test verify |
2c68e6b to
94ca794
Compare
|
/hold cancel |
|
/lgtm This is what we agreed to in the enhancement. |
|
/hold till #1114 is addressed. |
Removing the hold because the issue is not related to PR. It's reproducible even without the changes from this RP. /hold cancel |
config/v1/types_node.go
Outdated
| CgroupMode_Empty = "" // Empty string will always use the Default | ||
| CgroupMode_v1 = "v1" | ||
| CgroupMode_v2 = "v2" | ||
| CgroupMode_Default = CgroupMode_v1 |
There was a problem hiding this comment.
Don't use _ in names, we rarely do that, instead use CGroupV1, CGroupV2. I'd suggest dropping CgroupMode_Default entirely, same with CgroupMode_empty both are expressed as empty/not set. No need to add these as constants. Just be explicit that when not set default v1 is used on the field itself.
There was a problem hiding this comment.
Same comment as before. cgroup v2 is the name, and the correct name is Cgroup.
There was a problem hiding this comment.
This naming is carried over from the enhancement... The naming should stay the same.
There was a problem hiding this comment.
Fine with the naming, but the part about empty vs default still applies, both mean the exact same thing so I'd only add v1 and v2.
There was a problem hiding this comment.
@deads2k mentioned adding support for empty and default.
There was a problem hiding this comment.
@deads2k mentioned adding support for empty and default.
Please keep empty. Empty avoids pointers, allows an empty object to succeed, and clearly distinguishes between "choose a good default for me" and "I definitely want v1".
There was a problem hiding this comment.
There are still 3 nits to get it merged:
CgroupModeEmptyinstead ofCgroupMode_Emptyto follow all the other types we have, ie. don't use_in the name- you need to explicitly tell that
CgroupModeEmptyis ofCgroupModeiow.CgroupModeEmpty CgroupMode = ""w/o it's a string and not the type you wanted it to be - rename
CgroupModetoCgroupModeTypeto follow other types like this.
@rphillips I was told to let you know about it 😄
|
/label px-approved |
|
@soltysh: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/label qe-approved |
6e8112d to
13762ae
Compare
Signed-off-by: Harshal Patil <harpatil@redhat.com> Signed-off-by: Ryan Phillips <rphillip@redhat.com> Signed-off-by: Swarup Ghosh <swghosh@redhat.com>
|
@harche: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: harche, rphillips, soltysh, swghosh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/label docs-approved |
This PR adds the Node API in config/v1 that holds cluster-wide information about node specific features.
Node API was proposed in the cgroups v2 enhancement and later further modified in WorkerLatencyProfile enhancement to suite those use cases.
Signed-off-by: Harshal Patil harpatil@redhat.com
Signed-off-by: Ryan Phillips rphillip@redhat.com
Signed-off-by: Swarup Ghosh swghosh@redhat.com