CORENET-6276: Use priorityClass from HostedControlPlane for OVN and NNI#2769
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bradbehle: This pull request references CORENET-6276 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. DetailsIn 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
needs-ok-to-test
|
Hi @bradbehle. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
/retest-required |
| data.Data["HCPNodeSelector"] = bootstrapResult.OVN.OVNKubernetesConfig.HyperShiftConfig.HCPNodeSelector | ||
| data.Data["HCPLabels"] = bootstrapResult.OVN.OVNKubernetesConfig.HyperShiftConfig.HCPLabels | ||
| data.Data["HCPTolerations"] = bootstrapResult.OVN.OVNKubernetesConfig.HyperShiftConfig.HCPTolerations | ||
| data.Data["PriorityClass"] = bootstrapResult.Infra.HostedControlPlane.PriorityClass |
There was a problem hiding this comment.
I think this needs to be bootstrapResult.OVN.OVNKubernetesConfig.HyperShiftConfig.PriorityClass to match the rest of the code and may explain why PR tests are failing.
There was a problem hiding this comment.
I changed the code as you suggested and added the PriorityClass to the OVNHyperShiftBootstrapResult struct and used it from there.
bradbehle
force-pushed
the
ovn-nni-use-configured-prio-class
branch
from
dabae8c to
bb74597
Compare
|
/retest-required |
| productFlavor = "managed" | ||
| data.Data["CAConfigMap"] = bootstrapResult.OVN.OVNKubernetesConfig.HyperShiftConfig.CAConfigMap | ||
| data.Data["CAConfigMapKey"] = bootstrapResult.OVN.OVNKubernetesConfig.HyperShiftConfig.CAConfigMapKey | ||
| data.Data["PriorityClass"] = bootstrapResult.OVN.OVNKubernetesConfig.HyperShiftConfig.PriorityClass |
There was a problem hiding this comment.
It seems a bit odd to set this here rather than where data.Data["HCPTolerations"] is set but I don't think we get different results. I'll defer to Red Hat on the correct thing to do.
There was a problem hiding this comment.
Yeah, @bradbehle would you mind moving all 3 vars to the block that sets the HCP vars from bootstrap?
|
@rtheis: changing LGTM is restricted to collaborators DetailsIn 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 kubernetes-sigs/prow repository. |
|
/retest-required |
@bradbehle @csrwng Don't we also need it for cloud-network-config-controller? It is currently hard-coded as: |
openshift-merge-robot
added
the
needs-rebase
priorityClass can be specified in the HostedControlPlane for hypershift managed clusters. When it is specified, it should be used by the control plane components. Currently the cluster-network-operator deployment and the multus-admission-controller deployment do use the priority class that is specified, but the ovnkube-control-plane and network-node-identity deployments do not, they use a hardcoded value. This change updates the ovnkube-control-plane and network-node-identity deployments to use teh priorityClass from the HostedControlPlane if it is specified
Addressing review comment that suggested that PriorityClass be set in the OVNHyperShiftBootstrapResult and then read from there to match the other code.
bradbehle
force-pushed
the
ovn-nni-use-configured-prio-class
branch
from
bb74597 to
0d6da2f
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/lgtm |
openshift-ci
Bot
added
lgtm
|
/retest-required |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/test e2e-aws-ovn-upgrade |
|
/tide refresh |
|
/retest-required |
|
/override ci/prow/e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
|
@kyrtapz: Overrode contexts on behalf of kyrtapz: ci/prow/e2e-metal-ipi-ovn-dualstack-bgp-local-gw DetailsIn 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 kubernetes-sigs/prow repository. |
|
/override ci/prow/e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
|
@kyrtapz: Overrode contexts on behalf of kyrtapz: ci/prow/e2e-metal-ipi-ovn-dualstack-bgp-local-gw DetailsIn 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 kubernetes-sigs/prow repository. |
|
This is a hypershift specific change, overrding unrelated jobs (they passed in previous runs): |
|
@kyrtapz: Overrode contexts on behalf of kyrtapz: ci/prow/e2e-aws-ovn-upgrade, ci/prow/e2e-aws-ovn-upgrade-ipsec, ci/prow/e2e-aws-ovn-windows, ci/prow/e2e-gcp-ovn-upgrade, ci/prow/e2e-metal-ipi-ovn-dualstack-bgp, ci/prow/e2e-metal-ipi-ovn-ipv6, ci/prow/e2e-ovn-ipsec-step-registry DetailsIn 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 kubernetes-sigs/prow repository. |
|
/test hypershift-e2e-aks |
|
/retest-required |
|
/override ci/prow/e2e-ovn-ipsec-step-registry |
|
@kyrtapz: Overrode contexts on behalf of kyrtapz: ci/prow/e2e-ovn-ipsec-step-registry DetailsIn 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 kubernetes-sigs/prow repository. |
|
/override ci/prow/e2e-aws-ovn-upgrade |
|
@kyrtapz: Overrode contexts on behalf of kyrtapz: ci/prow/e2e-aws-ovn-upgrade DetailsIn 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 kubernetes-sigs/prow repository. |
|
/test hypershift-e2e-aks |
1 similar comment
|
/test hypershift-e2e-aks |
|
@bradbehle: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
https://redhat-external.slack.com/archives/C01C8502FMM/p1755787091649519 |
|
@kyrtapz: Overrode contexts on behalf of kyrtapz: ci/prow/hypershift-e2e-aks DetailsIn 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 kubernetes-sigs/prow repository. |
openshift-merge-bot
Bot
merged commit c765f72
into
openshift:master
|
[ART PR BUILD NOTIFIER] Distgit: cluster-network-operator |
6 participants
priorityClass can be specified in the HostedControlPlane for hypershift managed clusters. When it is specified, it should be used by the control plane components. Currently the cluster-network-operator deployment and the multus-admission-controller deployment do use the priority class that is specified, but the ovnkube-control-plane and network-node-identity deployments do not, they use a hardcoded value.
This change updates the ovnkube-control-plane and network-node-identity deployments to use teh priorityClass from the HostedControlPlane if it is specified