store cluster version into clusterdeployment status #120

joelddiaz · 2018-11-30T19:01:58Z

when we have a kubeconfig for the remote cluster, fetch the ClusterVersion.Status and store it in ClusterDeployment.Status.ClusterVersionStatus

handle nil values for fields in clusterVersion.Status (so we can pass object validation)

update test cases to work with new need to mock up remote ClusterVersion object

refactor the building of a remotecluster kubeclient (and update remotemachineset controller accordingly)

build on top of #127 to get beyond the gomock issues

dgoodwin · 2018-12-03T12:06:47Z

pkg/apis/hive/v1alpha1/clusterdeployment_types.go


+	// ClusterVersion locally stores the remote cluster's clusterversion.status.current.version field
+	ClusterVersion string `json:"clusterVersion"`
+


Realistically I think treating this as just a string will probably get us into a spot where we need to break API compatibility in the near future. We should make this a struct, the Update struct also has a Payload which looks really interesting for our purposes as well and we should.

Should we pull the FULL ClusterVersionStatus? Or just make our own and copy over parts of it? Looking through https://github.com/openshift/api/blob/master/config/v1/types_cluster_version.go#L75 we're not really interested in the Generation or VersionHash but the Conditions and the AvailableUpdates could be extremely useful. I'd be tempted to re-use their type and copy the whole thing.

Thoughts?

CC @csrwng in case he has ideas as well.

I agree, I think there's usefulness in embedding the entire status struct

dgoodwin · 2018-12-03T12:11:28Z

pkg/controller/clusterdeployment/clusterdeployment_controller.go

 	object.SetFinalizers(finalizers.List())
 }
+
+func buildRemoteClusterAPIClient(secretData string) (client.Client, error) {


We should probably get this refactored out to a common utility as this is probably the second usage.

dgoodwin · 2018-12-03T12:12:13Z

pkg/controller/clusterdeployment/clusterdeployment_controller_test.go

-				assert.Equal(t, cd.Status.APIURL, "https://bar-api.clusters.example.com:6443")
-				assert.Equal(t, cd.Status.WebConsoleURL, "https://bar-api.clusters.example.com:6443/console")
+				assert.Equal(t, "https://bar-api.clusters.example.com:6443", cd.Status.APIURL)
+				assert.Equal(t, "https://bar-api.clusters.example.com:6443/console", cd.Status.WebConsoleURL)


Good catch.

csrwng · 2018-12-03T13:53:25Z

pkg/controller/clusterdeployment/clusterdeployment_controller.go

+			return err
+		}
+
+		cdLog.Debugf("remote desired version for cluster %v: %+v", cd.Name, remoteClusterVersion.Status.Current)


If you want to report the desired version, it should be remoteCusterVersion.Spec.DesiredUpdate.Version

yep. i meant to print the current version out of status, not the desired version in spec

joelddiaz · 2018-12-06T21:11:07Z

Now storing the entirety of the clusterversion.Status as suggested.

Have a temporary hack commit to get around needing to allow empty clusterversion.Status.AvailableVersion[]. Waiting for openshift/api#143 (or something like it) to merge, and then can re-vendor openshift/api.

dgoodwin · 2018-12-07T13:04:08Z

/retest

dgoodwin · 2018-12-07T13:08:56Z

Looks like a legit compile error, missing a package.

dgoodwin · 2018-12-10T12:18:53Z

Still a problem, looks like a missing omit empty.

dgoodwin · 2018-12-11T12:01:05Z

/retest

when we have a kubeconfig for the remote cluster, fetch the ClusterVersion.Status and store it in ClusterDeployment.Status.ClusterVersionStatus handle nil values for fields in clusterVersion.Status (so we can pass object validation) update test cases to work with new need to mock up remote ClusterVersion object refactor the building of a remotecluster kubeclient (and update remotemachineset controller accordingly)

joelddiaz · 2018-12-11T17:46:10Z

/retest

dgoodwin · 2018-12-11T18:02:48Z

/lgtm

joelddiaz requested a review from dgoodwin November 30, 2018 19:02

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 30, 2018

openshift-ci-robot requested review from abutcher and csrwng November 30, 2018 19:02

joelddiaz force-pushed the clusterversionstatus branch from 755d0ab to afaf711 Compare November 30, 2018 19:54

joelddiaz changed the title ~~WIP: store cluster version into clusterdeployment status~~ store cluster version into clusterdeployment status Nov 30, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 30, 2018

dgoodwin reviewed Dec 3, 2018

View reviewed changes

csrwng reviewed Dec 3, 2018

View reviewed changes

joelddiaz force-pushed the clusterversionstatus branch from afaf711 to 77acb73 Compare December 6, 2018 21:08

joelddiaz force-pushed the clusterversionstatus branch from 77acb73 to 2b188c5 Compare December 7, 2018 14:40

joelddiaz force-pushed the clusterversionstatus branch 3 times, most recently from 0cd0688 to db32e31 Compare December 10, 2018 21:58

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 10, 2018

joelddiaz force-pushed the clusterversionstatus branch from db32e31 to e453029 Compare December 10, 2018 22:29

joelddiaz force-pushed the clusterversionstatus branch from d97a1dd to 2cdff7e Compare December 11, 2018 15:39

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 11, 2018

joelddiaz force-pushed the clusterversionstatus branch 2 times, most recently from 6220829 to 6b498e0 Compare December 11, 2018 16:32

openshift-ci-robot assigned dgoodwin Dec 11, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2018

openshift-merge-robot merged commit c1d6545 into openshift:master Dec 11, 2018

joelddiaz mentioned this pull request Dec 11, 2018

Please add the OpenShift version of the cluster to the clusterdeployment status #77

Closed

joelddiaz deleted the clusterversionstatus branch January 15, 2019 19:31


		// ClusterVersion locally stores the remote cluster's clusterversion.status.current.version field
		ClusterVersion string `json:"clusterVersion"`

store cluster version into clusterdeployment status #120

store cluster version into clusterdeployment status #120

Uh oh!

Conversation

joelddiaz commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dgoodwin Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

csrwng Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

dgoodwin Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

dgoodwin Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

csrwng Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

joelddiaz Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

joelddiaz commented Dec 6, 2018

Uh oh!

dgoodwin commented Dec 7, 2018

Uh oh!

dgoodwin commented Dec 7, 2018

Uh oh!

dgoodwin commented Dec 10, 2018

Uh oh!

dgoodwin commented Dec 11, 2018

Uh oh!

joelddiaz commented Dec 11, 2018

Uh oh!

dgoodwin commented Dec 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joelddiaz commented Nov 30, 2018 •

edited

Loading