Implement a Golang cluster installer job that hides the template #288

smarterclayton · 2019-03-05T03:30:41Z

Take the existing cluster-launch-installer-e2e template and hide it inside
a e2e step definition. Support an upgrade flag that alters the parameters
passed to the template such that the installer image from the input (either
release tag or an actual payload) is used to install, then launch the
upgrade tests from the job definition.

This sets the path for moving this functionality into the operator.

The user still has to upload the right cluster profile secret, but now we
print an error if we don't find it (for stronger typing). Regular users can
launch an upgrade from a single command.

Builds off #287

stevekuznetsov

wowza

stevekuznetsov · 2019-03-06T01:47:35Z

ARCHITECTURE.md

 2018/10/08 05:29:15 Building machine-config-operator
 2018/10/08 05:29:15 Building machine-config-controller
 2018/10/08 05:31:47 Build machine-config-server succeeded after 2m28s
-2018/10/08 05:31:47 Tagging machine-config-server into stable


We removed this months ago.

stevekuznetsov · 2019-03-06T01:47:47Z

cmd/ci-operator/main.go

 	}

-	if len(o.secrets) > 0 {
-		log.Printf("Populating secrets for test")


Not actually useful, was just noise. We already print when we populate a secret.

stevekuznetsov · 2019-03-06T01:49:39Z

pkg/steps/clusterinstall/clusterinstall.go

+	step := steps.TemplateExecutionStep(template, params, podClient, templateClient, artifactDir, jobSpec)
+	subTests, ok := step.(nestedSubTests)
+	if !ok {
+		panic(fmt.Sprintf("unexpected %T", step))


this sounds fun

I could make the step return an error. No reason we can't.

stevekuznetsov · 2019-03-06T01:49:54Z

pkg/steps/clusterinstall/clusterinstall.go

+}
+
+func (s *e2eTestStep) Inputs(ctx context.Context, dry bool) (api.InputDefinition, error) {
+	return nil, nil


The e2e test step doesn't load any unique input from the graph that would alter the hash used to guarantee unique namespaces.

stevekuznetsov · 2019-03-06T01:50:09Z

pkg/steps/clusterinstall/clusterinstall.go

+}
+
+func (s *e2eTestStep) Done() (bool, error) {
+	return false, nil


Done isn't used, suggest we remove it.

I thought we used it to trigger children?

stevekuznetsov · 2019-03-06T01:51:08Z

pkg/steps/template.go

 	}

-	addArtifactsToTemplate(s.template)
+	if len(s.artifactDir) > 0 {


Should this have failed a test somewhere?

I'll add a test, but it only occurs when you don't ask for artifacts.

stevekuznetsov · 2019-03-06T01:51:28Z

This doesn't seem finished

smarterclayton · 2019-03-06T03:07:37Z

What isn't finished?

smarterclayton · 2019-03-07T02:45:22Z

I'm iterating on this and cleaning up issues related to dependencies.

/hold

I may create a test ci-operator image and set up test prow jobs running against it to let them soak.

Print the full graph, with each line being an edge between nodes with a space between them. Works with the `digraph` go utility to let you debug ordering problems between graph nodes instead of inventing our own tools.

smarterclayton · 2019-03-07T22:30:47Z

Ok, removing hold. Have tested in a normal e2e-aws job and an e2e-aws-upgrade job on prow (using slightly different config settings to verify). The most significant issue was not using the parameters map to control access to env var checking - we were actually doing it wrong in one place and that caused other jobs to flake.

Added --print-graph to make it easier to spot graph failures - that output can be used with digraph.

smarterclayton · 2019-03-07T22:30:52Z

/hold cancel

Take the existing cluster-launch-installer-e2e template and hide it inside a e2e step definition. Support an `upgrade` flag that alters the parameters passed to the template such that the installer image from the input (either release tag or an actual payload) is used to install, then launch the upgrade tests from the job definition. This sets the path for moving this functionality into the operator. The user still has to upload the right cluster profile secret, but now we print an error if we don't find it (for stronger typing). Regular users can launch an upgrade from a single command. Remove the previous behavior of not returning parameter links when the env var is set - the logic in defaults.go is sufficient to prune steps that are fully specified. The release command now cannot be overriden by env (it imports when env is specified) and must always be run.

stevekuznetsov · 2019-03-09T00:05:47Z

/lgtm

openshift-ci-robot · 2019-03-09T00:06:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton, stevekuznetsov

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~~ [smarterclayton,stevekuznetsov]

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

* Update release periodics to use the new parameters in ci-operator. * Add the upgrade release periodic for use with ci-operator, mark optional * Fix some misplaced roles * Add a promotion periodic for machine-os-content from rhcos/maipo:latest to ocp/4.0 Requires changes to ci-operator-prowgen and openshift/ci-operator#288 first but shows what the jobs will look like.

This came in with ad5a4e3 (Implement a Golang cluster installer job that hides the template, 2019-03-04, openshift#288), but the lack of 'oc adm release new' logs is making it hard to debug broken ImageStream retrieval [1]. I don't understand the bulk of that commit, so hopefully this narrow pivot doesn't break anything... [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1707928#c3

openshift-ci-robot requested review from petr-muller and stevekuznetsov March 5, 2019 03:30

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 5, 2019

smarterclayton force-pushed the upgrade_initial branch from 9d58ceb to 9375085 Compare March 5, 2019 05:40

smarterclayton mentioned this pull request Mar 5, 2019

Add a presubmit job to test upgrades on the CVO repo openshift/release#2997

Closed

stevekuznetsov reviewed Mar 6, 2019

View reviewed changes

smarterclayton force-pushed the upgrade_initial branch from 9375085 to 12e0403 Compare March 6, 2019 20:02

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2019

smarterclayton force-pushed the upgrade_initial branch from 12e0403 to 36f0e04 Compare March 7, 2019 03:05

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 7, 2019

This was referenced Mar 7, 2019

Add one of the new style upgrade tests to ci-operator openshift/release#3030

Merged

The new upgrade job is supported as part of the ci-operator openshift/ci-operator-prowgen#102

Merged

smarterclayton force-pushed the upgrade_initial branch from 36f0e04 to 1eadbc0 Compare March 7, 2019 05:43

smarterclayton force-pushed the upgrade_initial branch 3 times, most recently from 38daae2 to 3dbf16e Compare March 7, 2019 18:27

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2019

smarterclayton force-pushed the upgrade_initial branch from 3dbf16e to ad5a4e3 Compare March 8, 2019 17:17

openshift-ci-robot assigned stevekuznetsov Mar 9, 2019

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

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

wking mentioned this pull request May 8, 2019

pkg/steps/release/create_release: Drop SkipLogs for release-* pods #343

Open

Implement a Golang cluster installer job that hides the template #288

Implement a Golang cluster installer job that hides the template #288

Uh oh!

Conversation

smarterclayton commented Mar 5, 2019

Uh oh!

stevekuznetsov 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!

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!

stevekuznetsov commented Mar 6, 2019

Uh oh!

smarterclayton commented Mar 6, 2019

Uh oh!

smarterclayton commented Mar 7, 2019

Uh oh!

smarterclayton commented Mar 7, 2019

Uh oh!

smarterclayton commented Mar 7, 2019

Uh oh!

stevekuznetsov commented Mar 9, 2019

Uh oh!

openshift-ci-robot commented Mar 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants