OCPBUGS-51009: OSUpdateStarted event should only be emitted on actual OS updates #4864

djoshy · 2025-02-18T19:46:24Z

- What I did

Stopped emitting OSUpdateStarted events for a specific scenario: there are some extensions currently in use, but no extension installs/uninstalls are taking place. In such cases, rpm-ostree update is not run and no OS updates are happening. I suspect this a special case that we accounted for that is no longer in use.
Made the OSUpdateStarted event's message a bit more verbose for easier debugging.

- How to verify it

Existing units/e2es should pass, this should not break extension functionality.
Not sure this needs pre merge QE testing.

openshift-ci-robot · 2025-02-18T19:46:33Z

@djoshy: This pull request references Jira Issue OCPBUGS-51009, 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:

- What I did

Stopped emitting OSUpdateStarted events for a specific scenario: there are some extensions currently in use, but no extension installs/uninstalls are taking place. In such cases, rpm-ostree update is not run and no OS updates are happening. I suspect this a special case that we accounted for that is no longer in use.

Made the OSUpdateStarted event's message a bit more verbose for easier debugging.

- How to verify it

Existing units/e2es should pass, this should not break extension functionality.

Not sure this needs pre merge QE testing.

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

/lgtm

I think the changes make sense. These events really shouldn't be canonical for OS upgrades, but I guess for now they work for catching CI flakes like this, so it somewhat helps

yuqi-zhang · 2025-02-18T23:59:59Z

pkg/daemon/update.go

 	// We have at least one customer that removes the pull secret from the cluster to "shrinkwrap" it for distribution and we want
 	// to make sure we don't break that use case, but realtime kernel update and extensions update always ran
 	// if they were in use, so we also need to preserve that behavior.
 	// https://issues.redhat.com/browse/OCPBUGS-4049
 	if mcDiff.osUpdate || mcDiff.extensions || mcDiff.kernelType || mcDiff.kargs ||
 		canonicalizeKernelType(newConfig.Spec.KernelType) == ctrlcommon.KernelTypeRealtime ||
-		canonicalizeKernelType(newConfig.Spec.KernelType) == ctrlcommon.KernelType64kPages ||
-		len(newConfig.Spec.Extensions) > 0 {
+		canonicalizeKernelType(newConfig.Spec.KernelType) == ctrlcommon.KernelType64kPages {


Hmm, the best I can interpret the intent was that on upgrades, the extensions packages should also be automatically updating with the OS version, that said, I think this logic would still be incorrect, since it should only emit an event if mcDiff.osUpdate && len(newConfig.Spec.Extensions) which then is covered by the mcDiff.osUpdate above so maybe that doesn't make sense

yuqi-zhang · 2025-02-19T00:01:03Z

pkg/daemon/update.go

+				// osChangesString() can return empty in cases where the above diffs are false,
+				// but the node uses a non standard kernel, so let's make it a bit more
+				// informative in such cases
+				reason = fmt.Sprintf("Updating to a target config with %s kernel", canonicalizeKernelType(newConfig.Spec.KernelType))


I was thinking of having this also refer to the rendered config that the update is happening for, but I guess that's relatively easy to match, so no need here.

openshift-ci · 2025-02-19T00:02:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: djoshy, 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~~ [djoshy,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 · 2025-02-19T11:18:30Z

/label acknowledge-critical-fixes-only

/hold

Holding for QE

sergiordlr · 2025-02-20T12:12:21Z

Verified using IPI on AWS

Install all extensions
Check the events

$ oc get events --sort-by metadata.creationTimestamp |grep OSUpdateStarted
11m         Normal    OSUpdateStarted                           node/ip-10-0-20-249.us-east-2.compute.internal                       Installing extensions
5m5s        Normal    OSUpdateStarted                           node/ip-10-0-35-192.us-east-2.compute.internal                       Installing extensions

Configure the MachineConfiguration resource to execute no action given a test file

oc edit machineconfiguration
...
  spec:
    logLevel: Normal
    managementState: Managed
    nodeDisruptionPolicy:
      files:
      - actions:
        - type: None
        path: /etc/test-file
    operatorLogLevel: Normal

Create a MC to deploy the test file


kind: MachineConfig
metadata:
  labels:
    machineconfiguration.openshift.io/role: worker
  name: test-machine-config
spec:
  config:
    ignition:
      version: 3.1.0
    storage:
      files:
      - contents:
          source: data:text/plain;charset=utf-8;base64,dGVzdA==
        filesystem: root
        mode: 420
        path: /etc/test-file

Check that no extra events were triggered

$ oc get events --sort-by metadata.creationTimestamp |grep OSUpdateStarted
16m         Normal    OSUpdateStarted                           node/ip-10-0-20-249.us-east-2.compute.internal                       Installing extensions
10m         Normal    OSUpdateStarted                           node/ip-10-0-35-192.us-east-2.compute.internal                       Installing extensions

Remove the extensions
Check that new events were triggered

$ oc get events --sort-by metadata.creationTimestamp |grep OSUpdateStarted
27m         Normal    OSUpdateStarted                           node/ip-10-0-20-249.us-east-2.compute.internal                       Installing extensions
21m         Normal    OSUpdateStarted                           node/ip-10-0-35-192.us-east-2.compute.internal                       Installing extensions
7m43s       Normal    OSUpdateStarted                           node/ip-10-0-20-249.us-east-2.compute.internal                       Installing extensions
2m50s       Normal    OSUpdateStarted                           node/ip-10-0-35-192.us-east-2.compute.internal                       Installing extensions

Deploy realtime kernel

apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfig
metadata:
  labels:
    machineconfiguration.openshift.io/role: worker
  name: worker-rt-kernel
spec:
  kernelType: realtime

Check events

$ oc get events --sort-by metadata.creationTimestamp |grep OSUpdateStarted
47m         Normal    OSUpdateStarted                           node/ip-10-0-20-249.us-east-2.compute.internal                       Installing extensions
40m         Normal    OSUpdateStarted                           node/ip-10-0-35-192.us-east-2.compute.internal                       Installing extensions
27m         Normal    OSUpdateStarted                           node/ip-10-0-20-249.us-east-2.compute.internal                       Installing extensions
22m         Normal    OSUpdateStarted                           node/ip-10-0-35-192.us-east-2.compute.internal                       Installing extensions
15m         Normal    OSUpdateStarted                           node/ip-10-0-20-249.us-east-2.compute.internal                       Changing kernel type
8m19s       Normal    OSUpdateStarted                           node/ip-10-0-35-192.us-east-2.compute.internal                       Changing kernel type

/label qe-approved

openshift-ci-robot · 2025-02-20T12:12:27Z

@djoshy: This pull request references Jira Issue OCPBUGS-51009, 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:

- What I did

Stopped emitting OSUpdateStarted events for a specific scenario: there are some extensions currently in use, but no extension installs/uninstalls are taking place. In such cases, rpm-ostree update is not run and no OS updates are happening. I suspect this a special case that we accounted for that is no longer in use.

Made the OSUpdateStarted event's message a bit more verbose for easier debugging.

- How to verify it

Existing units/e2es should pass, this should not break extension functionality.

Not sure this needs pre merge QE testing.

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.

djoshy · 2025-02-20T12:40:58Z

/unhold

djoshy · 2025-02-20T14:56:25Z

/retest-required

openshift-ci-robot · 2025-02-20T17:14:05Z

/retest-required

Remaining retests: 0 against base HEAD d34ac99 and 2 for PR HEAD c75a003 in total

openshift-ci-robot · 2025-02-20T22:04:00Z

/retest-required

Remaining retests: 0 against base HEAD d34ac99 and 2 for PR HEAD c75a003 in total

openshift-ci · 2025-02-21T01:06:34Z

@djoshy: 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-gcp-op-techpreview	`c75a003`	link	false	`/test e2e-gcp-op-techpreview`
ci/prow/e2e-aws-ovn-upgrade-out-of-change	`c75a003`	link	false	`/test e2e-aws-ovn-upgrade-out-of-change`
ci/prow/e2e-azure-ovn-upgrade-out-of-change	`c75a003`	link	false	`/test e2e-azure-ovn-upgrade-out-of-change`
ci/prow/e2e-gcp-op-ocl	`c75a003`	link	false	`/test e2e-gcp-op-ocl`
ci/prow/e2e-gcp-op	`c75a003`	link	true	`/test e2e-gcp-op`

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.

daemon: clarify OSUpdate events

c75a003

openshift-ci bot requested review from sergiordlr, LorbusChris and umohnani8 February 18, 2025 19:46

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 18, 2025

yuqi-zhang approved these changes Feb 19, 2025

View reviewed changes

openshift-ci bot assigned yuqi-zhang Feb 19, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 19, 2025

pperiyasamy mentioned this pull request Feb 19, 2025

Fix IPsec tests for monitor failures openshift/origin#29437

Open

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. labels Feb 19, 2025

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Feb 20, 2025

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-51009: OSUpdateStarted event should only be emitted on actual OS updates #4864

OCPBUGS-51009: OSUpdateStarted event should only be emitted on actual OS updates #4864

djoshy commented Feb 18, 2025

openshift-ci-robot commented Feb 18, 2025

yuqi-zhang left a comment

yuqi-zhang Feb 18, 2025

yuqi-zhang Feb 19, 2025

openshift-ci bot commented Feb 19, 2025

djoshy commented Feb 19, 2025

sergiordlr commented Feb 20, 2025

openshift-ci-robot commented Feb 20, 2025

djoshy commented Feb 20, 2025

djoshy commented Feb 20, 2025

openshift-ci-robot commented Feb 20, 2025

openshift-ci-robot commented Feb 20, 2025

openshift-ci bot commented Feb 21, 2025

OCPBUGS-51009: OSUpdateStarted event should only be emitted on actual OS updates #4864

Are you sure you want to change the base?

OCPBUGS-51009: OSUpdateStarted event should only be emitted on actual OS updates #4864

Conversation

djoshy commented Feb 18, 2025

openshift-ci-robot commented Feb 18, 2025

yuqi-zhang left a comment

Choose a reason for hiding this comment

yuqi-zhang Feb 18, 2025

Choose a reason for hiding this comment

yuqi-zhang Feb 19, 2025

Choose a reason for hiding this comment

openshift-ci bot commented Feb 19, 2025

djoshy commented Feb 19, 2025

sergiordlr commented Feb 20, 2025

openshift-ci-robot commented Feb 20, 2025

djoshy commented Feb 20, 2025

djoshy commented Feb 20, 2025

openshift-ci-robot commented Feb 20, 2025

openshift-ci-robot commented Feb 20, 2025

openshift-ci bot commented Feb 21, 2025