Skip to content

OCPNODE-1515 : Support Evented PLEG feature in Openshift #1458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sairameshv wants to merge 2 commits into from
Closed

OCPNODE-1515 : Support Evented PLEG feature in Openshift #1458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cdf627
Support evented pleg feature in the Openshift
sairameshv Jan 24, 2024
8b0fb6b
Enable evented pleg flag
sairameshv Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .../0000_10_config-operator_01_node.crd.yaml → ..._config-operator_01_node-Default.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ metadata:
include.release.openshift.io/ibm-cloud-managed: "true"
include.release.openshift.io/self-managed-high-availability: "true"
include.release.openshift.io/single-node-developer: "true"
release.openshift.io/feature-set: Default
name: nodes.config.openshift.io
spec:
group: config.openshift.io
Expand Down
67 changes: 67 additions & 0 deletions config/v1/0000_10_config-operator_01_node-TechPreviewNoUpgrade.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
api-approved.openshift.io: https://github.com/openshift/api/pull/1107
include.release.openshift.io/ibm-cloud-managed: "true"
include.release.openshift.io/self-managed-high-availability: "true"
include.release.openshift.io/single-node-developer: "true"
release.openshift.io/feature-set: TechPreviewNoUpgrade
name: nodes.config.openshift.io
spec:
group: config.openshift.io
names:
kind: Node
listKind: NodeList
plural: nodes
singular: node
scope: Cluster
versions:
- name: v1
schema:
openAPIV3Schema:
description: "Node holds cluster-wide information about node specific features. \n Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer)."
type: object
required:
- spec
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
description: spec holds user settable values for configuration
type: object
properties:
cgroupMode:
description: CgroupMode determines the cgroups version on the node
type: string
enum:
- v1
- v2
- ""
eventedPleg:
description: 'eventedPleg enables the event based PLEG between the kubelet and CRI-O Reference: https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/3386-kubelet-evented-pleg/README.md Valid values are `Enabled`, `Disabled` and "" By default, the evented pleg feature is not enabled in the cluster'
type: string
enum:
- Enabled
- Disabled
- ""
workerLatencyProfile:
description: WorkerLatencyProfile determins the how fast the kubelet is updating the status and corresponding reaction of the cluster
type: string
enum:
- Default
- MediumUpdateAverageReaction
- LowUpdateSlowReaction
status:
description: status holds observed values.
type: object
served: true
storage: true
subresources:
status: {}
2 changes: 1 addition & 1 deletion config/v1/stable.node.testsuite.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
apiVersion: apiextensions.k8s.io/v1 # Hack because controller-gen complains if we don't have this
name: "[Stable] Node"
crd: 0000_10_config-operator_01_node.crd.yaml
crd: 0000_10_config-operator_01_node-Default.crd.yaml
tests:
onCreate:
- name: Should be able to create a minimal Node
Expand Down
14 changes: 14 additions & 0 deletions config/v1/techpreview.node.testsuite.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
apiVersion: apiextensions.k8s.io/v1 # Hack because controller-gen complains if we don't have this
name: "[TechPreviewNoUpgrade] Node"
crd: 0000_10_config-operator_01_node-TechPreviewNoUpgrade.crd.yaml
tests:
onCreate:
- name: Should be able to create a minimal Node
initial: |
apiVersion: config.openshift.io/v1
kind: Node
spec: {} # No spec is required for a Node
expected: |
apiVersion: config.openshift.io/v1
kind: Node
spec: {}
3 changes: 2 additions & 1 deletion config/v1/types_feature.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ var FeatureSets = map[FeatureSet]*FeatureGateEnabledDisabled{
with(dynamicResourceAllocation).
with(gateGatewayAPI).
with(maxUnavailableStatefulSet).
without(eventedPleg).
with(eventedPleg).
with(sigstoreImageVerification).
with(gcpLabelsTags).
with(gcpClusterHostedDNS).
Expand Down Expand Up @@ -215,6 +215,7 @@ var defaultFeatures = &FeatureGateEnabledDisabled{
privateHostedZoneAWS,
buildCSIVolumes,
kmsv1,
eventedPleg,
},
Disabled: []FeatureGateDescription{
disableKubeletCloudCredentialProviders, // We do not currently ship the correct config to use the external credentials provider.
Expand Down
19 changes: 19 additions & 0 deletions config/v1/types_node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,14 @@ type NodeSpec struct {
// the status and corresponding reaction of the cluster
// +optional
WorkerLatencyProfile WorkerLatencyProfileType `json:"workerLatencyProfile,omitempty"`

// eventedPleg enables the event based PLEG between the kubelet and CRI-O
// Reference: https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/3386-kubelet-evented-pleg/README.md
Copy link
Copy Markdown
Contributor

@JoelSpeed JoelSpeed Jun 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a user friendly explanation we can include? A link to a KEP is quite a lot of context.

Is the intention when a user enables this feature that CRIO and Kubelet are both configured? Is there explicit configuration required for both?

EventedPLEG is an upstream feature gate, what happens when that is enabled by default?

// Valid values are `Enabled`, `Disabled` and ""
// By default, the evented pleg feature is not enabled in the cluster
Copy link
Copy Markdown
Contributor

@JoelSpeed JoelSpeed Jun 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically have a particular prose we use for this, can you update please

Suggested change
// By default, the evented pleg feature is not enabled in the cluster
// When omitted, this means no opinion and the platform is left to choose a reasonable default, which is subject to change over time.
// The current default value is Disabled.

// +openshift:enable:FeatureSets=TechPreviewNoUpgrade
// +optional
EventedPleg EventedPLEG `json:"eventedPleg"`
}

type NodeStatus struct{}
Expand Down Expand Up @@ -70,6 +78,17 @@ const (
DefaultUpdateDefaultReaction WorkerLatencyProfileType = "Default"
)

// +kubebuilder:validation:Enum=Enabled;Disabled;""
Copy link
Copy Markdown
Contributor

@JoelSpeed JoelSpeed May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are using omitempty which I think you are doing because this is a workload API rather than a configuration API, you don't need to have "" in place, when the field is omitted the validation for the enum will not execute

Copy link
Copy Markdown
Member Author

@sairameshv sairameshv May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, Removed the "" option and I think nodes.config API is a configuration API as this resource is unique cluster-wide

Copy link
Copy Markdown
Contributor

@JoelSpeed JoelSpeed May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a singleton resource or is there one per Node in the cluster? If it's a singleton then yep, it's a configuration API, in which case I would suggest including "" in the enum and dropping omitempty. This improves the discoverability of the API for the user to configure it

Copy link
Copy Markdown
Member Author

@sairameshv sairameshv May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a singleton resource named "cluster" that has some of the node related configs like cgroupMode, workerlatencyprofiles etc.
I agree that removing omitempty improves the discoverability of the API for the user. At the same time, I want to maintain consistency with the other API fields already present which are again optional
WDYT?

Copy link
Copy Markdown
Contributor

@JoelSpeed JoelSpeed May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the same time, I want to maintain consistency with the other API fields already present which are again optional

We tend to say we don't repeat the sins of the past here. We have conventions that evolve over time so new fields should follow current conventions even when they make the field look inconsistent with older fields.

IMO this should be no omitempty, but allow empty string on the enum please.

Copy link
Copy Markdown
Member Author

@sairameshv sairameshv May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with your point, Updated !!

type EventedPLEG string

const (
// Enabled enables the event based pleg between the kubelet and the cri-o
Enabled EventedPLEG = "Enabled"

// Disabled disables the event based pleg between the kubelet and the cri-o
Disabled EventedPLEG = "Disabled"
)

const (
// DefaultNodeStatusUpdateFrequency refers to the "--node-status-update-frequency" of the kubelet in case of DefaultUpdateDefaultReaction WorkerLatencyProfile type
DefaultNodeStatusUpdateFrequency = 10 * time.Second
Expand Down
1 change: 1 addition & 0 deletions config/v1/zz_generated.swagger_doc_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions openapi/generated_openapi/zz_generated.openapi.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions openapi/openapi.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8186,6 +8186,11 @@
"description": "CgroupMode determines the cgroups version on the node",
"type": "string"
},
"eventedPleg": {
"description": "eventedPleg enables the event based PLEG between the kubelet and CRI-O Reference: https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/3386-kubelet-evented-pleg/README.md Valid values are `Enabled`, `Disabled` and \"\" By default, the evented pleg feature is not enabled in the cluster",
"type": "string",
"default": ""
},
"workerLatencyProfile": {
"description": "WorkerLatencyProfile determins the how fast the kubelet is updating the status and corresponding reaction of the cluster",
"type": "string"
Expand Down
60 changes: 60 additions & 0 deletions payload-manifests/crds/0000_10_config-operator_01_node-Default.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
api-approved.openshift.io: https://github.com/openshift/api/pull/1107
include.release.openshift.io/ibm-cloud-managed: "true"
include.release.openshift.io/self-managed-high-availability: "true"
include.release.openshift.io/single-node-developer: "true"
release.openshift.io/feature-set: Default
name: nodes.config.openshift.io
spec:
group: config.openshift.io
names:
kind: Node
listKind: NodeList
plural: nodes
singular: node
scope: Cluster
versions:
- name: v1
schema:
openAPIV3Schema:
description: "Node holds cluster-wide information about node specific features. \n Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer)."
type: object
required:
- spec
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
description: spec holds user settable values for configuration
type: object
properties:
cgroupMode:
description: CgroupMode determines the cgroups version on the node
type: string
enum:
- v1
- v2
- ""
workerLatencyProfile:
description: WorkerLatencyProfile determins the how fast the kubelet is updating the status and corresponding reaction of the cluster
type: string
enum:
- Default
- MediumUpdateAverageReaction
- LowUpdateSlowReaction
status:
description: status holds observed values.
type: object
served: true
storage: true
subresources:
status: {}
67 changes: 67 additions & 0 deletions payload-manifests/crds/0000_10_config-operator_01_node-TechPreviewNoUpgrade.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
api-approved.openshift.io: https://github.com/openshift/api/pull/1107
include.release.openshift.io/ibm-cloud-managed: "true"
include.release.openshift.io/self-managed-high-availability: "true"
include.release.openshift.io/single-node-developer: "true"
release.openshift.io/feature-set: TechPreviewNoUpgrade
name: nodes.config.openshift.io
spec:
group: config.openshift.io
names:
kind: Node
listKind: NodeList
plural: nodes
singular: node
scope: Cluster
versions:
- name: v1
schema:
openAPIV3Schema:
description: "Node holds cluster-wide information about node specific features. \n Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer)."
type: object
required:
- spec
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
description: spec holds user settable values for configuration
type: object
properties:
cgroupMode:
description: CgroupMode determines the cgroups version on the node
type: string
enum:
- v1
- v2
- ""
eventedPleg:
description: 'eventedPleg enables the event based PLEG between the kubelet and CRI-O Reference: https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/3386-kubelet-evented-pleg/README.md Valid values are `Enabled`, `Disabled` and "" By default, the evented pleg feature is not enabled in the cluster'
type: string
enum:
- Enabled
- Disabled
- ""
workerLatencyProfile:
description: WorkerLatencyProfile determins the how fast the kubelet is updating the status and corresponding reaction of the cluster
type: string
enum:
- Default
- MediumUpdateAverageReaction
- LowUpdateSlowReaction
status:
description: status holds observed values.
type: object
served: true
storage: true
subresources:
status: {}
6 changes: 3 additions & 3 deletions payload-manifests/featuregates/featureGate-Default.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,6 @@
{
"name": "DynamicResourceAllocation"
},
{
"name": "EventedPLEG"
},
{
"name": "GCPClusterHostedDNS"
},
Expand Down Expand Up @@ -120,6 +117,9 @@
{
"name": "CloudDualStackNodeIPs"
},
{
"name": "EventedPLEG"
},
{
"name": "ExternalCloudProvider"
},
Expand Down
6 changes: 3 additions & 3 deletions payload-manifests/featuregates/featureGate-LatencySensitive.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@
{
"name": "DynamicResourceAllocation"
},
{
"name": "EventedPLEG"
},
{
"name": "GCPClusterHostedDNS"
},
Expand Down Expand Up @@ -122,6 +119,9 @@
{
"name": "CloudDualStackNodeIPs"
},
{
"name": "EventedPLEG"
},
{
"name": "ExternalCloudProvider"
},
Expand Down
6 changes: 3 additions & 3 deletions payload-manifests/featuregates/featureGate-TechPreviewNoUpgrade.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@
{
"name": "DisableKubeletCloudCredentialProviders"
},
{
"name": "EventedPLEG"
},
{
"name": "MachineAPIOperatorDisableMachineHealthCheckController"
}
Expand Down Expand Up @@ -53,6 +50,9 @@
{
"name": "DynamicResourceAllocation"
},
{
"name": "EventedPLEG"
},
{
"name": "ExternalCloudProvider"
},
Expand Down