OCPBUGS-52656: Update the MCN PIS status of only the primary pool #4948
Conversation
openshift-ci-robot
added
jira/severity-low
|
@RishabhSaini: This pull request references Jira Issue OCPBUGS-52656, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
RishabhSaini
changed the title
openshift-ci
bot
added
the
approved
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@RishabhSaini: This pull request references Jira Issue OCPBUGS-52656, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
|
@RishabhSaini: This pull request references Jira Issue OCPBUGS-52656, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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 |
|
/test e2e-hypershift |
|
@RishabhSaini: 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. |
| primaryPool, err := helpers.GetPrimaryPoolForNode(p.mcpLister, node) | ||
| if err != nil { | ||
| klog.Errorf("error getting primary pool for node: %v", node.Name) | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
Question: Since this is is being added to replace the previous get pool functionality, are you able to remove the pool selection code up here?
Suggestion: Since GetPrimaryPoolForNode can return nil, nil (no pool, but also no error), it might be good to add a check to make sure you do get a pool (like the check here).
There was a problem hiding this comment.
Question: Since this is is being added to replace the previous get pool functionality, are you able to remove the pool selection code up here?
We still want to sync the individual pools of the node. Just not update the status as it duplicates
Suggestion: Since GetPrimaryPoolForNode can return nil, nil (no pool, but also no error), it might be good to add a check to make sure you do get a pool (like the check here).
agreed
There was a problem hiding this comment.
We still want to sync the individual pools of the node. Just not update the status as it duplicates
Understood, thanks!
|
/lgtm thanks 🙏 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hexfusion, RishabhSaini 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 |
|
@RishabhSaini Are you able to fix the typo on this line in this PR also? /hold |
openshift-ci
bot
added
the
do-not-merge/hold
Since this typo is a miscellaneous change rather than part of this bug fix, I believe it would be more appropriate to address it in PR #4934. I'll make the necessary changes there. |
|
Pre-merge verification steps :
Infra pool templateoc create -f - << EOF
apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfigPool
metadata:
name: infra
spec:
machineConfigSelector:
matchExpressions:
- {key: machineconfiguration.openshift.io/role, operator: In, values: [worker,infra]}
nodeSelector:
matchLabels:
node-role.kubernetes.io/infra: ""
EOF
oc label node/ip-10-0-56-194.us-east-2.compute.internal node-role.kubernetes.io/infra= node/ip-10-0-56-194.us-east-2.compute.internal labeled- Apply the PIS on worker pool oc create -f - << EOF
apiVersion: machineconfiguration.openshift.io/v1
kind: PinnedImageSet
metadata:
labels:
machineconfiguration.openshift.io/role: worker
name: worker-pinned-images
spec:
pinnedImages:
- name: "quay.io/openshifttest/busybox@sha256:0415f56ccc05526f2af5a7ae8654baec97d4a614f24736e8eef41a4591f08019"
- name: quay.io/openshifttest/alpine@sha256:be92b18a369e989a6e86ac840b7f23ce0052467de551b064796d67280dfa06d5
EOF
pinnedimageset.machineconfiguration.openshift.io/worker-pinned-images created
- Verify No error is seen in MCD logs and infra MCN does not report any error
for node in $(oc get nodes -l node-role.kubernetes.io/infra -o name)
do
n=${node/node\//}
echo $n
oc get machineconfignode $n -ojsonpath='{.status.conditions[?(@.type=="PinnedImageSetsDegraded")]}' | jq
oc get machineconfignode $n -ojsonpath='{.status.conditions[?(@.type=="PinnedImageSetsProgressing")]}' | jq
done
ip-10-0-56-194.us-east-2.compute.internal
{
"lastTransitionTime": "2025-04-03T11:09:28Z",
"message": "All is good",
"reason": "AsExpected",
"status": "False",
"type": "PinnedImageSetsDegraded"
}
{
"lastTransitionTime": "2025-04-03T11:09:28Z",
"message": "All pinned image sets complete",
"reason": "AsExpected",
"status": "False",
"type": "PinnedImageSetsProgressing"
}
oc logs machine-config-daemon-r5946 | tail -n 10 Defaulted container "machine-config-daemon" out of: machine-config-daemon, kube-rbac-proxy I0403 10:03:23.063704 2636 certificate_writer.go:294] Certificate was synced from controllerconfig resourceVersion 109207 I0403 10:03:23.106638 2636 certificate_writer.go:294] Certificate was synced from controllerconfig resourceVersion 109208 I0403 10:03:23.898563 2636 certificate_writer.go:294] Certificate was synced from controllerconfig resourceVersion 109225 I0403 10:14:08.096306 2636 certificate_writer.go:294] Certificate was synced from controllerconfig resourceVersion 109225 I0403 10:34:16.735136 2636 pinned_image_set.go:308] Reconciling pinned image set: tc-80334-worker-pinned-images: generation: 1 I0403 10:34:22.360835 2636 pinned_image_set.go:432] Completed scheduling 50% of images I0403 10:34:22.360859 2636 pinned_image_set.go:432] Completed scheduling 100% of images I0403 10:34:23.767860 2636 file_writers.go:234] Writing file "/etc/crio/crio.conf.d/50-pinned-images" I0403 10:34:23.770925 2636 update.go:2731] Running: systemctl reload crio I0403 10:36:02.458515 2636 certificate_writer.go:294] Certificate was synced from controllerconfig resourceVersion 109225- Verify images are pin to worker pool oc debug -q node/$(oc get nodes -l node-role.kubernetes.io/worker -ojsonpath="{.items[0].metadata.name}") -- chroot host crictl images --pinned
IMAGE TAG IMAGE ID SIZE PINNED
quay.io/openshifttest/alpine 45683da4f97c2 5.87MB true
quay.io/openshifttest/busybox b97242f89c8a2 1.45MB true
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
openshift-merge-bot
bot
merged commit 7331a53
into
openshift:main
|
@RishabhSaini: Jira Issue OCPBUGS-52656: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-52656 has been moved to the MODIFIED state. 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. |
|
[ART PR BUILD NOTIFIER] Distgit: ose-machine-config-operator |
- What I did
Do not add the same PIS to the MCNStatusPIS for separate different MCPs
- How to verify it
- Description for the changelog