MCO-1870: Split MachineConfigNodeUpdateFilesAndOS condition into MachineConfigNodeUpdateOS and MachineConfigNodeUpdateFiles
#5411
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci
bot
added
the
approved
isabella-janssen
force-pushed
the
mco-1870
branch
3 times, most recently
from
3e034f5 to
eb2cfa0
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
isabella-janssen
force-pushed
the
mco-1870
branch
from
eb2cfa0 to
5697495
Compare
| // Note that the following two conditions replace the previous, singular | ||
| // `MachineConfigNodeUpdateFilesAndOS` condition |
There was a problem hiding this comment.
Note to reviewer: The idea here is that the MachineConfigNodeUpdateFilesAndOS condition in 4.19 and 4.20 will no longer be populated and instead we will be populating the MachineConfigNodeUpdateFiles and MachineConfigNodeUpdateOS conditions. I don't think this will cause any issues but would like a second sanity check on that.
There was a problem hiding this comment.
We are using the MachineConfigNodeUpdateFilesAndOS just to report, isn't it? I mean, we are not reacting to it for other purposes like tracking the MCP status aren't we? If so, I don't think this should be a problem.
From a user point of view, only customers using ImageMode should see the difference I guess, cause the MachineConfigNodeUpdateFilesAndOS is still reported for non-image mode. If so, I think it's still a small user base that won't be doing something with the condition. If someone asks me for my opinion, I'd say I prefer to have this change now early in the life of the feature than later, when maybe I have created some automation around it.
There was a problem hiding this comment.
We are using the MachineConfigNodeUpdateFilesAndOS just to report, isn't it? I mean, we are not reacting to it for other purposes like tracking the MCP status aren't we?
Yes, the condition is just for reporting, no MCP statuses are reacting to this status, and it's getting cleaned up when the FeatureGate is enabled, so it should be ok.
From a user point of view, only customers using ImageMode should see the difference I guess, cause the MachineConfigNodeUpdateFilesAndOS is still reported for non-image mode.
For now, but once the ImageModeStatusReporting FG is GAed they would instead see the split up condition always (image mode or not). We could change it so that MachineConfigNodeUpdateFilesAndOS persists for non-image mode ipdates, but it seems odd to me to keep it if there are the split, more specific conditions instead. The plan was to remove the MachineConfigNodeUpdateFilesAndOS condition at some point in favor of the two conditions (MCO-1775). Does there need to be more of a "smooth transition" than just splitting them, do you think?
There was a problem hiding this comment.
Just documenting here that I spoke with @yuqi-zhang and @pablintino about this today. The general consensus, as I understand it (please correct me if I'm misrepresenting Pablo & Jerry), is that removing the singular, combined MachineConfigNodeUpdateFilesAndOS condition and replacing it with the separated conditions is fine considering how new the MCN resource is, the low likelihood that customers are keying scripts off the combined condition, and the idea that duplicating statuses will likely be more confusing than simply splitting the condition.
There was a problem hiding this comment.
As a note, I realized that it's being referenced in a printer column: https://github.com/openshift/api/blob/master/machineconfiguration/v1/types_machineconfignode.go#L26
(and of course, the const below).
We should probably update that (unless that's already tracked somewhere), but I think that should be safe in the API since it's not an explicit enum or validated state.
There was a problem hiding this comment.
Oh yes, good catch @yuqi-zhang. Given that this is not going to land today anyways, I'll make sure to have an API PR for removing that print column ready to merge alongside this PR after the 4.22 branch opens. I've also added that as an acceptance criteria for this story. Thanks for the extra set of eyes on this!
|
/payload-job periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-aws-mco-disruptive-techpreview-1of2 periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-aws-mco-disruptive-techpreview-2of2 |
|
@isabella-janssen: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a0870430-c5b9-11f0-969b-64edc28a3b3d-0 |
|
changes LGTM |
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
isabella-janssen
force-pushed
the
mco-1870
branch
from
85ff498 to
2f06a94
Compare
…teFilesAndOS' condition to 'MachineConfigNodeUpdateOS' and 'MachineConfigNodeUpdateFiles' and introduce 'ImagePulledFromRegistry' when 'ImageModeStatusReporting' is enabled
…lied OS and file conditions
isabella-janssen
force-pushed
the
mco-1870
branch
from
2f06a94 to
e9e7e74
Compare
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
|
/test unit |
|
/payload-job periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-aws-mco-disruptive-techpreview-1of2 periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-aws-mco-disruptive-techpreview-2of2 |
|
@isabella-janssen: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5ed988b0-c685-11f0-9291-a6766f42b6b3-0 |
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
|
/retest-required |
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
|
/payload-job periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-gcp-mco-disruptive-techpreview-1of2 periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-gcp-mco-disruptive-techpreview-2of2 |
|
@isabella-janssen: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/fa755070-c6e8-11f0-89b5-66047601ca74-0 |
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
|
@isabella-janssen: This pull request references MCO-1870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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. |
|
/hold This should land after openshift/origin#30505, which is currently blocked by CI failures (see this Slack thread). |
openshift-ci
bot
added
the
do-not-merge/hold
| needsImagePulledFromRegistry := true | ||
| var newConditions []metav1.Condition | ||
| for _, condition := range mcn.Status.Conditions { | ||
| //nolint:gocritic // (ijanssen) the linter thinks this block would be clearer as a switch statement, but I disagree |
| // Note that the following two conditions replace the previous, singular | ||
| // `MachineConfigNodeUpdateFilesAndOS` condition |
There was a problem hiding this comment.
We are using the MachineConfigNodeUpdateFilesAndOS just to report, isn't it? I mean, we are not reacting to it for other purposes like tracking the MCP status aren't we? If so, I don't think this should be a problem.
From a user point of view, only customers using ImageMode should see the difference I guess, cause the MachineConfigNodeUpdateFilesAndOS is still reported for non-image mode. If so, I think it's still a small user base that won't be doing something with the condition. If someone asks me for my opinion, I'd say I prefer to have this change now early in the life of the feature than later, when maybe I have created some automation around it.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: isabella-janssen, pablintino 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 |
5 participants
Note: this PR should merge after openshift/origin#30505.
- What I did
MachineConfigNodeUpdateFilesAndOSMCN condition intoMachineConfigNodeUpdateOSandMachineConfigNodeUpdateFilesfor clusters where theImageModeStatusReportingfeature gate is enabled.MachineConfigNodeUpdateFilesAndOSMCN condition whenImageModeStatusReportingfeature gate is enabled and introduce the newMachineConfigNodeUpdateOS,MachineConfigNodeUpdateFilesandImagePulledFromRegistryconditions.- How to verify it
Regression test verification: The
ImageModeStatusReportingregression tests should pass.Manual verification:
MachineConfigNodeUpdateOSorMachineConfigNodeUpdateFilesconditions update, according to the type of update triggered.See below example of MCNs without the
ImageModeStatusReportingFeatureGate enabled:See below example of MCNs when the
ImageModeStatusReportingFeatureGate is enabled:See below example of MCNs when cluster is launched in TechPreview:
See below example of MCNs when TechPreview cluster has had an image update applied:
Example MC I used
apiVersion: machineconfiguration.openshift.io/v1 kind: MachineConfig metadata: labels: machineconfiguration.openshift.io/role: infra name: 90-infra-testfile spec: config: ignition: version: 3.2.0 storage: files: - contents: source: data:,hello%20world%0A mode: 420 path: /home/core/testSee below example of MCNs when TechPreview cluster has had a non-image based update applied:
I enabled Image Mode for this
$ oc create -f - << EOF apiVersion: machineconfiguration.openshift.io/v1 kind: MachineOSConfig metadata: name: infra spec: machineConfigPool: name: infra imageBuilder: imageBuilderType: Job renderedImagePushSecret: name: $(oc get -n openshift-machine-config-operator sa builder -ojsonpath='{.secrets[0].name}') renderedImagePushSpec: "image-registry.openshift-image-registry.svc:5000/openshift-machine-config-operator/ocb-image:latest" EOF- Description for the changelog
MCO-1870: Split
MachineConfigNodeUpdateFilesAndOScondition intoMachineConfigNodeUpdateOSandMachineConfigNodeUpdateFiles