Cluster image set #241

csrwng · 2019-03-04T16:03:25Z

No description provided.

dgoodwin · 2019-03-04T16:05:03Z

config/manifests/01_clusterdeployment_crd.yaml

+                      applied to the created MachineSet's MachineSpec. This list will
+                      overwrite any modifications made to Node labels on an ongoing
+                      basis.
+                    type: object


What's causing these to show up in here?

me not running make manifests ... will be fixed in next push

dgoodwin · 2019-03-04T16:07:14Z

config/samples/hive_v1alpha1_clusterimageset.yaml

+  name: clusterimageset-sample
+spec:
+  hive:
+    pullSpec: openshift/hive:latest


Are these real images? Haven't seen them before. If not we should use the CI ones here perhaps.

no, purely made up... will replace with valid ones. This is just the crd sample.

dgoodwin · 2019-03-04T16:07:47Z

pkg/apis/hive/v1alpha1/clusterimageset_types.go

+
+// ClusterImageSetSpec defines the desired state of ClusterImageSet
+type ClusterImageSetSpec struct {
+	// hiveImage is the Hive image to use when installing or destroying a cluster.


Case in the comments looks wrong here, should it match the go var name case or does it not matter? (status as well if so)

So in recent times, the case of new api type comments has been changed to reflect the names of the external json field names, instead of the Go names, given that those comments are used for external documentation (as in https://github.com/openshift/api/blob/master/operator/v1/types.go#L141-L155). However, that is not very evenly enforced. In our openshift API I see mostly the Go-style casing. I think that whatever we do we should be consistent about it, so I'm okay going to the Go-style case just to keep internal consistency.

dgoodwin · 2019-03-04T16:11:16Z

config/samples/hive_v1alpha1_clusterimageset.yaml

+    pullPolicy: Always
+  install:
+    pullSpec: openshift/installer:latest
+    pullPolicy: Always


This looks like it might be out of sync with the actual type now, perhaps from a previous iteration. It looks like it's just image = string now?

dgoodwin · 2019-03-04T16:13:06Z

pkg/controller/clusterimageset/clusterimageset_controller.go

+	// Watch for jobs created by a ClusterImageSet
+	// The reason that this is not down via ownership is that the ClusterImageSet is a cluster-scoped
+	// resource. Before Kubernetes 1.13, cluster-scoped resources should not have ownership of other
+	// resources.


👍 good comment.

dgoodwin · 2019-03-04T16:13:39Z

pkg/controller/clusterimageset/clusterimageset_controller.go

+		}
+		return reconcile.Result{}, err
+	}
+	logger := log.WithFields(log.Fields{"clusterImageSet": request.NamespacedName.String()})


When you've just got one field there's a nice concise log.WithField() function just for ref.

dgoodwin · 2019-03-04T16:14:59Z

pkg/apis/hive/v1alpha1/clusterimageset_types.go

+	// image. This is used when fetching the release image to extract the installer
+	// image spec.
+	// +optional
+	ReleaseImagePullSecret *corev1.ObjectReference `json:"releaseImagePullSecret,omitempty"`


This is interesting, clearly we need one, I wonder what we should do in prod for this.

dgoodwin · 2019-03-04T16:17:12Z

pkg/controller/clusterimageset/clusterimageset_controller.go

+	if cis.Status.InstallerImage != nil {
+		logger.Debug("clusterImageSet has an InstallerImage set.")
+		err := r.clearStatusErrorResolvingInstallerImage(cis, logger)
+		return reconcile.Result{}, err


Do we have an issue here if the release image gets updated? I would expect that this could happen, in which case we'd want the installer image re-set.

We can add support for that later potentially.

Wouldn't we want to create a new ClusterImageSet at that point? I was thinking of these more as immutable (which reminds me I need to add validation)

That is probably ok, we'd do 4.0beta2 or whatever during times like this.

dgoodwin · 2019-03-04T16:37:43Z

pkg/controller/images/controller_images.go

+
+	// defaultCLIImage is the image to use for the openshift CLI when an image is not specified
+	// via the environment variable
+	defaultCLIImage = "registry.svc.ci.openshift.org/openshift/origin-v4.0:cli"


Good thinking.

app-sre-bot · 2019-03-04T22:41:33Z

Can one of the admins verify this patch?

csrwng · 2019-03-05T23:36:53Z

@dgoodwin this should be ready for another look

dgoodwin

Looks good just a few questions to possibly round it out.

dgoodwin · 2019-03-06T11:56:41Z

pkg/apis/hive/v1alpha1/validating-webhooks/clusterimageset_validating_admission_hook.go

+	}
+
+	if !reflect.DeepEqual(oldObject.Spec, newObject.Spec) {
+		message := "ClusterImageSet.Spec is immutable"


Immutable, but if we really got into a pickle, we could delete and recreate?

dgoodwin · 2019-03-06T12:10:59Z

pkg/imageset/updateinstaller_test.go

+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			client := fake.NewFakeClient(test.existingClusterDeployment)
+			workDir, err := ioutil.TempDir("", "test-update")


Looks like workDir might not be getting cleaned up.

dgoodwin · 2019-03-06T12:16:38Z

pkg/controller/clusterdeployment/clusterdeployment_controller_test.go

+					t.Errorf("did not find expected imageset job")
+				}
+			},
+		},


Could you add a test for deprovision, when the CD references a ClusterImageSet that has since been deleted. I'm not quite sure what we want there but I think it might be ok, we don't need an installer image anymore, if we do start using the installer image to deprovision in future we still have a reference at least to what was used to install the cluster, although that's probably no good anymore. For now though, we'd just use the latest hive image defaulted in the code, which I think is correct as we want it to remain backwards compat.

dgoodwin · 2019-03-06T12:17:16Z

pkg/apis/hive/v1alpha1/clusterdeployment_types.go

+
+	// InstallerImageResolutionFailedCondition is a condition that indicates whether the job
+	// to determine the installer image based on a release image was successful.
+	InstallerImageResolutionFailedCondition ClusterDeploymentConditionType = "InstallerImageResolutionFailed"


Not in this PR, but between this and incoming preflight checks we should start up a document in the repo outlining the possible conditions and what they mean as a contract for us and UHC.

dgoodwin · 2019-03-06T12:21:29Z

config/samples/hive_v1alpha1_clusterimageset.yaml

+  name: clusterimageset-sample
+spec:
+  hiveImage: registry.svc.ci.openshift.org/openshift/hive-v4.0:hive
+  releaseImage: quay.io/openshift-release-dev/ocp-release:4.0.0-0.6


This is probably going to be the primary use case to install a cluster going forward, we should probably factor it into the cluster-deployment.yaml template and e2e tests. We could setup make deploy to automatically setup an openshift-latest that points to latest CI images (assuming there is one for release that we can pull)? Or should we drop it into the clusterdeployment template as another object?

We can just add it in as another object in the clusterdeployment template. If ok with you, I'll handle in a follow-up PR.

Sounds good.

dgoodwin · 2019-03-06T12:44:18Z

pkg/controller/clusterdeployment/clusterdeployment_controller.go

+			return "", err
+		}
+	}
+	return images.GetHiveImage(cdLog), nil


Looks like we fall back to this if IsNotFound, which looks correct. 👍

csrwng · 2019-03-06T15:04:48Z

@dgoodwin rebased and added additional deprovision tests to the clusterdeployment controller. I'll handle the move to using ClusterImageSets by default in a follow-up PR. I'm not sure if there is a "latest" release image, but will definitely look into it.

dgoodwin · 2019-03-06T15:07:10Z

/lgtm

csrwng · 2019-03-06T15:17:49Z

sorry @dgoodwin I missed fixing a comment's case. fixed now

…t controller

dgoodwin · 2019-03-06T15:56:42Z

/lgtm

openshift-ci-robot · 2019-03-06T15:56:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csrwng, dgoodwin

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [csrwng,dgoodwin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2019-03-06T16:46:03Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot requested review from dgoodwin and joelddiaz March 4, 2019 16:03

dgoodwin reviewed Mar 4, 2019

View reviewed changes

csrwng force-pushed the cluster_image_set branch from 0fac0f7 to 34b72c0 Compare March 4, 2019 22:41

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2019

csrwng force-pushed the cluster_image_set branch from 34b72c0 to 4ca04a0 Compare March 5, 2019 23:26

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 5, 2019

csrwng changed the title ~~WIP: Cluster image set~~ Cluster image set Mar 5, 2019

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 Mar 5, 2019

dgoodwin reviewed Mar 6, 2019

View reviewed changes

csrwng force-pushed the cluster_image_set branch from 4ca04a0 to 80fbeca Compare March 6, 2019 15:03

openshift-ci-robot assigned dgoodwin Mar 6, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2019

csrwng force-pushed the cluster_image_set branch from 80fbeca to d2f4f23 Compare March 6, 2019 15:16

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2019

Add ClusterImageSet CRD and InstallerImage update in ClusterDeploymen…

d2f4f23

…t controller

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2019

openshift-merge-robot merged commit 9b55455 into openshift:master Mar 6, 2019

csrwng deleted the cluster_image_set branch March 8, 2019 19:36

dgoodwin mentioned this pull request Mar 13, 2019

WIP: *: Extract installer from the target update payload #210

Closed

Cluster image set #241

Cluster image set #241

Uh oh!

Conversation

csrwng commented Mar 4, 2019

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!

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!

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!

app-sre-bot commented Mar 4, 2019

Uh oh!

csrwng commented Mar 5, 2019

Uh oh!

dgoodwin left a comment

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!

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!

csrwng commented Mar 6, 2019

Uh oh!

dgoodwin commented Mar 6, 2019

Uh oh!

csrwng commented Mar 6, 2019

Uh oh!

dgoodwin commented Mar 6, 2019

Uh oh!

openshift-ci-robot commented Mar 6, 2019

Uh oh!

openshift-bot commented Mar 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!