OCPBUGS-45860: daemon: add nil check for annotation fetching

[yuqi-zhang]

Theoretically this shouldn't be empty by the time it gets to here, but in the error stacktrace: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/periodic-ci-openshift-release-master-nightly-4.18-e2e-vsphere-ovn-upi-serial/1861922894817267712/artifacts/e2e-vsphere-ovn-upi-serial/gather-extra/artifacts/pods/openshift-machine-config-operator_machine-config-daemon-4mzxf_machine-config-daemon_previous.log we see this function panicking.

This seems to have actually been the second place where we hit this function in that logic loop? i.e. loadNodeAnnotations -> getNodeAnnotation -> getNodeAnnotationExt should have panicked if either node or node.Annotations is empty, but for some reason it didn't. Weird. Also the fact that we have not seen this before makes it extra odd.

In either case, for safety reasons, we shouldn't be attempting to dereference a potentially nil pointer anyways, so let's just make this quick fix.

[openshift-ci-robot]

@yuqi-zhang: This pull request references Jira Issue OCPBUGS-45860, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.19.0) matches configured target version for branch (4.19.0)
• bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @sergiordlr

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

In response to this:

Theoretically this shouldn't be empty by the time it gets to here, but in the error stacktrace: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/periodic-ci-openshift-release-master-nightly-4.18-e2e-vsphere-ovn-upi-serial/1861922894817267712/artifacts/e2e-vsphere-ovn-upi-serial/gather-extra/artifacts/pods/openshift-machine-config-operator_machine-config-daemon-4mzxf_machine-config-daemon_previous.log we see this function panicking.

This seems to have actually been the second place where we hit this function in that logic loop? i.e. loadNodeAnnotations -> getNodeAnnotation -> getNodeAnnotationExt should have panicked if either node or node.Annotations is empty, but for some reason it didn't. Weird. Also the fact that we have not seen this before makes it extra odd.

In either case, for safety reasons, we shouldn't be attempting to dereference a potentially nil pointer anyways, so let's just make this quick fix.

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 openshift-eng/jira-lifecycle-plugin repository.

[ptalgulk01]

For verification need the PR to get merged, will verify when ON_QA.

/label qe-approved

[openshift-ci-robot]

@yuqi-zhang: This pull request references Jira Issue OCPBUGS-45860, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.19.0) matches configured target version for branch (4.19.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @sergiordlr

In response to this:

Theoretically this shouldn't be empty by the time it gets to here, but in the error stacktrace: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/periodic-ci-openshift-release-master-nightly-4.18-e2e-vsphere-ovn-upi-serial/1861922894817267712/artifacts/e2e-vsphere-ovn-upi-serial/gather-extra/artifacts/pods/openshift-machine-config-operator_machine-config-daemon-4mzxf_machine-config-daemon_previous.log we see this function panicking.

This seems to have actually been the second place where we hit this function in that logic loop? i.e. loadNodeAnnotations -> getNodeAnnotation -> getNodeAnnotationExt should have panicked if either node or node.Annotations is empty, but for some reason it didn't. Weird. Also the fact that we have not seen this before makes it extra odd.

In either case, for safety reasons, we shouldn't be attempting to dereference a potentially nil pointer anyways, so let's just make this quick fix.

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 openshift-eng/jira-lifecycle-plugin repository.

[umohnani8]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: umohnani8, yuqi-zhang

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:

• OWNERS [umohnani8,yuqi-zhang]

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

[djoshy]

This seems to have actually been the second place where we hit this function in that logic loop? i.e. loadNodeAnnotations -> getNodeAnnotation -> getNodeAnnotationExt should have panicked if either node or node.Annotations is empty, but for some reason it didn't. Weird. Also the fact that we have not seen this before makes it extra odd.

Right, this should only be happening if somehow the initial loadNodeAnnotations call is mutating the node object(making the node itself nil or making it a node with empty annotations), thereby updating dn.Node prior to the call here. Weiiiiird. Could we be somehow hitting this deletion bit without actually setting any annotations?

For the record, I'm happy with adding a nil check here, it definitely can't hurt! Just wonder what we are missing 🤔

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD 6778953 and 2 for PR HEAD a49c33d in total

[openshift-ci]

@yuqi-zhang: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-azure-ovn-upgrade-out-of-change	a49c33d	link	false	/test e2e-azure-ovn-upgrade-out-of-change
ci/prow/okd-scos-e2e-aws-ovn	a49c33d	link	false	/test okd-scos-e2e-aws-ovn
ci/prow/e2e-vsphere-ovn-upi	a49c33d	link	false	/test e2e-vsphere-ovn-upi

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

[openshift-ci-robot]

@yuqi-zhang: Jira Issue OCPBUGS-45860: All pull requests linked via external trackers have merged:

• openshift/machine-config-operator#4740

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

In response to this:

Theoretically this shouldn't be empty by the time it gets to here, but in the error stacktrace: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/periodic-ci-openshift-release-master-nightly-4.18-e2e-vsphere-ovn-upi-serial/1861922894817267712/artifacts/e2e-vsphere-ovn-upi-serial/gather-extra/artifacts/pods/openshift-machine-config-operator_machine-config-daemon-4mzxf_machine-config-daemon_previous.log we see this function panicking.

This seems to have actually been the second place where we hit this function in that logic loop? i.e. loadNodeAnnotations -> getNodeAnnotation -> getNodeAnnotationExt should have panicked if either node or node.Annotations is empty, but for some reason it didn't. Weird. Also the fact that we have not seen this before makes it extra odd.

In either case, for safety reasons, we shouldn't be attempting to dereference a potentially nil pointer anyways, so let's just make this quick fix.

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 openshift-eng/jira-lifecycle-plugin repository.

[yuqi-zhang]

/cherry-pick release-4.18

[openshift-cherrypick-robot]

@yuqi-zhang: new pull request created: #4743

In response to this:

/cherry-pick release-4.18

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-sigs/prow repository.

[openshift-bot]

[ART PR BUILD NOTIFIER]

Distgit: ose-machine-config-operator
This PR has been included in build ose-machine-config-operator-container-v4.19.0-202412110336.p0.g2bcdb61.assembly.stream.el9.
All builds following this will include this PR.