support imagePullSecrets and imagePullPolicy in kubefed init #50740

dixudx · 2017-08-16T07:14:16Z

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #50718

Special notes for your reviewer:
/assign @gyliu513

Release note:

support imagePullSecrets and imagePullPolicy in kubefed init

k8s-ci-robot · 2017-08-16T07:14:23Z

Hi @dixudx. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

gyliu513 · 2017-08-16T09:18:45Z

/cc @kubernetes/sig-federation-pr-reviews

gyliu513 · 2017-08-16T09:24:34Z

federation/pkg/kubefed/init/init.go

@@ -159,6 +161,8 @@ type initFederationOptions struct {
 func (o *initFederationOptions) Bind(flags *pflag.FlagSet, defaultServerImage, defaultEtcdImage string) {
 	flags.StringVar(&o.dnsZoneName, "dns-zone-name", "", "DNS suffix for this federation. Federated Service DNS names are published with this suffix.")
 	flags.StringVar(&o.serverImage, "image", defaultServerImage, "Image to use for federation API server and controller manager binaries.")
+	flags.StringVar(&o.imagePullPolicy, "imagePullPolicy", string(api.PullIfNotPresent), "PullPolicy describes a policy for if/when to pull a container image. The default pull policy is IfNotPresent which will not pull an image if it already exists.")


I'd prefer we use image-pull-policy as the flag

gyliu513 · 2017-08-16T09:24:45Z

federation/pkg/kubefed/init/init.go

@@ -159,6 +161,8 @@ type initFederationOptions struct {
 func (o *initFederationOptions) Bind(flags *pflag.FlagSet, defaultServerImage, defaultEtcdImage string) {
 	flags.StringVar(&o.dnsZoneName, "dns-zone-name", "", "DNS suffix for this federation. Federated Service DNS names are published with this suffix.")
 	flags.StringVar(&o.serverImage, "image", defaultServerImage, "Image to use for federation API server and controller manager binaries.")
+	flags.StringVar(&o.imagePullPolicy, "imagePullPolicy", string(api.PullIfNotPresent), "PullPolicy describes a policy for if/when to pull a container image. The default pull policy is IfNotPresent which will not pull an image if it already exists.")
+	flags.StringVar(&o.imagePullSecrets, "imagePullSecrets", "", "Provide secrets that can access the private registry.")


I'd prefer we use image-pull-secrets as the flag

SGTM. Thanks.

gyliu513 · 2017-08-16T09:25:04Z

federation/pkg/kubefed/init/init.go

@@ -361,7 +365,7 @@ func (i *initFederation) Run(cmdOut io.Writer, config util.AdminConfig) error {

 	fmt.Fprint(cmdOut, "Creating federation component deployments...")
 	glog.V(4).Info("Creating federation control plane components")
-	_, err = createAPIServer(hostClientset, i.commonOptions.FederationSystemNamespace, serverName, i.commonOptions.Name, i.options.serverImage, i.options.etcdImage, advertiseAddress, serverCredName, i.options.apiServerEnableHTTPBasicAuth, i.options.apiServerEnableTokenAuth, i.options.apiServerOverrides, pvc, i.options.dryRun)
+	_, err = createAPIServer(hostClientset, i.commonOptions.FederationSystemNamespace, serverName, i.commonOptions.Name, i.options.serverImage, i.options.imagePullPolicy, i.options.imagePullSecrets, i.options.etcdImage, advertiseAddress, serverCredName, i.options.apiServerEnableHTTPBasicAuth, i.options.apiServerEnableTokenAuth, i.options.apiServerOverrides, pvc, i.options.dryRun)


Put new parameters to the end, ditto for others

@gyliu513 I was thinking to do like this. But current params of createAPIServer seem to get grouped by their type. Directly appending new params will break current style. WDYT?

gyliu513 · 2017-08-16T09:31:24Z

federation/pkg/kubefed/init/init_test.go

@@ -263,6 +275,7 @@ func TestInitFederation(t *testing.T) {
 		cmd.Flags().Set("dns-zone-name", tc.dnsZoneName)
 		cmd.Flags().Set("image", tc.serverImage)
 		cmd.Flags().Set("etcd-image", tc.etcdImage)
+		cmd.Flags().Set("imagePullPolicy", tc.imagePullPolicy)


losing image pull secrets here?

adding it in below L295.

gyliu513 · 2017-08-16T09:31:38Z

federation/pkg/kubefed/init/init_test.go

@@ -279,6 +292,9 @@ func TestInitFederation(t *testing.T) {
 		if tc.etcdPersistence != "true" {
 			cmd.Flags().Set("etcd-persistent-storage", tc.etcdPersistence)
 		}
+		if tc.imagePullSecrets != "" {


also need to check image pull policy

imagePullPolicy is set to a default value, just like serverImage. Still need checking?

gyliu513 · 2017-08-16T09:32:34Z

/ok-to-test

dixudx · 2017-08-16T13:06:52Z

@gyliu513 Updated. PTAL. Thanks.

gyliu513 · 2017-08-16T15:59:44Z

federation/pkg/kubefed/init/init_test.go

@@ -621,7 +637,7 @@ func TestCertsHTTPS(t *testing.T) {
 	}
 }

-func fakeInitHostFactory(apiserverServiceType v1.ServiceType, federationName, namespaceName, advertiseAddress, lbIp, dnsZoneName, serverImage, etcdImage, dnsProvider, dnsProviderConfig, etcdPersistence, etcdPVCapacity, etcdPVStorageClass, apiserverOverrideArg, cmOverrideArg, tmpDirPath string, apiserverEnableHTTPBasicAuth, apiserverEnableTokenAuth, isRBACAPIAvailable bool) (cmdutil.Factory, error) {
+func fakeInitHostFactory(apiserverServiceType v1.ServiceType, federationName, namespaceName, advertiseAddress, lbIp, dnsZoneName, serverImage, imagePullPolicy, imagePullSecrets, etcdImage, dnsProvider, dnsProviderConfig, etcdPersistence, etcdPVCapacity, etcdPVStorageClass, apiserverOverrideArg, cmOverrideArg, tmpDirPath string, apiserverEnableHTTPBasicAuth, apiserverEnableTokenAuth, isRBACAPIAvailable bool) (cmdutil.Factory, error) {


put new added parameters to the end as well.

gyliu513

/lgtm

/cc @kubernetes/sig-federation-pr-reviews

dixudx · 2017-08-17T09:37:51Z

/assign @nikhiljindal @quinton-hoole
PTAL. Need your /approval. Thanks.

dixudx · 2017-08-22T01:52:07Z

/test pull-kubernetes-kubemark-e2e-gce

dixudx · 2017-08-27T00:39:41Z

/test pull-kubernetes-unit

ghost · 2017-10-02T18:49:39Z

/approve

dixudx · 2017-10-03T02:04:41Z

/retest

dixudx · 2017-10-03T06:36:22Z

ping @gyliu513 Rebased. Needs your lgtm again. Thanks.

irfanurrehman · 2017-10-19T19:49:12Z

/retest

irfanurrehman · 2017-10-19T19:54:09Z

/assign

dixudx · 2017-10-20T09:29:15Z

ping @irfanurrehman @gyliu513 PTAL. Needs your lgtm. Thanks.

gyliu513 · 2017-10-20T09:41:34Z

/lgtm

k8s-github-robot · 2017-10-20T09:41:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dixudx, gyliu513, quinton-hoole

Associated issue: 50718

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~federation/OWNERS~~ [quinton-hoole]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-10-20T09:41:37Z

/test all

Tests are more than 96 hours old. Re-running tests.

k8s-github-robot · 2017-10-20T11:19:38Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-10-20T12:06:23Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot assigned gyliu513 Aug 16, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 16, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 16, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 16, 2017

dixudx force-pushed the kubefed_add_imagepull branch from 3007cbe to 9d6db2d Compare August 16, 2017 08:03

k8s-ci-robot added the sig/federation label Aug 16, 2017

gyliu513 reviewed Aug 16, 2017

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 16, 2017

dixudx force-pushed the kubefed_add_imagepull branch from 9d6db2d to a9d6a52 Compare August 16, 2017 13:06

gyliu513 reviewed Aug 16, 2017

View reviewed changes

dixudx force-pushed the kubefed_add_imagepull branch from a9d6a52 to 869c91d Compare August 17, 2017 01:25

gyliu513 reviewed Aug 17, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2017

k8s-ci-robot assigned nikhiljindal and ghost Aug 17, 2017

ghost added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Oct 2, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2017

support imagePullSecrets and imagePullPolicy in kubefed init

5b7f2b3

dixudx force-pushed the kubefed_add_imagepull branch from 869c91d to 5b7f2b3 Compare October 3, 2017 06:35

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2017

csbell added sig/multicluster Categorizes an issue or PR as relevant to SIG Multicluster. area/federation and removed sig/federation labels Oct 6, 2017

k8s-ci-robot assigned irfanurrehman Oct 19, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2017

k8s-github-robot merged commit 1e67fea into kubernetes:master Oct 20, 2017

dixudx deleted the kubefed_add_imagepull branch October 20, 2017 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support imagePullSecrets and imagePullPolicy in kubefed init #50740

support imagePullSecrets and imagePullPolicy in kubefed init #50740

dixudx commented Aug 16, 2017

k8s-ci-robot commented Aug 16, 2017

gyliu513 commented Aug 16, 2017

gyliu513 Aug 16, 2017

gyliu513 Aug 16, 2017

dixudx Aug 16, 2017

gyliu513 Aug 16, 2017

dixudx Aug 16, 2017

gyliu513 Aug 16, 2017

dixudx Aug 16, 2017

gyliu513 Aug 16, 2017

dixudx Aug 16, 2017

gyliu513 commented Aug 16, 2017

dixudx commented Aug 16, 2017

gyliu513 Aug 16, 2017

gyliu513 left a comment

dixudx commented Aug 17, 2017

dixudx commented Aug 22, 2017

dixudx commented Aug 27, 2017

ghost commented Oct 2, 2017

dixudx commented Oct 3, 2017

dixudx commented Oct 3, 2017

irfanurrehman commented Oct 19, 2017

irfanurrehman commented Oct 19, 2017

dixudx commented Oct 20, 2017

gyliu513 commented Oct 20, 2017

k8s-github-robot commented Oct 20, 2017

k8s-github-robot commented Oct 20, 2017

k8s-github-robot commented Oct 20, 2017

k8s-github-robot commented Oct 20, 2017

support imagePullSecrets and imagePullPolicy in kubefed init #50740

support imagePullSecrets and imagePullPolicy in kubefed init #50740

Conversation

dixudx commented Aug 16, 2017

k8s-ci-robot commented Aug 16, 2017

gyliu513 commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyliu513 commented Aug 16, 2017

dixudx commented Aug 16, 2017

Choose a reason for hiding this comment

gyliu513 left a comment

Choose a reason for hiding this comment

dixudx commented Aug 17, 2017

dixudx commented Aug 22, 2017

dixudx commented Aug 27, 2017

ghost commented Oct 2, 2017

dixudx commented Oct 3, 2017

dixudx commented Oct 3, 2017

irfanurrehman commented Oct 19, 2017

irfanurrehman commented Oct 19, 2017

dixudx commented Oct 20, 2017

gyliu513 commented Oct 20, 2017

k8s-github-robot commented Oct 20, 2017

k8s-github-robot commented Oct 20, 2017

k8s-github-robot commented Oct 20, 2017

k8s-github-robot commented Oct 20, 2017