Set degraded if nmstate is requested in networkAddonsConfig #1292

rhrazdil · 2022-03-22T12:29:28Z

What this PR does / why we need it:
This PR aims to block upgrade if still using CNAO to deploy nmstate by Setting NetworkAddonsConfig Degraded Status to true.
HCO will reconcile it and see that the NAC status is degraded, consequently it will set it's status upgradeable to false.

See HCO: https://github.com/kubevirt/hyperconverged-cluster-operator/blob/main/controllers/hyperconverged/hyperconverged_controller.go#L740

That will block upgrade until user installs KNO (Kubernetes Nmstate Operator).

Special notes for your reviewer:

HCO change:

Remove nmstate from deployed components in networkaddonsconfig hyperconverged-cluster-operator#1847

Release note:

Block CNV upgrade if still using CNAO to deploy nmstate

rhrazdil · 2022-03-23T08:35:14Z

/hold

rhrazdil · 2022-03-29T11:15:30Z

/hold cancel

rhrazdil · 2022-03-30T10:37:06Z

Hello @tiraboschi @qinqon @RamLavi
Could you please review this PR?

qinqon

I think we don't need to check openshift.

qinqon · 2022-03-31T05:43:04Z

pkg/controller/statusmanager/status_manager.go

+			reason := "InvalidConfiguration"
+			message := "NMState deployment is not provided in CNV >= 4.11, please install Kubernetes NMState Operator"
+			status.eventEmitter.EmitFailingForConfig(reason, message)
+			conditionsv1.SetStatusConditionNoHeartbeat(&config.Status.Conditions,


This should change heartbeat right ? it's not a noop

Right correct

qinqon · 2022-03-31T05:47:09Z

pkg/controller/statusmanager/status_manager.go

-			},
-		)
-		config.Status.ObservedVersion = operatorVersion
+		if status.isRunningOnOpenshift411OrLater() && config.Spec.NMState != nil {


Maybe we don't need to check openshift at all at the end at this u/s release for wherever is running we will fail on invalid networkaddonsconfig meaning that if they ask for NMState we fail, openshift kubernetes or whatever.

The thing is, if we don't check OCP 4.11, then we block not only upgrade from CNV 4.10 to 4.11, but also all z-stream upgrades on 4.10

However, we could notify u/s users as you suggest

We are not backporting this so this code will run only on OCP > 4.10

IIUC, we have to backport this to 4.10. If we didn't backport this a have this code in 4.11, then by the time users upgrade OCP to 4.11 and CNV to 4.11, it's already too late.

qinqon · 2022-03-31T05:48:56Z

pkg/controller/statusmanager/status_manager.go

+		} else {
+			// If successfully deployed all components and is not failing on anything, mark as Available
+			status.eventEmitter.EmitAvailableForConfig()
+			conditionsv1.SetStatusConditionNoHeartbeat(&config.Status.Conditions,


We should change heratbeat if previous condition was not success, although I don't know if we can check that or if it's worth it.

imo if we don't keep track about the heartbeat anymore then there's no need to update it in any scenario - neither good or bad.
We do have the TransitionTimestamp (when the status last changed value) which might be enough, no?

Ah, so we don't want to update it in #1292 (comment) either, correct?

Then feels like heartbeat is kind of obsolete and replaced by TransitionTimestamp ?

[fixing the name of TransitionTimestamp - exact name is LastTransitionTime)

So, it doesn't replace LastTransitionTime because LastTransitionTime is already on the original SetStatusCondition() function.

heartbeat is no longer published on CNAO. This is because it also causes a resourceVersion change (every 1 minute) and that triggers a repeating reconcile on HCO.
As a result, we decided to move to SetStatusConditionNoHeartbeat on main and release-0.65 branches.

HCO will reconcile and see that the NAC status is degraded and in turn set it's status upgradeable to false. That will block upgrade until user installs KNO Signed-off-by: Radim Hrazdil <rhrazdil@redhat.com>

sonarcloud · 2022-04-01T09:22:31Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

rhrazdil · 2022-04-01T12:51:30Z

After discussing with @qinqon I'm setting the Degraded condition always when nmstate is requested.
In a backport to release-0.65, I only do so if we find we're running on OCP 4.11
#1323

qinqon · 2022-04-04T06:51:05Z

/lgtm
/approve

kubevirt-bot · 2022-04-04T06:51:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qinqon

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~~ [qinqon]

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

…evirt#1292)" This reverts commit 575bdd5.

…evirt#1292)" This reverts commit 575bdd5. Signed-off-by: Radim Hrazdil <rhrazdil@redhat.com>

…)" (#1361) This reverts commit 575bdd5. Signed-off-by: Radim Hrazdil <rhrazdil@redhat.com>

kubevirt-bot requested review from alonSadan and qinqon March 22, 2022 12:29

kubevirt-bot added the size/M label Mar 22, 2022

rhrazdil force-pushed the block_411_upgrade_if_nmstate_deployed_without_kno branch 2 times, most recently from ce457b8 to cd824e9 Compare March 23, 2022 07:07

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2022

rhrazdil changed the title ~~WIP Set degraded if nmstate is deployed on OCP 4.11 or later~~ Set degraded if nmstate is deployed on OCP 4.11 or later Mar 23, 2022

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 23, 2022

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 29, 2022

rhrazdil force-pushed the block_411_upgrade_if_nmstate_deployed_without_kno branch 2 times, most recently from 289bc4c to 2752eed Compare March 30, 2022 06:50

qinqon requested changes Mar 31, 2022

View reviewed changes

kubevirt-bot assigned qinqon Mar 31, 2022

rhrazdil mentioned this pull request Apr 1, 2022

Remove nmstate from deployed components in networkaddonsconfig kubevirt/hyperconverged-cluster-operator#1847

Merged

11 tasks

Set degraded if nmstate is deployed on OCP 4.11 or later

e436a2a

HCO will reconcile and see that the NAC status is degraded and in turn set it's status upgradeable to false. That will block upgrade until user installs KNO Signed-off-by: Radim Hrazdil <rhrazdil@redhat.com>

rhrazdil force-pushed the block_411_upgrade_if_nmstate_deployed_without_kno branch from 2752eed to e436a2a Compare April 1, 2022 09:21

rhrazdil changed the title ~~Set degraded if nmstate is deployed on OCP 4.11 or later~~ Set degraded if nmstate is requested in networkAddonsConfig Apr 1, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 4, 2022

kubevirt-bot merged commit 575bdd5 into kubevirt:main Apr 4, 2022

rhrazdil pushed a commit to rhrazdil/cluster-network-addons-operator that referenced this pull request May 24, 2022

Revert "Set degraded if nmstate is deployed on OCP 4.11 or later (kub…

c32a3df

…evirt#1292)" This reverts commit 575bdd5.

kubevirt-bot mentioned this pull request May 24, 2022

Revert "Set degraded if nmstate is deployed on OCP 4.11 or later #1361

Merged

kubevirt-bot pushed a commit that referenced this pull request May 24, 2022

Revert "Set degraded if nmstate is deployed on OCP 4.11 or later (#1292…

b59befb

…)" (#1361) This reverts commit 575bdd5. Signed-off-by: Radim Hrazdil <rhrazdil@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set degraded if nmstate is requested in networkAddonsConfig #1292

