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

Introduce node selector support for HostedClusters #1592

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

enxebre
Copy link
Member

@enxebre enxebre commented Jul 20, 2022

What this PR does / why we need it:
This enables the use case where we want all product workloads to be allocated into infra nodes.
The PR includes refactor of support/config/deployment:

  • Make setControlPlaneIsolation, setColocation and setMultizoneSpread private.
  • Introduces atomic public setLocation.
  • Introduces setReplicas.
  • Introduces public setDefaults.
  • Clarify comments and structured based on PodAfinnity vs NodeAfinnity.
  • Add unit test for setLocation.
  • Update docs.

Follow up refactor:

  • Include PriorityClass and setDefaultSecurityContext in the SetDefaults signature so enforce every deployment has all expected values consistently.
  • Pass labels with deployment hash and setMultizoneSpread unconditionally so we don't skew setup.
  • Drop hypershif-operator/util

Some considerations:
Instead of proposed plain map[string]string nodeSelector, do we prefer a nodePlacement field so we preserve our ability to extend it in the future?

Which issue(s) this PR fixes *(optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where https://issues.redhat.com/browse/HOSTEDCP-506

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

@enxebre
Copy link
Member Author

enxebre commented Jul 20, 2022

/hold
Need to do some testing.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 20, 2022
@netlify
Copy link

netlify bot commented Jul 20, 2022

Deploy Preview for hypershift-docs ready!

Name Link
🔨 Latest commit 35bb814
🔍 Latest deploy log https://app.netlify.com/sites/hypershift-docs/deploys/62dfa7045d3df40008169de0
😎 Deploy Preview https://deploy-preview-1592--hypershift-docs.netlify.app/reference/api
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@openshift-ci openshift-ci bot requested review from alvaroaleman and sjenning July 20, 2022 14:37
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 20, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2022
@enxebre enxebre force-pushed the cp-infra-nodes branch 2 times, most recently from dec42a0 to 9af7e83 Compare July 20, 2022 15:24
@enxebre
Copy link
Member Author

enxebre commented Jul 20, 2022

/test verify

@enxebre
Copy link
Member Author

enxebre commented Jul 20, 2022

/retest

In addition:
- Pods for a Hosted Cluster prefer to be scheduled into the same Node.
- If the `ControllerAvailabilityPolicy` is `HighlyAvailable` Pods for each Deployment within a Hosted Cluster will require to be scheduled across different failure domains by setting `topology.kubernetes.io/zone` as the topology key.
- A HostedCluster can require their Pods to be scheduled into particlar Nodes by setting `HostedCluster.spec.nodeSelector`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it makes sense to give examples here for infra nodes?

@enxebre enxebre force-pushed the cp-infra-nodes branch 2 times, most recently from 26d3c0f to fb4e842 Compare July 21, 2022 12:32
@enxebre
Copy link
Member Author

enxebre commented Jul 21, 2022

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 21, 2022
@enxebre enxebre force-pushed the cp-infra-nodes branch 2 times, most recently from b296d06 to 779669e Compare July 21, 2022 18:15
@enxebre
Copy link
Member Author

enxebre commented Jul 22, 2022

/retest

1 similar comment
@enxebre
Copy link
Member Author

enxebre commented Jul 22, 2022

/retest

c.setNodeSelector(hcp)
c.setControlPlaneIsolation(hcp)
c.setColocation(hcp)
c.setMultizoneSpread(multiZoneSpreadLabels)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be called if the deployment has replicas > 1, otherwise it prevents single replica deployment rollouts.

@enxebre enxebre force-pushed the cp-infra-nodes branch 5 times, most recently from 82fab47 to 43b16e3 Compare July 26, 2022 10:02
@enxebre
Copy link
Member Author

enxebre commented Jul 26, 2022

/test e2e-aws

@enxebre
Copy link
Member Author

enxebre commented Jul 27, 2022

/test e2e-aws-nested

2 similar comments
@enxebre
Copy link
Member Author

enxebre commented Jul 27, 2022

/test e2e-aws-nested

@enxebre
Copy link
Member Author

enxebre commented Jul 27, 2022

/test e2e-aws-nested

@enxebre
Copy link
Member Author

enxebre commented Aug 2, 2022

/test unit

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 2, 2022

@enxebre: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws 43b16e3 link false /test e2e-aws

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.

@enxebre
Copy link
Member Author

enxebre commented Aug 2, 2022

/test e2e-aws-nested

This enables the use case where we want all product workloads to be allocated into infra nodes.
The PR includes refactor of support/config/deployment:
- Make setControlPlaneIsolation, setColocation and setMultizoneSpread private.
- Introduces atomic public setLocation.
- Introduces setReplicas.
- Introduces public setDefaults.
- Clarify comments and structured based on PodAfinnity vs NodeAfinnity.
- Add unit test for setLocation.
- Update docs.
@enxebre
Copy link
Member Author

enxebre commented Aug 2, 2022

/test unit

1 similar comment
@enxebre
Copy link
Member Author

enxebre commented Aug 2, 2022

/test unit

@csrwng
Copy link
Contributor

csrwng commented Aug 2, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2022
@openshift-merge-robot openshift-merge-robot merged commit aadcf32 into openshift:main Aug 2, 2022
enxebre added a commit to enxebre/hypershift that referenced this pull request Aug 3, 2022
Following up to openshift#1592, this keeps refactoring dropping hypershif-operator/util in favour of support/ and consolidates legacy HO reconciliation for mapprover and autoscaler with PCO reconciliation.
enxebre added a commit to enxebre/hypershift that referenced this pull request Aug 3, 2022
Following up to openshift#1592, this keeps refactoring dropping hypershif-operator/util in favour of support/ and consolidates legacy HO reconciliation for mapprover and autoscaler with PCO reconciliation.
enxebre added a commit to enxebre/hypershift that referenced this pull request Aug 3, 2022
Following up to openshift#1592, this keeps refactoring dropping hypershif-operator/util in favour of support/ and consolidates legacy HO reconciliation for mapprover and autoscaler with PCO reconciliation.
enxebre added a commit to enxebre/hypershift that referenced this pull request Aug 3, 2022
Following up to openshift#1592, this keeps refactoring dropping hypershif-operator/util in favour of support/ and consolidates legacy HO reconciliation for mapprover and autoscaler with PCO reconciliation.
enxebre added a commit to enxebre/hypershift that referenced this pull request Aug 3, 2022
Following up to openshift#1592, this keeps refactoring dropping hypershif-operator/util in favour of support/ and consolidates legacy HO reconciliation for mapprover and autoscaler with PCO reconciliation.
enxebre added a commit to enxebre/hypershift that referenced this pull request Aug 4, 2022
Following up to openshift#1592, this keeps refactoring dropping hypershif-operator/util in favour of support/ and consolidates legacy HO reconciliation for mapprover and autoscaler with PCO reconciliation.
enxebre added a commit to enxebre/hypershift that referenced this pull request Aug 4, 2022
Following up to openshift#1592, this keeps refactoring dropping hypershif-operator/util in favour of support/ and consolidates legacy HO reconciliation for mapprover and autoscaler with PCO reconciliation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants