MCO-1713:[API 4/6] Update MCO to reflect API changes made for Status Reporting #5177
Conversation
|
@naseerahkani: This pull request references MCO-1713 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.20.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-robot
added
the
jira/valid-reference
|
PR needs rebase. 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-merge-robot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: naseerahkani The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pkg/operator/sync.go
Outdated
| //Add: check if image mode status reporting fg is enables and if yes, use 821-826 as a guideline for configImage initialization | ||
| if fg.Enabled(features.FeatureGateImageModeStatusReporting) { | ||
| newMCS.Spec.ConfigImage = mcfgv1.MachineConfigNodeSpecConfigImage{ | ||
| DesiredImage: mcfgv1.ImageDigestFormat(node.Annotations[daemonconsts.DesiredImageAnnotationKey]), | ||
| } | ||
| } |
There was a problem hiding this comment.
I think how we handle this depends on the decision made this thread on the API review (openshift/api#2383) on whether the configImage value is required or not and what it means for whether OCL is enabled or not.
If configImage should only be present when OCL is enabled (when a node generally has a desiredImage or current Image annotation), then I'd lean towards
- If either annotation is not nil --> set the desired image ref since image mode is enabled
- If both annotations are nil --> OCL not enabled, so don't set this object
pkg/operator/sync.go
Outdated
| if fg.Enabled(features.FeatureGateImageModeStatusReporting) { | ||
| if mcn.Spec.ConfigImage.DesiredImage == upgrademonitor.NotYetSet { | ||
| err = upgrademonitor.GenerateAndApplyMachineConfigNodeSpec(optr.fgAccessor, pool, node, optr.client) | ||
| if err != nil { | ||
| klog.Errorf("Error making MCN spec for Update Compatible: %v", err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this duplicative of lines 829-835? If I'm understanding this added conditional correctly, GenerateAndApplyMachineConfigNodeSpec would be called twice if both mcn.Spec.ConfigVersion.Desired and mcn.Spec.ConfigImage.DesiredImage are not yet set. Two questions on this:
- Will the desired image value ever be
not-yet-set? If so, I'm not sure that's a fair representation of the desiredImage annotation not existing for a node. - Should this be combined with the previous similar check to be something like
if mcn.Spec.ConfigVersion.Desired == upgrademonitor.NotYetSet || mcn.Spec.ConfigImage.DesiredImage == <empty-status-value>(where depends on the answer to no. 1) to decrease the extraGenerateAndApplyMachineConfigNodeSpeccall?
| statusConfigImageApplyImage := machineconfigurationv1.MachineConfigNodeStatusConfigImage().WithDesiredImage(newMCNode.Status.ConfigImage.DesiredImage) | ||
| if node.Annotations[daemonconsts.CurrentImageAnnotationKey] != "" { | ||
| statusConfigImageApplyImage = statusConfigImageApplyImage.WithCurrentImage(newMCNode.Status.ConfigImage.CurrentImage) | ||
| } |
There was a problem hiding this comment.
Question: Why does current image have a nil check, but desired does not?
| if newMCNode.Spec.ConfigImage.DesiredImage == "" { | ||
| newMCNode.Spec.ConfigImage.DesiredImage = "" | ||
| } |
There was a problem hiding this comment.
This does not seem like it would make any change.
| } | ||
| // check if it should be empty | ||
| if newMCNode.Spec.ConfigImage.DesiredImage == "" { | ||
| newMCNode.Spec.ConfigVersion.Desired = NotYetSet |
There was a problem hiding this comment.
It seems like this may be a copy-paste issue, as I'd expect this to be newMCNode.Spec.ConfigImage.Desired, not newMCNode.Spec.ConfigVersion.Desired.
However, do we want to make the config image values not-yet-set if they are nil, or just leave them empty, I figured we'd leave it empty?
There was a problem hiding this comment.
I can fix the copy-paste issue. As for the config image value, I put it down as NotYetSet temporarily and depending on how the API changes are finalized, I planned on going back and updating it to empty. Do you think this would be a good approach?
There was a problem hiding this comment.
Sure, let's revisit once the API Is finalized.
pkg/daemon/update.go
Outdated
| _, newOCLImage := extractOCLImageFromMachineConfig(newConfig) | ||
| err = upgrademonitor.GenerateAndApplyMachineConfigNodes( | ||
| &upgrademonitor.Condition{State: mcfgv1.MachineConfigNodeUpdateExecuted, Reason: string(mcfgv1.MachineConfigNodeImagePulledFromRegistry), Message: fmt.Sprintf("Image %s pulled from registry.", newOCLImage)}, | ||
| &upgrademonitor.Condition{State: mcfgv1.MachineConfigNodeImagePulledFromRegistry, Reason: fmt.Sprintf("%s%s", string(mcfgv1.MachineConfigNodeUpdateExecuted), string(mcfgv1.MachineConfigNodeImagePulledFromRegistry)), Message: fmt.Sprintf("ask message")}, |
There was a problem hiding this comment.
Hey, it looks like this message "ask message" might be a placeholder.
There was a problem hiding this comment.
Yes, do you have any suggestions on the message that should be produced?
There was a problem hiding this comment.
anything that signifies that the image was pulled!
pkg/daemon/update.go
Outdated
| klog.Errorf("Error making MCN spec for Update Compatible: %v", err) | ||
| } | ||
|
|
||
| _, newOCLImage := extractOCLImageFromMachineConfig(newConfig) |
There was a problem hiding this comment.
This function is responsible for extracting the ocl image from the mc if it exists. I think that function should work, buttt I'm not sure that this reporting should go here for a couple of reasons.
- The update workflow is agnostic to OCL, meaning there will be times when we go through this MCD update when we aren't even opted into OCL. Thus, we will be returning empty strings that can cause confusing status messages (so we want to make sure we handle this case).
- Also, i think the placement of this is premature because we could be status reporting before the correct image is in place.
I think update is a good place to put this work but we just want to ensure the timing is correct!
There was a problem hiding this comment.
I can add code to handle the empty string case but where do you recommend adding this?
There was a problem hiding this comment.
maybe after applyOSChanges has succeeded- meaning the image was pulled and the OS was rebased successfully.
|
@naseerahkani: The following tests failed, say
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. |
- What I did
Updating MCO code to reflect changing from API to support Image Mode Status Reporting