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

DNS templates cleanups #29720

Closed
wants to merge 5 commits into from
Closed

Conversation

thockin
Copy link
Member

@thockin thockin commented Jul 28, 2016

This is a set of cleanups to the DNS templates that are used to configure the kubedns addon. It should be reviewed commit-by-commit.

Missing from this are a handful of directories that still have old references to "skydns" names.

@errordeveloper : you are the major author of docs/getting-started-guides/coreos/azure/addons/skydns-*. These files are ANCIENT forks of the DNS config files. We can not really have this in our docs. Can we find a way to pull from the real template files? This is in both the main repo and teh docs repo while docs are converting.

@yifan-gu : you are the major author of cluster/gce/coreos/kube-manifests/addons/dns/skydns-*. These files are forks of the DNS config files, and are drifting out of sync. We can not really have this. Can we find a way to pull from the real template files instead?

@mbruzek @chuckbutler : you are the major authors of cluster/juju/layers/kubernetes/templates/skydns-*. These files are ANCIENT forks of the DNS config files. We can not really have this. Can we find a way to pull from the real template files instead?

@sdminonne : you are the major author of cluster/libvirt-coreos/skydns-*. These files are ANCIENT forks of the DNS config files. We can not really have this. Can we find a way to pull from the real template files instead?


This change is Reviewable

@thockin thockin added the release-note-none Denotes a PR that doesn't merit a release note. label Jul 28, 2016
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. retest-not-required-docs-only and removed retest-not-required-docs-only labels Jul 28, 2016
@sdminonne
Copy link
Contributor

@thockin agree: what we have today for libvirt is not reliable. Having a look to see if we can pull the real ones. Thanks

@mbruzek
Copy link
Contributor

mbruzek commented Jul 28, 2016

@thockin Yes we can look at ways to keep current with the skydns files. We use templating logic to pass in user defined configuration so we will have to investigate what we can do.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2016
@girishkalele
Copy link

LGTM

for the config variables mentioned above. Other than the templating, these are
normal kubernetes objects, and can be instantiated with `kubectl create`.
the example files
([ReplicationController](../../cluster/addns/dns/kubedns-rc.yaml.sed) and
Copy link
Contributor

Choose a reason for hiding this comment

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

@thockin typo: ../../cluster/addons/dns/...

mbruzek pushed a commit to mbruzek/kubernetes that referenced this pull request Aug 2, 2016
@thockin thockin removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 3, 2016
@thockin
Copy link
Member Author

thockin commented Aug 3, 2016

Girish said LGTM

@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2016
k8s-github-robot pushed a commit that referenced this pull request Aug 4, 2016
Automatic merge from submit-queue

libvirt_coreos: to remove old skydns fork fetching from addons

@thockin @girishkalele  ref #29720.
Goal of this PR is: 

- to remove obsolete DNS config files.
- to propose a way to fetch the real template

As soon #29720 will be merged I'm OK to modify this accordingly.

FYI: @lhuard1A
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2016
@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2016
k8s-github-robot pushed a commit that referenced this pull request Aug 4, 2016
Automatic merge from submit-queue

Replacing skydns with kubedns for the juju cluster. #29720

```release-note
* Updating the cluster/juju provider to use kubedns in place of skydns.
```
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2016
Rename *.in to *.jinja - it's clearer and more precise.

Fix a couple cases to use the .sed files rather than the .in files.
@girishkalele
Copy link

@thockin I guess this was just a cleanup item, can go into the next release.

cc @MrHohn

@thockin
Copy link
Member Author

thockin commented Sep 13, 2016

I've resigned myself to needing to basically recreate this after other
changes land :)

On Tue, Sep 13, 2016 at 10:11 AM, Girish Kalele notifications@github.com
wrote:

@thockin https://github.com/thockin I guess this was just a cleanup
item, can go into the next release.

cc @MrHohn https://github.com/MrHohn


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#29720 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVP6U2NqAATXWCJ_oVCEF04U1WZvJks5qptlJgaJpZM4JW23S
.

@yifan-gu
Copy link
Contributor

yifan-gu commented Sep 17, 2016

@thockin The plan is to use some facilities in https://github.com/kubernetes/kubernetes/blob/master/cluster/gce/trusty/configure-helper.sh#L864-L888
(which you also updated in this PR)
But I don't think I have a chance to to that until rkt's integration with CRI landed.

/cc @euank , since you are reworking the kube-up for coreos/rkt using hyperkube image, you might have a chance to touch this.

@euank
Copy link
Contributor

euank commented Sep 29, 2016

Thanks for the ping @yifan-gu, I'm working on getting our addons to use the shared base now

@euank
Copy link
Contributor

euank commented Oct 3, 2016

PR above, I deleted the cluster/gce/coreos copies of the files and pull from the templates as GCI does.

Once that's merged, that copy of the DNS templates should be much more maintainable (aka gone) :)

@k8s-github-robot
Copy link

ok to test
@k8s-bot test this

pr builder appears to be missing, activating due to 'lgtm' label.

8 similar comments
@k8s-github-robot
Copy link

ok to test
@k8s-bot test this

pr builder appears to be missing, activating due to 'lgtm' label.

@k8s-github-robot
Copy link

ok to test
@k8s-bot test this

pr builder appears to be missing, activating due to 'lgtm' label.

@k8s-github-robot
Copy link

ok to test
@k8s-bot test this

pr builder appears to be missing, activating due to 'lgtm' label.

@k8s-github-robot
Copy link

ok to test
@k8s-bot test this

pr builder appears to be missing, activating due to 'lgtm' label.

@k8s-github-robot
Copy link

ok to test
@k8s-bot test this

pr builder appears to be missing, activating due to 'lgtm' label.

@k8s-github-robot
Copy link

ok to test
@k8s-bot test this

pr builder appears to be missing, activating due to 'lgtm' label.

@k8s-github-robot
Copy link

ok to test
@k8s-bot test this

pr builder appears to be missing, activating due to 'lgtm' label.

@k8s-github-robot
Copy link

ok to test
@k8s-bot test this

pr builder appears to be missing, activating due to 'lgtm' label.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 6dbfe2a. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 6dbfe2a. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 6dbfe2a. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 6dbfe2a. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@thockin thockin closed this Nov 2, 2016
@thockin thockin deleted the dns-templates branch November 2, 2016 06:35
@thockin thockin restored the dns-templates branch November 2, 2016 06:35
k8s-github-robot pushed a commit that referenced this pull request Dec 20, 2016
…ud-init

Automatic merge from submit-queue

Coreos kube-up now with less cloud init

This update includes significant refactoring. It moves almost all of the
logic into bash scripts, modeled after the `gci` cluster scripts.

The reason to do this is:
1. Avoid duplicating the saltbase manifests by reusing gci's parsing logic (easier maintenance)
2. Take an incremental step towards sharing more code between gci/trusty/coreos, again for better maintenance
3. Pave the way for making future changes (e.g. improved rkt support, kubelet support) easier to share

The primary differences from the gci scripts are the following:
1. Use of the `/opt/kubernetes` directory over `/home/kubernetes`
2. Support for rkt as a runtime
3. No use of logrotate
4. No use of `/etc/default/`
5. No logic related to noexec mounts or gci-specific firewall-stuff

It will make sense to move 2 over to gci, as well as perhaps a few other small improvements. That will be a separate PR for ease of review.

Ref #29720, this is a part of that because it removes a copy of them.

Fixes #24165

cc @yifan-gu 

Since this logic largely duplicates logic from the gci folder, it would be nice if someone closely familiar with that gave an OK or made sure I didn't fall into any gotchas related to that, so cc @andyzheng0831
@thockin thockin deleted the dns-templates branch August 14, 2019 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet