CORS-2814: Add support for in-cluster DNS on Cloud Platforms when cloud DNS cannot be used #4018
Conversation
sadasu
force-pushed
the
custom-dns
branch
5 times, most recently
from
af6e921 to
cd53a07
Compare
sadasu
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
sadasu
force-pushed
the
custom-dns
branch
2 times, most recently
from
ea46d07 to
e5a1767
Compare
sadasu
force-pushed
the
custom-dns
branch
4 times, most recently
from
b1585f0 to
92bdbd0
Compare
sadasu
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest-required |
|
@cybertron, @mkowalski could PTAL ? |
sadasu
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sadasu: This pull request references CORS-2814 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.16.0" version, but no target version was set. 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. |
|
@sadasu: This pull request references CORS-2814 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.16.0" version, but no target version was set. 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. |
|
@sadasu: This pull request references CORS-2814 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.16.0" version, but no target version was set. 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. |
1 similar comment
|
@sadasu: This pull request references CORS-2814 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.16.0" version, but no target version was set. 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 |
|
/retest-required |
| @@ -277,12 +277,52 @@ spec: | |||
| description: url is fully qualified URI with scheme https, that overrides the default generated endpoint for a client. This must be provided and cannot be empty. | |||
| type: string | |||
| pattern: ^https:// | |||
| x-kubernetes-list-type: atomic | |||
There was a problem hiding this comment.
controllerconfig CRD lives now in openshift/api , you will need to update https://github.com/openshift/api/tree/master/machineconfiguration/v1 and vendor latest changes here.
There was a problem hiding this comment.
@sinnykumari ControllerConfig in openshift/api has already been updated: https://github.com/openshift/api/blob/master/machineconfiguration/v1/0000_80_controllerconfig-TechPreviewNoUpgrade.crd.yaml#L1629 and vendored https://github.com/openshift/machine-config-operator/blob/master/vendor/github.com/openshift/api/machineconfiguration/v1/0000_80_controllerconfig-TechPreviewNoUpgrade.crd.yaml#L1064
I ran make update on my mco branch and that resulted in this manifest being updated. It appears that it is unnecessary. So, I will remove this from my commit.
There was a problem hiding this comment.
ah ok. @cdoern is this expected?
There was a problem hiding this comment.
@cdoern the same manifest is updated when I run make update on master. So, nothing specific to this PR. Removed this from my commit.
There was a problem hiding this comment.
This is expected. Make update brings in the new manifests and copies them where necessary. There is a hack script that lists all of the CRDs we copy.
Generate the CoreDNS pod definition and Corefile for cloud platforms when their DNSType is specified to be `ClusterHosted`. Currently implemented just for GCP.
The in-cluster CoreDNS pods for cloud platforms would be running in this namespace.
Start CoreDNS pod for some cloud platforms when their DNSType is `ClusterHosted`. Currently implemented for GCP. Cloud default Load Balancers are used so there is no need to start in-cluster Load Balancers.
|
/retest-required |
|
/retest |
There was a problem hiding this comment.
Overall this looks fine to me.
I don't have much expertise in technical details going on here, I assume that @cybertron has taken care of that part.
Before adding approval, few sanity check questions:
- Linked enhancement CORS-2062: Customer configured DNS for cloud platforms AWS, Azure and GCP enhancements#1468 got closed. Is this design already been agreed upon with needed people?
- Do we need review from additional domain expert people?
- We usually do pre-merge testing of feature. Are you working with any QE for verifying this feature or this will get tested later by corresponding QE?
There was a problem hiding this comment.
As per OCPBUGS-26951, if you are adding new CoreDNS pods you should also annotate them for the workload partitioning. Look at https://github.com/openshift/machine-config-operator/pull/4143/files to see how it's done for on-prem platforms
Yes, the design has been agreed upon. The enhancement has to be updated. Implementation took priority over that.
I think the experts on on-prem CoreDNS @cybertron and @mkowalski are already involved.
Installer QE is testing this feature on 4.15. Hence, the backport PR #4155 has been created. |
Adding `PreferredDuringScheduling` annotation to the cloud platform coreDNS static pods in order to use reserved CPUs according to workload partitioning.
|
/lgtm |
|
Thanks, this seems to be a feature at the moment. Features are usually not backported, you may require staff-eng approval to that. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cybertron, mkowalski, sadasu, sinnykumari 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 |
openshift-ci
bot
added
the
approved
|
@sadasu: The following test 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/test-infra repository. I understand the commands that are listed here. |
openshift-merge-bot
bot
merged commit 030bcf7
into
openshift:master
- What I did
Based on enhancement : openshift/enhancements#1468
- How to verify it
Verification still in-progress.
- Description for the changelog
These changes allow MCO to read an optional configmap containing the LB IPs of the LBs configured on cloud platforms, to be used to generate in-cluster DNS for API and API-Int URLs.
Contains implementation for CORS-2814, CORS-2815 and CORS-3169