Set degraded if nmstate is requested in networkAddonsConfig #1292

rhrazdil commented Mar 22, 2022 •

edited by qinqon

rhrazdil commented Mar 23, 2022

rhrazdil commented Mar 29, 2022

rhrazdil commented Mar 30, 2022

qinqon left a comment

qinqon Mar 31, 2022

rhrazdil Mar 31, 2022

qinqon Mar 31, 2022

rhrazdil Mar 31, 2022

rhrazdil Mar 31, 2022

qinqon Mar 31, 2022

rhrazdil Apr 1, 2022

qinqon Mar 31, 2022

RamLavi Mar 31, 2022 •

edited

rhrazdil Mar 31, 2022

qinqon Mar 31, 2022

RamLavi Apr 3, 2022 •

edited

sonarcloud bot commented Apr 1, 2022

rhrazdil commented Apr 1, 2022

qinqon commented Apr 4, 2022

kubevirt-bot commented Apr 4, 2022

Set degraded if nmstate is requested in networkAddonsConfig #1292

Set degraded if nmstate is requested in networkAddonsConfig #1292

Conversation

rhrazdil commented Mar 22, 2022 • edited by qinqon

rhrazdil commented Mar 23, 2022

rhrazdil commented Mar 29, 2022

rhrazdil commented Mar 30, 2022

qinqon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamLavi Mar 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamLavi Apr 3, 2022 • edited

Choose a reason for hiding this comment

sonarcloud bot commented Apr 1, 2022

rhrazdil commented Apr 1, 2022

qinqon commented Apr 4, 2022

kubevirt-bot commented Apr 4, 2022

rhrazdil commented Mar 22, 2022 •

edited by qinqon

RamLavi Mar 31, 2022 •

edited

RamLavi Apr 3, 2022 •

edited