-
Notifications
You must be signed in to change notification settings - Fork 189
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
OPNET-133: Support remote worker #858
OPNET-133: Support remote worker #858
Conversation
tsorya
commented
Nov 15, 2022
•
edited
Loading
edited
Skipping CI for Draft Pull Request. |
209252b
to
3be2d3f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll also need to update hashableDeployment
and deploymentConfigChanged
. Please also add a unit test in TestDeploymentConfigChanged
.
Isn't having remote worker nodes already a supported feature? Is the "node-role.kubernetes.io/remote-worker" label new? This change implies that running router pods on nodes with that label is strictly prohibited, but if it was possible to do it before, then it would be a regression to prohibit it now.
13cf1e9
to
a788853
Compare
a788853
to
b634d2d
Compare
It was not possible previously as we don't support remote worker at all for now |
e614a77
to
e1a1e31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some minor suggestions or questions around comments. Looks fine overall.
@@ -592,6 +592,21 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController | |||
nodeSelector["node-role.kubernetes.io/master"] = "" | |||
default: | |||
nodeSelector["node-role.kubernetes.io/worker"] = "" | |||
// Disabling running on remote workers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Disabling running on remote workers | |
// Disabling running on remote workers. |
Is there any intention to allow scheduling routers on remote workers in some future release?
@@ -40,6 +40,8 @@ const ( | |||
// DefaultCanaryNamespace is the default namespace for | |||
// the ingress canary check resources. | |||
DefaultCanaryNamespace = "openshift-ingress-canary" | |||
|
|||
RemoteWorkerLabel = "node.openshift.io/remote-worker" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind adding godoc for this definition?
Defining the label here is fine as a temporary measure, but will it eventually be defined in openshift/api or the like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope yes, currently we want to start using it and there is no actual need to add it to openshift-api right a way.
But yes, when we will start increasing usage, we will move it to openshift-api
Adding node affinity rule to router deployement that will not allow router to run on remote worker node (node with custom role label)
e1a1e31
to
4e7f152
Compare
Thanks! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah 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 |
/retest |
/retest-required |
2 similar comments
/retest-required |
/retest-required |
e2e-aws-operator failed on deprovisioning, which is a known problem with CI on AWS. Because the job failed due to a known issue that is unrelated to this PR and because the e2e-azure-operator and e2e-gcp-operator jobs succeeded, I'm overriding the e2e-aws-operator job. |
@Miciah: Overrode contexts on behalf of Miciah: ci/prow/e2e-aws-operator 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 kubernetes/test-infra repository. |
@tsorya: 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/test-infra repository. I understand the commands that are listed here. |
/label qe-approved
after adding label
|
/label docs-approved |
/label px-approved |