Bug 1946506: Add hosts template for monitor-based resolution#2465
Bug 1946506: Add hosts template for monitor-based resolution#2465openshift-merge-robot merged 3 commits intoopenshift:masterfrom
Conversation
|
This is dependent on openshift/baremetal-runtimecfg#129. I suspect it may not pass ci until that merges. |
|
/retest |
|
/test e2e-openstack metal-ipi is currently broken, but the runtimecfg patch merged so this should be good to go whenever it starts working again. |
openshift-ci-robot
requested review from
Gal-Zaidman,
jcpowermac,
mandre and
patrickdillon
|
/test e2e-ovirt |
|
/lgtm oVirt seems to be ok with the change :) |
|
/retest |
|
Ping: @mandre @jcpowermac @patrickdillon |
|
/test e2e-metal-ipi |
|
Reviewing the e2e-vsphere logs the test failures are unrelated. Upgrade is failing at MCO which might indicate a problem - I will look more in depth tomorrow. |
MCD having trouble with image registry pod: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_machine-config-operator/2465/pull-ci-openshift-machine-config-operator-master-e2e-vsphere-upgrade/1377258688363892736/artifacts/e2e-vsphere-upgrade/gather-extra/artifacts/pods/openshift-machine-config-operator_machine-config-daemon-nzw6c_machine-config-daemon.log It's covered by: https://bugzilla.redhat.com/show_bug.cgi?id=1944762 It's hitting some jobs incl vsphere |
There was a problem hiding this comment.
Seems like we no longer need the rule for mdns in the security groups and it should be removed from the installer. Are we tracking this somewhere?
There was a problem hiding this comment.
Ah, no I hadn't considered that because baremetal doesn't have security groups. What would be the best way to make sure that doesn't get lost?
There was a problem hiding this comment.
Normally, I would say in the same BZ that tracks the removal of mdns, or just a patch in the installer, but with only 3 days to feature freeze I think it's unrealistic to expect both patch to merge before FF. I'll create the BZ.
There was a problem hiding this comment.
We're now tracking mdns removal in https://bugzilla.redhat.com/show_bug.cgi?id=1946506. Aside from allowing to merge past feature freeze, this also ensures QE looks at and verifies the code change.
There was a problem hiding this comment.
As it stands right now, we do. My thought was that after this and #2410 merged we could remove mdns-publisher completely, but right now some platforms are still going to be using mdns.
Given how close we are to feature freeze now, maybe I need to pull 2410 into this PR and do everything at once.
There was a problem hiding this comment.
Are the two changes of updating the path of the kubeconfig file and removing the mdns plugin related? If not they should probably go in separate PRs or at least separate commits.
There was a problem hiding this comment.
Yes, in order for the coredns-monitor to read the node list and populate this new template we need the kubelet kubeconfig. The old one doesn't have permission to read nodes.
|
/test e2e-metal-ipi |
There was a problem hiding this comment.
Normally, I would say in the same BZ that tracks the removal of mdns, or just a patch in the installer, but with only 3 days to feature freeze I think it's unrealistic to expect both patch to merge before FF. I'll create the BZ.
|
/retitle Bug 1946506: Add hosts template for monitor-based resolution |
openshift-ci-robot
changed the title
openshift-ci-robot
added
the
bugzilla/severity-medium
|
@cybertron: This pull request references Bugzilla bug 1946506, which is invalid:
Comment 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/test-infra repository. |
openshift-ci-robot
added
the
bugzilla/invalid-bug
|
/bugzilla refresh |
openshift-ci-robot
added
the
bugzilla/valid-bug
|
/test e2e-metal-ipi 403 downloading go. o.O |
|
/test e2e-metal-ipi |
|
/test e2e-aws I think the vsphere-upgrade and okd jobs are known flaky, but it would be nice to get an e2e-aws pass. |
|
/test e2e-aws I'm seeing the same errors in other PRs which eventually merged, so I don't think it's related to this one. |
|
/test e2e-aws |
|
Okay, this is now passing all of the expected jobs. The only change since the previous lgtms was adding fallthrough to fix external DNS resolution. |
|
@mandre @Gal-Zaidman @jcpowermac Can someone lgtm this again? It should be good to go now, and this fixes a pretty big scaling issue with on-prem. Thanks! |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cybertron, Gal-Zaidman, JAORMX, kikisdeliveryservice, mandre, yboaron The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
10 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@cybertron: Some pull requests linked via external trackers have merged: The following pull requests linked via external trackers have not merged:
These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Once unlinked, request a bug refresh with Bugzilla bug 1946506 has not been moved to the MODIFIED state. 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/test-infra repository. |
mDNS was removed in openshift#2465, but a couple of the related templates were missed. This just cleans those up too.
11 participants
Now that node resolution is not required for bootstrapping, we no
longer need to use mdns to build the list of nodes. We can just wait
until the api is up and retrieve it from there.
This change removes the mdns plugin from the coredns config and
replaces it with a hosts plugin template which will be populated
by the coredns-monitor.
- Description for the changelog
Removed mdns node resolution in favor of a kubernetes api-based implementation.