Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OSD-25656] Enhance the affinity rules to avoid pod mis-scheduling #62

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

xiaoyu74
Copy link

@xiaoyu74 xiaoyu74 commented Nov 25, 2024

What type of PR is this?

(bug)

What this PR does / Why we need it?

Enhance the affinity rules to avoid metrics-forwarder pod mis-scheduling.

Which Jira/Github issue(s) does this PR fix?

https://issues.redhat.com/browse/OSD-25656

For reviewer

Based on the ticket description, the current logic to determine if the metrics-forwarder pod is mis-scheduled is:

1. Check the node label:

  • For each node running a metrics-forwarder pod, check the value of the hypershift.openshift.io/cluster label

2. Compare namespace and serving node label:

  • Compare the namespace of the metrics-forwarder pod to the value of the hypershift.openshift.io/cluster label on the serving node.
  • If the namespace of the pod does not match the value of the hypershift.openshift.io/cluster label, the pod is considered mis-scheduled.

So, to resolve the mis-scheduling issue, I just enforce strict affinity rules in the metrics-forwarder deployment config to ensure the pod is scheduled only on nodes with the hypershift.openshift.io/cluster label matching the pod's namespace.

Also, based on the test outcome, I can see there is case that many serving nodes have the hypershift.openshift.io/cluster label missing, we need to provide a affinity configuration that accommodates these scenarios as well.

Basically, in the PR, I just:

  • Added required "requiredDuringSchedulingIgnoredDuringExecution" with the hypershift.openshift.io/cluster label.
  • Keep the previous affinity rules for handling the label missing or other edge cases.

P.S More context and test outcome is in the Jira ticket OSD-25656

@openshift-ci openshift-ci bot requested review from feichashao and Tafhim November 25, 2024 03:41
package/resources.yaml.gotmpl Outdated Show resolved Hide resolved
@xiaoyu74 xiaoyu74 requested a review from bmeng December 3, 2024 05:45
package/resources.yaml.gotmpl Outdated Show resolved Hide resolved
@xiaoyu74
Copy link
Author

xiaoyu74 commented Dec 8, 2024

  • Tested to create the deployment on the MC hs-mc-13vnfh7o0 in the namespace
oc get deployment -n ocm-staging-2eigjr6f5uj8u7l6ie5ql5656ohk1nbr-rosa-hcp-new | grep -i metrics
metrics-forwarder                         2/2     2            2           46d
metrics-forwarder-2                       1/2     2            1           52s
  • The newly added affinity rule can be present properly.
oc get deployment metrics-forwarder-2 -n ocm-staging-2eigjr6f5uj8u7l6ie5ql5656ohk1nbr-rosa-hcp-new -o json | jq .spec.template.spec.affinity.nodeAffinity
{
  "preferredDuringSchedulingIgnoredDuringExecution": [
    {
      "preference": {
        "matchExpressions": [
          {
            "key": "hypershift.openshift.io/control-plane",
            "operator": "In",
            "values": [
              "true"
            ]
          }
        ]
      },
      "weight": 50
    },
    {
      "preference": {
        "matchExpressions": [
          {
            "key": "hypershift.openshift.io/request-serving-component",
            "operator": "In",
            "values": [
              "true"
            ]
          }
        ]
      },
      "weight": 100
    },
    {
      "preference": {
        "matchExpressions": [
          {
            "key": "node-role.kubernetes.io/worker",
            "operator": "Exists"
          }
        ]
      },
      "weight": 30
    }
  ],
  "requiredDuringSchedulingIgnoredDuringExecution": {
    "nodeSelectorTerms": [
      {
        "matchExpressions": [
          {
            "key": "hypershift.openshift.io/cluster",
            "operator": "In",
            "values": [
              "ocm-staging-2eigjr6f5uj8u7l6ie5ql5656ohk1nbr-rosa-hcp-new"
            ]
          }
        ]
      }
    ]
  }
}

@xiaoyu74 xiaoyu74 requested a review from bmeng December 8, 2024 07:53
Copy link
Contributor

openshift-ci bot commented Dec 16, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xiaoyu74
Once this PR has been reviewed and has the lgtm label, please assign tafhim for approval. For more information see the Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@xiaoyu74
Copy link
Author

I created a script to loop through all the HCP namespaces on the MC (hs-mc-13vnfh7o0) and identified that at least one of the serving nodes hosting the control-plane workloads in each HCP namespace has both the labels 'hypershift.openshift.io/cluster=ocm-staging-{{.package.metadata.namespace}}' and 'hypershift.openshift.io/request-serving-component=true' present in pairs.

Based on this, the updated logic sets the nodeAffinity for scheduling metrics-forward pods on nodes that have both required labels. Additionally, the preferredDuringSchedulingIgnoredDuringExecution section has been updated with different weights to improve robustness in cases where the required labels are missing. cc: @bmeng

@xiaoyu74
Copy link
Author

/label tide/merge-method-squash

@openshift-ci openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 21, 2025
Copy link
Contributor

openshift-ci bot commented Jan 21, 2025

@xiaoyu74: all tests passed!

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants