Skip to content
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.

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

OCPBUGS-4621: Add non-nil check before comparing if KubeletConfig is fully rendered #190

Merged
merged 1 commit into from
Jan 12, 2023

Conversation

Vincent056
Copy link

Added check for nil pointer before comparing if KubeletConfig is fully rendered。
This is necessary because setting the KubeletConfig object incorrectly can result
in an empty KubeletConfig Spec, which can cause a panic error. This check will prevent
this issue and ensure the comparison is performed safely.
OCPBUGS-4621

Example of an incorrect way of setting autoSizingReserved KubletConfig:

apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
metadata:
  name: dynamic-node
spec:
  autoSizingReserved: true
  machineConfigPoolSelector:
    matchLabels:
      pools.operator.machineconfiguration.openshift.io/worker: ""

Instead, it should be set as follows according to OpenShift Doc:

apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
metadata:
  name: dynamic-node 
spec:
  kubeletConfig:
    autoSizingReserved: true 
  machineConfigPoolSelector:
    matchLabels:
      pools.operator.machineconfiguration.openshift.io/worker: "" 

@openshift-ci openshift-ci bot requested review from mrogers950 and rhmdnd December 9, 2022 02:10
@openshift-ci openshift-ci bot added the approved label Dec 9, 2022
Copy link

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Dec 9, 2022
@jhrozek jhrozek changed the title Add non-nil check before comparing if KubeletConfig is fully rendered OCPBUGS-4621: Add non-nil check before comparing if KubeletConfig is fully rendered Dec 9, 2022
@openshift-ci-robot
Copy link
Collaborator

@Vincent056: This pull request references Jira Issue OCPBUGS-4621, which is invalid:

  • expected the bug to target the "4.13.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Added check for nil pointer before comparing if KubeletConfig is fully rendered。
This is necessary because setting the KubeletConfig object incorrectly can result
in an empty KubeletConfig Spec, which can cause a panic error. This check will prevent
this issue and ensure the comparison is performed safely.
OCPBUGS-4621

Example of an incorrect way of setting autoSizingReserved KubletConfig:

apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
metadata:
 name: dynamic-node
spec:
 autoSizingReserved: true
 machineConfigPoolSelector:
   matchLabels:
     pools.operator.machineconfiguration.openshift.io/worker: ""

Instead, it should be set as follows according to OpenShift Doc:

apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
metadata:
 name: dynamic-node 
spec:
 kubeletConfig:
   autoSizingReserved: true 
 machineConfigPoolSelector:
   matchLabels:
     pools.operator.machineconfiguration.openshift.io/worker: "" 

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.

@jhrozek
Copy link

jhrozek commented Dec 9, 2022

/jira refresh

@openshift-ci-robot
Copy link
Collaborator

