Registry daemonset #7596

pweil- · 2016-02-24T18:24:24Z

Adds the ability to run the registry as a daemonset via the oadm command.

TODO:

~~if this looks ok then we will add the output of this command as a template in contrib~~ This command is pretty complicated now. I don't think a template is high priority. Edit: confirmed that template is not a high priority and not necessary for this PR.
~~test-cmd tests to exercise the option~~

deads2k · 2016-02-24T18:31:42Z

pkg/cmd/admin/registry/registry.go

 	cmd.Flags().StringVar(&cfg.Credentials, "credentials", "", "Path to a .kubeconfig file that will contain the credentials the registry should use to contact the master.")
 	cmd.Flags().StringVar(&cfg.ServiceAccount, "service-account", cfg.ServiceAccount, "Name of the service account to use to run the registry pod.")
 	cmd.Flags().StringVar(&cfg.Selector, "selector", cfg.Selector, "Selector used to filter nodes on deployment. Used to run registries on a specific set of nodes.")
+	cmd.Flags().BoolVar(&cfg.DaemonSet, "daemonset", cfg.DaemonSet, "Use a daemonset instead of a deployment config.")


This is not specific to this pull, but do we have a plan for rolling out updates to daemonsets like DC's allow?

Orchestration of daemonset updates are in progress upstream at:

kubernetes/kubernetes#18319
kubernetes/kubernetes#19627

miminar · 2016-02-25T15:11:15Z

pkg/cmd/admin/registry/registry.go

+		// which only supports DCs.
+		if cfg.DaemonSet {
+			daemonSet := generateDaemonSet(podTemplate, name, label)
+			objects = replaceDeploymentConfig(objects, daemonSet)


Can't we just append desired object in both cases instead of replacing unwanted?

The referenced code for adding a service would need to change - it only supports DCs at this point. That isn't a huge deal in and of itself but I hesitated to make the change. I can look in to it though.

soltysh · 2016-02-26T14:36:02Z

I'd really, really, really want all our commands follow upstream Complete-Validate-Run scheme. Since you're touching this command can you please refactor it appropriately? I just did that with image-import in #7581.

pweil- · 2016-02-26T14:36:53Z

I'd really, really, really want all our commands follow upstream Complete-Validate-Run scheme. Since you're touching this command can you please refactor it appropriately? I just did that with image-import in #7581.

sure, I'll take a look since I'm already refactoring it.

soltysh · 2016-02-26T14:37:07Z

🎉

pweil- · 2016-04-06T18:57:07Z

given the changes to the command since this was created and the conflicts when rebasing I decided to limit this to just what is needed to run as a daemonset for now and take a second pass at refactoring. Issue for follow up: #8390

@miminar @soltysh PTAL again. Thanks

pweil- · 2016-04-06T19:21:26Z

[test]

miminar · 2016-04-07T08:07:59Z

LGTM. re-[test]

soltysh · 2016-04-08T11:58:43Z

LGTM

pweil- · 2016-04-08T12:03:34Z

[test]

soltysh · 2016-04-08T14:45:20Z

[test]

soltysh · 2016-04-08T19:42:26Z

[test]

pweil- · 2016-05-02T12:28:05Z

rebased and re[test]

pweil- · 2016-05-02T13:37:02Z

any other comments here? @deads2k would you mind taking a glance at the e2e additions?

deads2k · 2016-05-02T14:49:32Z

pkg/cmd/admin/registry/registry.go

 	}

+	// if we're using a daemon set we'll add it here and remove the DC.
+	// We allow the dc to be created above to get the goodness of app.AddServices


That doesn't look too bad to fix. Why wouldn't we make the update in that function?

deads2k · 2016-05-02T14:52:34Z

test/cmd/admin.sh

+os::cmd::expect_success_and_text "oadm registry --daemonset -o yaml --credentials=${KUBECONFIG}" 'DaemonSet'
+os::cmd::expect_success "oadm registry --daemonset --credentials=${KUBECONFIG} --images='${USE_IMAGES}'"
+os::cmd::expect_success_and_text 'oadm registry --daemonset' 'service exists'
+os::cmd::expect_success_and_text 'oc describe ds/docker-registry' 'Desired Number of Nodes Scheduled:\s*1'


oc get ds/docker-registry with a template will be more stable.

deads2k · 2016-05-02T14:53:30Z

Minor comments. lgtm otherwise.

0xmichalis · 2016-05-02T15:02:12Z

We should document the fact that there is no automatic rolling update functionality currently. Even the initial implementation discussed upstream will be in the client. Oh, and we don't even use kubectl rolling-update (only the underlying rolling updater in our dc strategy)

0xmichalis · 2016-05-02T15:03:37Z

@smarterclayton do we have any plans for having process-based deployments for DaemonSets?

deads2k · 2016-05-02T15:06:57Z

@smarterclayton do we have any plans for having process-based deployments for DaemonSets?

Rolling update is in the graduation list: kubernetes/kubernetes#15310

smarterclayton · 2016-05-02T15:10:35Z

We need a "generic trigger" controller for image streams

On Mon, May 2, 2016 at 11:07 AM, David Eads notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton do we have any plans
for having process-based deployments for DaemonSets?

Rolling update is in the graduation list: kubernetes/kubernetes#15310
kubernetes/kubernetes#15310

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#7596 (comment)

pweil- · 2016-05-18T17:14:23Z

@deads2k updates for your comments in commit two. If that looks good I'll squash an merge. Thanks!

deads2k · 2016-05-18T17:33:19Z

pkg/generate/app/pipeline.go

+			// the labels on the pod template
+			selector := t.Spec.Template.Labels
+			if t.Spec.Selector != nil {
+				selector = t.Spec.Selector.MatchLabels


I don't think we default in this direction though. The daemonset node selector can be destroyed via defaulting, but the pod template spec (the one that the service has to match) is never defaulted, right?

Right, (in deployments/deploymentconfigs and I imagine it's the same for daemonsets) you can have a nil selector and it will be defaulted to the podtemplate labels.

I don't think we default in this direction though. The daemonset node selector can be destroyed via defaulting, but the pod template spec (the one that the service has to match) is never defaulted, right?

Seen the error of my ways regarding node selection, but I still think we should base our service selector on the podtemplatespec labels, not the ds labels.

Still seems weird to me that your service selector would not be the exact same selector that the DS uses to determine what pods it is managing. If we create it based on pod labels the only weirdness that could happen is that a DS is still managing the pod but it no longer appears in the service. I don't think we can run into a scenario where a pod not managed by the DS shows up in the service without some deliberate updates to the service.

Will change.

pweil- · 2016-05-18T19:22:07Z

squashed. Will merge on green unless there are any other comments

pweil- · 2016-05-18T19:22:52Z

We should document the fact that there is no automatic rolling update functionality currently

doc'd in openshift/openshift-docs#2118

deads2k · 2016-05-18T19:27:26Z

lgtm

soltysh · 2016-05-18T20:06:13Z

Still LGTM.

pweil- · 2016-05-18T20:06:47Z

re[test]

pweil- · 2016-05-18T20:53:10Z

[merge]

openshift-bot · 2016-05-18T21:15:17Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5944/) (Image: devenv-rhel7_4236)

openshift-bot · 2016-05-19T12:10:14Z

Evaluated for origin merge up to 899062e

openshift-bot · 2016-05-19T12:10:22Z

Evaluated for origin test up to 899062e

openshift-bot · 2016-05-19T13:15:12Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3915/)

deads2k reviewed Feb 24, 2016
View reviewed changes

pweil- force-pushed the registry-daemonset branch from a16537a to 5654f67 Compare February 24, 2016 18:37

miminar reviewed Feb 25, 2016
View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2016

pweil- force-pushed the registry-daemonset branch from 5654f67 to 63ef8d1 Compare April 6, 2016 18:54

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2016

pweil- force-pushed the registry-daemonset branch 2 times, most recently from bb87e04 to c5ce23d Compare April 6, 2016 19:20

miminar mentioned this pull request Apr 7, 2016

Flake in kubectl e2e test #8397

Closed

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 22, 2016

pweil- force-pushed the registry-daemonset branch from c5ce23d to 8291d9e Compare May 2, 2016 12:27

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2016

deads2k reviewed May 2, 2016
View reviewed changes

deads2k reviewed May 18, 2016
View reviewed changes

pweil- mentioned this pull request May 18, 2016

registry daemonset option openshift/openshift-docs#2118

Merged

pweil- force-pushed the registry-daemonset branch 2 times, most recently from 2be1972 to d19a633 Compare May 18, 2016 19:21

run registry as daemonset

899062e

pweil- force-pushed the registry-daemonset branch from d19a633 to 899062e Compare May 19, 2016 12:07

openshift-bot merged commit 27e2696 into openshift:master May 19, 2016

pweil- mentioned this pull request Jun 14, 2016

Edit section on daemonsets openshift/openshift-docs#2242

Merged

Registry daemonset #7596

Registry daemonset #7596

Uh oh!

Conversation

pweil- commented Feb 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soltysh commented Feb 26, 2016

Uh oh!

pweil- commented Feb 26, 2016

Uh oh!

soltysh commented Feb 26, 2016

Uh oh!

pweil- commented Apr 6, 2016

Uh oh!

pweil- commented Apr 6, 2016

Uh oh!

miminar commented Apr 7, 2016

Uh oh!

soltysh commented Apr 8, 2016

Uh oh!

pweil- commented Apr 8, 2016

Uh oh!

soltysh commented Apr 8, 2016

Uh oh!

soltysh commented Apr 8, 2016

Uh oh!

pweil- commented May 2, 2016

Uh oh!

pweil- commented May 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented May 2, 2016

Uh oh!

0xmichalis commented May 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xmichalis commented May 2, 2016

Uh oh!

deads2k commented May 2, 2016

Uh oh!

smarterclayton commented May 2, 2016

Uh oh!

pweil- commented May 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pweil- commented May 18, 2016

Uh oh!

pweil- commented May 18, 2016

Uh oh!

deads2k commented May 18, 2016

Uh oh!

soltysh commented May 18, 2016

Uh oh!

pweil- commented May 18, 2016

Uh oh!

pweil- commented May 18, 2016

Uh oh!

openshift-bot commented May 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xmichalis commented May 2, 2016 •

edited

Loading

openshift-bot commented May 18, 2016 •

edited

Loading