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

Registry Addon Fixup #2332

Merged
merged 3 commits into from Mar 22, 2018
Merged

Conversation

hswong3i
Copy link
Contributor

Integrate both changes from:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 14, 2018
@hswong3i hswong3i force-pushed the registry_fixup branch 12 times, most recently from 40dd69f to 5d4da31 Compare February 17, 2018 09:54
@ghost
Copy link

ghost commented Feb 18, 2018

While we are here could you check network plugin and change hostPort to hostNetwork in case plugin is calico?

@ghost
Copy link

ghost commented Feb 18, 2018

Is it okay to hard code .cluster.local in REGISTRY_HOST when cluster domain might be different?

@hswong3i
Copy link
Contributor Author

I am using kubespray default calico for all of my development, so could confirm that hostport works as expected?

@ghost
Copy link

ghost commented Feb 18, 2018

As far as I know calico doesn't support hostPort, to be sure I've tested this again and it didn't work.

Edit:
From https://kubernetes.io/docs/concepts/cluster-administration/network-plugins/

Limitation: Due to kubernetes/kubernetes#31307, HostPort won’t work with CNI networking plugin at the moment. That means all hostPort attribute in pod would be simply ignored.

@hswong3i
Copy link
Contributor Author

That’s strange... I do that for Nginx Ingress (#2324) too with hostport but it works ?_?

@ghost
Copy link

ghost commented Feb 18, 2018

With brief look through issues seems like this issue hasn't fixed yet or it has very recently

@hswong3i
Copy link
Contributor Author

hswong3i commented Feb 18, 2018

@ghost
Copy link

ghost commented Feb 19, 2018

@hswong3i Great, that's good news, so there is no need for hostNetwork

@hswong3i
Copy link
Contributor Author

hswong3i commented Mar 2, 2018

ping @Atoms need you review it, thanks

@hswong3i
Copy link
Contributor Author

hswong3i commented Mar 8, 2018

@Atoms already running this patch in my production for almost 2 weeks and looks stable enough, may you give a hand for review?

@hswong3i hswong3i force-pushed the registry_fixup branch 2 times, most recently from 1cf6811 to dbba0b4 Compare March 8, 2018 03:58
@hswong3i
Copy link
Contributor Author

may someone give a hand and so let it go?

@woopstar
Copy link
Member

ci check this

@hswong3i
Copy link
Contributor Author

ci not run...

@hswong3i
Copy link
Contributor Author

@woopstar i had been using this patch for almost a month and all looks good enough for my production, would you mind to give a hand for review?

anyway, CI not running this...

@ghost
Copy link

ghost commented Mar 22, 2018

Why nobody is paying any attention

@rsmitty
Copy link
Contributor

rsmitty commented Mar 22, 2018

ci check this

@rsmitty
Copy link
Contributor

rsmitty commented Mar 22, 2018

I kicked CI for this PR. Sorry for the delay on it. CI isn't actually going to test the deployment of the registry, but it'll at least make sure the clusters still deploy successfully given these changes to the code base. Pending that, I'll merge it in.

@hswong3i
Copy link
Contributor Author

CI looks good now~

@rsmitty rsmitty merged commit 4175431 into kubernetes-sigs:master Mar 22, 2018
@hswong3i
Copy link
Contributor Author

@rsmitty thank you very much~~~

@hswong3i hswong3i deleted the registry_fixup branch March 22, 2018 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants