Implement cluster status command, fixes #9935 #11171

jim-minter · 2016-09-30T11:18:03Z

php-coder

BTW, it's a good practice to not put the GitHub issue number in the commit's comment because it can appears in the original issue many times if you're changing commit. Instead you can refer to the issue in the PR (also with keywords "Fixes"/"Closes" that will also close the issue when PR gets merged).

php-coder · 2016-09-30T11:27:22Z

pkg/bootstrap/docker/status.go

+		}
+	}
+
+	return errors.NewError("OpenShift cluster is not running")


It looks inconsistent that in case of success we're printing "container is running" and otherwise we're printing "cluster is not running". Let's use "cluster" in the success message too.

php-coder · 2016-09-30T11:28:29Z

pkg/bootstrap/docker/up.go

-	if len(c.DockerMachine) > 0 {
-		glog.V(2).Infof("Getting client for Docker machine %q", c.DockerMachine)
-		c.dockerClient, c.engineAPIClient, err = getDockerMachineClient(c.DockerMachine, out)
+// getDockerClient will obtain a new Docker client from the environment or


s/will obtain/obtains/

jim-minter · 2016-09-30T11:36:18Z

@php-coder thanks for the good practice; suggested changes made.

php-coder · 2016-09-30T11:48:41Z

@jim-minter Thanks!

Did you see that Travis fails with "FAILURE: Generated completions out of date. Please run hack/update-generated-completions.sh"?

jim-minter · 2016-09-30T11:52:23Z

Oh dear - thanks for pointing that out.

csrwng · 2016-09-30T12:11:58Z

Looks like you'll also need to run hack/update-generated-docs.sh

csrwng · 2016-09-30T12:28:53Z

@jim-minter if possible we should display additional information when running status:

URL to the web console
URL to metrics if you decided to add metrics
Host dir for config
Host dir for volumes
Host dir for data (or a message saying that it will be discarded if you didn't specify one)
Uptime ? (not that important)

Something like:

The cluster has been running for 30 minutes

Web console URL: https://127.0.0.1:8443/console
Metrics URL:     https://metrics-openshift-infra.1.2.3.4.xip.io/

Config is at host directory /var/lib/origin/openshift.local.config
Volumes are at host directory /var/lib/origin/openshift.local.volumes 
Data will be discarded when cluster is destroyed.

Mount and uptime information can be obtained by inspecting the origin container. URL and metrics info can be obtained from the master-config.yaml (you can get a copy by copying it from the origin container).

jim-minter · 2016-10-04T16:20:58Z

$ oc cluster status
The OpenShift cluster was started 20 minutes ago

Web console URL: https://192.168.121.57:8443
Metrics URL:     https://metrics-openshift-infra.192.168.121.57.xip.io/hawkular/metrics

Config is at host directory /var/lib/origin/openshift.local.config
Volumes are at host directory /var/lib/origin/openshift.local.volumes
Data will be discarded when cluster is destroyed

csrwng

@jim-minter some comments... overall it looks nice. I like the downloader/uploader code. I would think that could be useful outside of this context, so in the future we may want to move it to a package outside of the bootstrap package.

csrwng · 2016-10-11T00:52:16Z

pkg/bootstrap/docker/dockerhelper/filetransfer.go

+func StreamFileFromContainer(client *docker.Client, container, src string) (io.ReadCloser, error) {
+	downloader := newContainerDownloader(client, container, src)
+
+	for {


the for loop is not needed

csrwng · 2016-10-11T00:54:53Z

pkg/bootstrap/docker/dockerhelper/filetransfer.go

+// container.
+func DownloadDirFromContainer(client *docker.Client, container, src, dst string) error {
+	downloader := newContainerDownloader(client, container, src)
+	defer downloader.Close()


We should call the code to untar from here:
https://github.com/openshift/origin/blob/master/vendor/github.com/openshift/source-to-image/pkg/tar/tar.go
(to help with maintenance)

The main reason I didn't was that I'm stripping off the leading directory name when writing files from the incoming tar file, e.g. foo/bar/baz -> bar/baz. The s2i tar code doesn't currently allow this, and I was hoping to avoid the extract to temporary directory and moving files around alternative.

I could add the relevant functionality into source-to-image and pull it through, but I think it implies bit of refactoring to make it sit well. Adding a "strip leading directory" flag to ExtractTarStreamWithLogging didn't feel like a great option, and breaking the function apart to abstractions seemed like overkill. So I made a copy.

So the reason I suggested reuse is that this extract/compress code will need to work on Windows/Mac. We've already fixed an issue or 2 around that on the s2i code. My preference would be to still keep that as the one place where we do this. We should change it and make it more flexible if needed.

csrwng · 2016-10-11T00:56:51Z

pkg/bootstrap/docker/dockerhelper/filetransfer.go

+			return nil, err
+		}
+		if header.Name != filepath.Base(src) || (header.Typeflag != tar.TypeReg && header.Typeflag != tar.TypeRegA) {
+			return nil, errors.New("unexpected tar file content " + header.Name)


also include the TypeFlag in the error message

Also use the same tar code for the uploader below if possible.

csrwng · 2016-10-11T01:04:22Z

pkg/bootstrap/docker/host/host.go

-// CopyFromHost copies a set of files from the Docker host to the local file system
-func (h *HostHelper) CopyFromHost(sourceDir, destDir string) error {
+// DownloadDirFromContainer copies a set of files from the Docker host to the local file system
+func (h *HostHelper) DownloadDirFromContainer(sourceDir, destDir string) error {


If we're going to always mount the config dir from the container, then the function name should reflect that ... like DownloadConfigDir

I was hoping to leave these names more broad to signpost a single path for down/uploading arbitrary files from the container, if required in the future for whatever reason. If necessary this may entail adding in the other bind mounts up top, but I left those out for now. What's best to do here?

I see. Can we pass the bindmounts in and have a wrapper function that passes the config one in?

csrwng · 2016-10-11T01:13:34Z

pkg/bootstrap/docker/host/host.go

-	bind := fmt.Sprintf("%s:/var/lib/origin/openshift.local.config:z", destDir)
+// UploadFileToContainer copies a local file to the Docker host
+func (h *HostHelper) UploadFileToContainer(src, dst string) error {
+	bind := fmt.Sprintf("%s:/var/lib/origin/openshift.local.config:z", h.configDir)


Same here, if we're always mounting the local config, then the function name should reflect that.

csrwng · 2016-10-11T01:15:26Z

pkg/bootstrap/docker/status.go

+		Long:    cmdStatusLong,
+		Example: fmt.Sprintf(cmdStatusExample, fullName),
+		Run: func(c *cobra.Command, args []string) {
+			kcmdutil.CheckErr(config.Status(f, out))


we should reuse the code to print the error that we have here:
https://github.com/openshift/origin/blob/master/pkg/bootstrap/docker/printer.go#L71
(otherwise additional error details like the cause don't get printed out)

csrwng · 2016-10-11T01:16:26Z

pkg/bootstrap/docker/status.go

+		return err
+	}
+	if !healthy {
+		return notRunning


If the container is running but not healthy, we should say so

csrwng · 2016-10-11T01:17:53Z

pkg/bootstrap/docker/status.go

+	DockerMachine string
+}
+
+// Status prints the developer cluster status


s/developer/OpenShift/

jim-minter · 2016-10-12T11:07:06Z

[test]

csrwng · 2016-10-12T15:00:53Z

#8571
[test]

jim-minter · 2016-10-13T09:27:14Z

Rebased.
flake #11240
[test]

jim-minter · 2016-10-13T13:07:26Z

flake #11240 again

csrwng · 2016-10-13T13:48:07Z

Hi @jim-minter, please rename your s2i bump commit to follow the bump naming convention:
#9128

jim-minter · 2016-10-13T13:52:07Z

@csrwng done

csrwng · 2016-10-17T21:42:36Z

#9548
#9490
[merge]

csrwng · 2016-10-19T11:12:44Z

#11438
[merge]

openshift-bot · 2016-10-20T05:45:20Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10377/) (Image: devenv-rhel7_5216)

csrwng · 2016-10-20T10:50:05Z

@jim-minter please rebase and regenerate docs and completions

jim-minter · 2016-10-20T14:57:06Z

@bparees @csrwng I have rebased this to sit on top of the s2i bump in #11414 which we're trying to get through.

PI-Victor · 2016-10-20T16:10:30Z

Looks like you'll also need to run hack/update-generated-docs.sh

or make update that takes care of everything :)

bparees · 2016-10-20T16:11:41Z

@PI-Victor careful w/ make update. it'll regen your protobufs and if you're on go1.7, it'll generate them wrong.

jim-minter · 2016-10-20T16:26:43Z

@bparees is that protobuf problem #10696 (which is now closed)?

bparees · 2016-10-20T16:42:36Z

@jim-minter no. different issue. go1.7 and go 1.6 protogen produce different outputs (both valid, but the verification check will complain that they are not the same)

jim-minter · 2016-10-20T16:49:04Z

[test]

jim-minter · 2016-10-20T19:20:24Z

[test]

csrwng · 2016-10-20T19:25:27Z

@jim-minter the PR for the logging flag did merge. I'll leave it up to you if you want to add the code to display the logging URL in this PR or we just create a new one after this one merges.

jim-minter · 2016-10-20T20:14:13Z

@csrwng I'll be glad once this one's merged! Logging URL code added.

csrwng · 2016-10-20T20:23:04Z

@jim-minter me too :) LGTM pending tests passing

jim-minter · 2016-10-20T20:48:38Z

flake #11004
[test]

bparees · 2016-10-21T05:01:47Z

@jim-minter well the s2i bump went in but your commits from that bump in this PR don't seem to have gone away like i would have expected. I think you may need to do another rebase and/or drop those commits from your branch.

jim-minter · 2016-10-21T07:03:28Z

@bparees yes, I rebased yesterday to fix the merge conflicts up front. Now I've rebased again and it should be good to go.

openshift-bot · 2016-10-21T07:05:33Z

Evaluated for origin test up to 66ae0b7

openshift-bot · 2016-10-21T08:27:39Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10377/) (Base Commit: f60c1df)

csrwng · 2016-10-21T09:45:14Z

[merge]

openshift-bot · 2016-10-21T09:45:28Z

Evaluated for origin merge up to 66ae0b7

jim-minter assigned csrwng Sep 30, 2016

php-coder suggested changes Sep 30, 2016

View reviewed changes

jim-minter force-pushed the issue9935 branch from 1bcc603 to b601d08 Compare September 30, 2016 11:35

php-coder approved these changes Sep 30, 2016

View reviewed changes

jim-minter force-pushed the issue9935 branch from b601d08 to 8ce8f13 Compare September 30, 2016 11:52

jim-minter force-pushed the issue9935 branch from 8ce8f13 to adae020 Compare September 30, 2016 12:15

jim-minter force-pushed the issue9935 branch from adae020 to b59a884 Compare October 4, 2016 16:19

jorgemoralespou mentioned this pull request Oct 10, 2016

Use upstream command for status once available openshift-evangelists/oc-cluster-wrapper#22

Closed

csrwng reviewed Oct 11, 2016

View reviewed changes

jim-minter force-pushed the issue9935 branch 3 times, most recently from 20c235d to 1ca3d84 Compare October 12, 2016 11:06

jim-minter mentioned this pull request Oct 13, 2016

cluster up: add option to install logging components #11343

Merged

jim-minter force-pushed the issue9935 branch from 1ca3d84 to f0af553 Compare October 13, 2016 09:26

jim-minter force-pushed the issue9935 branch from f0af553 to b158859 Compare October 13, 2016 13:51

csrwng mentioned this pull request Oct 14, 2016

oc cluster console #11355

Closed

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

jim-minter force-pushed the issue9935 branch from b158859 to 8e1f5bd Compare October 20, 2016 14:56

jim-minter force-pushed the issue9935 branch from 8e1f5bd to d62dec7 Compare October 20, 2016 16:47

jim-minter removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2016

jim-minter force-pushed the issue9935 branch from d62dec7 to 6077d9b Compare October 20, 2016 19:19

jim-minter force-pushed the issue9935 branch from 6077d9b to 863fc0d Compare October 20, 2016 20:13

Implement cluster status command

66ae0b7

jim-minter force-pushed the issue9935 branch from 863fc0d to 66ae0b7 Compare October 21, 2016 07:02

openshift-bot merged commit 643f3f8 into openshift:master Oct 21, 2016

Implement cluster status command, fixes #9935 #11171

Implement cluster status command, fixes #9935 #11171

Conversation

jim-minter commented Sep 30, 2016 • edited by php-coder Loading

php-coder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jim-minter commented Sep 30, 2016

php-coder commented Sep 30, 2016 • edited Loading

jim-minter commented Sep 30, 2016

csrwng commented Sep 30, 2016

csrwng commented Sep 30, 2016

jim-minter commented Oct 4, 2016

csrwng left a 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

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

jim-minter commented Oct 12, 2016

csrwng commented Oct 12, 2016

jim-minter commented Oct 13, 2016

jim-minter commented Oct 13, 2016

csrwng commented Oct 13, 2016

jim-minter commented Oct 13, 2016

csrwng commented Oct 17, 2016

csrwng commented Oct 19, 2016

openshift-bot commented Oct 20, 2016 • edited Loading

csrwng commented Oct 20, 2016

jim-minter commented Oct 20, 2016

PI-Victor commented Oct 20, 2016

bparees commented Oct 20, 2016

jim-minter commented Oct 20, 2016

bparees commented Oct 20, 2016

jim-minter commented Oct 20, 2016

jim-minter commented Oct 20, 2016

csrwng commented Oct 20, 2016

jim-minter commented Oct 20, 2016

csrwng commented Oct 20, 2016

jim-minter commented Oct 20, 2016

bparees commented Oct 21, 2016 • edited Loading

jim-minter commented Oct 21, 2016

openshift-bot commented Oct 21, 2016

openshift-bot commented Oct 21, 2016

csrwng commented Oct 21, 2016

openshift-bot commented Oct 21, 2016

jim-minter commented Sep 30, 2016 •

edited by php-coder

Loading

php-coder commented Sep 30, 2016 •

edited

Loading

openshift-bot commented Oct 20, 2016 •

edited

Loading

bparees commented Oct 21, 2016 •

edited

Loading