Tracked addition of federation, sed support in kube DNS #28040
Conversation
The kube DNS app recently gained support for federation (whatever that is), including a new Salt parameter. It also gained alternate templates, intended to be friendly to `sed`. Fortunately, those do not demand a federation parameter.
|
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
4 similar comments
|
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
|
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
|
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
|
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
k8s-github-robot
added
size/XS
|
This is a bug fix, no release note needed. |
roberthbailey
added
release-note-none
|
/cc @quinton-hoole FYI (since this broke due to federation changes). |
roberthbailey
added
the
lgtm
|
This looks like a cherry pick candidate because otherwise this will be broken on the 1.3 release branch. |
|
GCE e2e build/test passed for commit e12fa40. |
|
@madhusudancs Could you take a look at this please? |
ghost
assigned 
| sed -e "s/{{ pillar\['dns_replicas'\] }}/${DNS_REPLICAS}/g;s/{{ pillar\['dns_domain'\] }}/${DNS_DOMAIN}/g;" "${KUBE_ROOT}/cluster/saltbase/salt/kube-dns/skydns-rc.yaml.in" > skydns-rc.yaml | ||
| sed -e "s/{{ pillar\['dns_server'\] }}/${DNS_SERVER_IP}/g" "${KUBE_ROOT}/cluster/saltbase/salt/kube-dns/skydns-svc.yaml.in" > skydns-svc.yaml | ||
| sed -e "s/\\\$DNS_REPLICAS/${DNS_REPLICAS}/g;s/\\\$DNS_DOMAIN/${DNS_DOMAIN}/g;" "${KUBE_ROOT}/cluster/saltbase/salt/kube-dns/skydns-rc.yaml.sed" > skydns-rc.yaml | ||
| sed -e "s/\\\$DNS_SERVER_IP/${DNS_SERVER_IP}/g" "${KUBE_ROOT}/cluster/saltbase/salt/kube-dns/skydns-svc.yaml.in" > skydns-svc.yaml |
There was a problem hiding this comment.
Should you also not switch to the .sed script here for consistency?
There was a problem hiding this comment.
I think so. The first time I looked I didn't see the other .sed file but on second look it's right there.
|
Minor nit, LGTM otherwise. |
roberthbailey
removed
the
lgtm
|
The skydns-rc.yaml.base file is transformed into the .in and .sed templates so that its easy for changes to the base file to propagate to various formats. Any changes to the .sed templates might break the Ubuntu case again, so for people making changes to the DNS templates to have visibility into your consumption of this template, one suggestion is to add your own skydns-rc.yaml.ubuntu generated file and transforms2ubuntu.sed script and add a Makefile target. That would make it a visible file in this directory. |
|
(1) Oops, I see I forgot to edit the file extension for the second substitution; I will fix that. (2) I am surprised by the suggestion to change the Makefile to refer specifically to the cluster/ubuntu/deployAddons.sh script. I did a little search for "skydns-rc.yaml", and found many other files that also seem to know the required set of substitutions. The beauty of the sed files is that they can be used by multiple scripts. It would seem tedious and odd to make special template files for each script. Perhaps instead we should add a comment somewhere reminding developers to do that search for scripts that use the templates? |
My original change neglected to change the template from the salt one to the sed one.
Added a friendly note, with a suggestion of how to find the scripts.
k8s-github-robot
removed
the
size/XS
k8s-github-robot
added
the
size/S
|
GCE e2e build/test passed for commit 6aeb5da. |
|
GCE e2e build/test passed for commit 33e176e. |
|
LGTM for now. |
madhusudancs
added
the
lgtm
|
GCE e2e build/test failed for commit 33e176e. Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake. |
|
@k8s-bot test this issue: #IGNORE |
|
GCE e2e build/test passed for commit 33e176e. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit 33e176e. |
|
Automatic merge from submit-queue |
erictune
added
cherry-pick-approved
Automatic merge from submit-queue Tracked addition of federation, sed support in kube DNS []() The kube DNS app recently gained support for federation (whatever that is), including a new Salt parameter. This broke the deployAddons.sh script for cluster ubuntu. The DNS app also gained alternate templates, intended to be friendly to `sed`. Fortunately, those do not demand a federation parameter. This PR fixes up the ` cluster/ubuntu/deployAddons.sh` script to track those changes, by switching to the `sed`-friendly templates. (cherry picked from commit 794dcc1)
Automatic merge from submit-queue Batch update for 1.3 #28030: Revert "Federation e2e supports aws" #28026: Address outstanding review comments in #27999. #28034: Adding lock files for kubeconfig updating #28004: return nil from NewClientConfig instead of empty struct #28032: Increase pod CPU/memory for fluentd, dns and kube-proxy. #27208: Bump minimum API version for docker to 1.21 #28061: Remove extra double quotes in --federations. #28060: rkt: Fix the 'privileged' check when stage1 annotation is provided. #27996: Image GC logic should compensate for reserved blocks #28044: rkt: Bump required rkt version to 1.9.1. #28040: Tracked addition of federation, sed support in kube DNS #28043: Set grace period to 0 when deleting namespaces after the test. #28002: Fix startup type error in initializeCaches #28087: Hotfix: Fixup the hyperkube dns manifest from a breaking federation PR #28108: Fix initialization of volume controller caches. #28056: Increase kube-dns requirements on CoreOS. #28147: Fix error checks after cloning. #28159: Use : as seccomp security option operator for Docker 1.10 #28165: Refactored, expanded and fixed federated-services e2e tests. #28095: Kubelet should mark VolumeInUse before checking if it is Attached #28172: Build: Add KUBE_GCS_RELEASE_BUCKET_MIRROR option to push-ci-build.sh #28207: Bump cluster autoscaler to 0.2.2 #28160: Volume manager must verify containers terminated before deleting for ungracefully terminated pods #28211: Fix federation e2e tests by correctly managing cluster clients #27944: Fix pvc label selector validation error #28186: federation: Upgrading the groupversion to v1beta1
|
Commit found in the "release-1.3" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked. |
The kube DNS app recently gained support for federation (whatever that
is), including a new Salt parameter. This broke the deployAddons.sh script for cluster ubuntu. The DNS app also gained alternate
templates, intended to be friendly to
sed. Fortunately, those donot demand a federation parameter.
This PR fixes up the
cluster/ubuntu/deployAddons.shscript to track those changes, by switching to thesed-friendly templates.