@jhrozek: This pull request references Jira Issue OCPBUGS-4621, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.13.0) matches configured target version for branch (4.13.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @xiaojiey

In response to this:

/jira refresh

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.

@openshift-ci openshift-ci bot requested a review from xiaojiey December 9, 2022 12:19
@jhrozek
Copy link

jhrozek commented Dec 9, 2022

/hold
for QE, PX and docs review

Copy link

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Dec 12, 2022
@xiaojiey
Copy link
Collaborator

Verification pass with 4.13.0-0.nightly-2022-12-16-040121 + code in the PR
Steps:

1. Create kubeletconfig:
$ cat kubeletconfig.yaml 
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
metadata:
  name: cpumanager-enabled-1
spec:
  machineConfigPoolSelector:
    matchLabels:
      pools.operator.machineconfiguration.openshift.io/master: ""
  autoSizingReserved: true
  kubeletConfig:
    cpuManagerPolicy: static
    cpuManagerReconcilePeriod: 5s
    systemReserved:
      cpu: 500m
      memory: 1Gi
      ephemeral-storage": 1Gi
---
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
metadata:
  name: cpumanager-enabled-2
spec:
  machineConfigPoolSelector:
    matchLabels:
      pools.operator.machineconfiguration.openshift.io/worker: ""
  autoSizingReserved: true
  kubeletConfig:
    cpuManagerPolicy: static
    cpuManagerReconcilePeriod: 5s
    systemReserved:
      memory: 1Gi
      cpu: 500m
$ oc apply -f kubeletconfig.yaml 
kubeletconfig.machineconfiguration.openshift.io/cpumanager-enabled-1 created
kubeletconfig.machineconfiguration.openshift.io/cpumanager-enabled-2 created
2. Install CO
$ oc get ip
NAME            CSV                           APPROVAL    APPROVED
install-9ch4n   compliance-operator.v0.1.59   Automatic   true
$ oc get pod
NAME                                              READY   STATUS    RESTARTS   AGE
compliance-operator-cd4d9544b-rgjnm               1/1     Running   0          11m
ocp4-openshift-compliance-pp-844974966c-nvgnb     1/1     Running   0          11m
rhcos4-openshift-compliance-pp-669547f86f-dkf89   1/1     Running   0          11m
3. Create ssb and check no panic error for compliance operator:
$ oc compliance bind -N test -S default-auto-apply profile/ocp4-cis
Creating ScanSettingBinding test
$ oc get suite -w
NAME   PHASE     RESULT
test   RUNNING   NOT-AVAILABLE
test   AGGREGATING   NOT-AVAILABLE
test   DONE          NON-COMPLIANT
test   DONE          NON-COMPLIANT
^C
$ oc get mcp -w
NAME     CONFIG                                             UPDATED   UPDATING   DEGRADED   MACHINECOUNT   READYMACHINECOUNT   UPDATEDMACHINECOUNT   DEGRADEDMACHINECOUNT   AGE
master   rendered-master-646669d933f9e59774d5119c2fe599e1   False     True       False      3              0                   0                     0                      6h30m
worker   rendered-worker-a0c504b81c3e4415c45b1f662de8205f   False     True       False      3              0                   0                     0                      6h30m
...
master   rendered-master-5f1997ccd26038db482f95ac15a46f28   True      False      False      3              3                   3                     0                      6h44m
worker   rendered-worker-aa35fa0cba37004c4eb6db8116985168   True      False      False      3              3                   3                     0                      6h44m
^C
$ oc get pod
NAME                                              READY   STATUS    RESTARTS   AGE
compliance-operator-cd4d9544b-rpvqn               1/1     Running   0          5m13s
ocp4-openshift-compliance-pp-844974966c-n9gmb     1/1     Running   0          5m13s
rhcos4-openshift-compliance-pp-669547f86f-55q7k   1/1     Running   0          5m16s
$ oc logs pod/compliance-operator-cd4d9544b-rpvqn --all-containers > co.log  &
[1] 201589
[1]+  Done                    oc logs pod/compliance-operator-cd4d9544b-rpvqn --all-containers > co.log
$ grep -i "panic" co.log 
$

@xiaojiey
Copy link
Collaborator

/qe-approved

@xiaojiey
Copy link
Collaborator

/unhold

CHANGELOG.md Show resolved Hide resolved
Copy link

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 12, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jan 12, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhrozek, Vincent056

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit fca9cb4 into ComplianceAsCode:master Jan 12, 2023
@openshift-ci-robot
Copy link
Collaborator

@Vincent056: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-4621 has been moved to the MODIFIED state.

In response to this:

Added check for nil pointer before comparing if KubeletConfig is fully rendered。
This is necessary because setting the KubeletConfig object incorrectly can result
in an empty KubeletConfig Spec, which can cause a panic error. This check will prevent
this issue and ensure the comparison is performed safely.
OCPBUGS-4621

Example of an incorrect way of setting autoSizingReserved KubletConfig:

apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
metadata:
 name: dynamic-node
spec:
 autoSizingReserved: true
 machineConfigPoolSelector:
   matchLabels:
     pools.operator.machineconfiguration.openshift.io/worker: ""

Instead, it should be set as follows according to OpenShift Doc:

apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
metadata:
 name: dynamic-node 
spec:
 kubeletConfig:
   autoSizingReserved: true 
 machineConfigPoolSelector:
   matchLabels:
     pools.operator.machineconfiguration.openshift.io/worker: "" 

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants